Implement a message for user account suspension, as per MSC3823. #8722

Yoric · 2022-05-31T09:14:24Z

Users may have their account temporarily suspended from a homeserver,
e.g. due to ToS violations. This prevents them from sending new messages
but not from reading messages or redacting their own messages.

This patch displays an error message comparable to "some of your messages have not been sent".

See matrix-org/synapse#12845 for more details.

Here's what your changelog entry will look like:

✨ Features

Implement a message for user account suspension, as per MSC3823. (#8722). Contributed by @Yoric.

Yoric · 2022-05-31T09:16:18Z

Unfortunately, I have no idea how to write a test for such a feature.

Users may have their account temporarily suspended from a homeserver, e.g. due to ToS violations. This prevents them from sending new messages but not from logging, reading messages or redacting their own messages. This patch displays an error message comparable to "some of your messages have not been sent". See matrix-org/synapse#12845 for more details. Notes: Support for MSC3823. Display an error message if the user's account has been suspended. Type: enhancement

Yoric · 2022-05-31T11:08:58Z

The test failure seems to be a timeout in unrelated code.

turt2live

generally lgtm - just concerns about the code style (some of which should be enforced by the linter, but empirically aren't).

Please also include screenshots to make design review easier, as they will need to look at the copy.

src/components/structures/RoomStatusBar.tsx

turt2live · 2022-05-31T21:45:51Z

For testing: a cypress test would be appreciated. It allows you to run a Synapse and call endpoints, which should be enough to trigger and test suspension.

Yoric · 2022-06-02T05:54:05Z

Matching screenshot:

Yoric · 2022-06-02T07:18:20Z

Also, test added.

t3chguy · 2022-06-02T07:36:28Z

@Yoric that screenshot looks rather broken

It is likely even worse with different scaling/font settings.

Yoric · 2022-06-02T07:58:08Z

@Yoric that screenshot looks rather broken

It is likely even worse with different scaling/font settings.

Yes, I believe that this is what what the status bar currently looks like on 1000x660 (the default size for cypress), even without my patch.

Here's a screenshot with a more reasonable 1280x1024:

Yoric · 2022-06-02T08:00:29Z

The cypress test passes locally but fails with the following error message in CI:

Error: Can't use 'tar -xzf' extract archive file: /home/runner/work/_actions/_temp_00e45d6c-6a29-4cbf-a707-3052823681c4/7b22363b-0b56-4817-8fec-0cd31a12eaad.tar.gz. return code: 2.

I don't have the authorization to relaunch tests.

turt2live

I've also re-run the cypress tests in hopes of fixing them. The cypress test looks valid, though we should structure the code around it to match our sort of best practices rather than past mistakes.

turt2live · 2022-06-02T22:25:56Z

cypress/integration/9-room-status-bar/account-suspension.ts

+        //cy.stopSynapse(synapse);
+    });
+
+    const TEXT = "Hello, world";


variables should be grouped together, and this is of questionable case - what do the other tests do?

Moving things up, but I don't understand what makes this questionable. TEXT is used ~6 times around the code, why shouldn't it be a constant?

it should be a constant, but we don't usually mix all caps and lowerCamelCase in our definition bodies.

turt2live · 2022-06-02T22:26:29Z

cypress/integration/9-room-status-bar/account-suspension.ts

+
+    // Pretend to send a message but inject an error response.
+    function sendWithErrorResponse(text, response) {
+        cy.intercept("http://localhost:*/_matrix/client/r0/rooms/*/send/m.room.message/*", req => {


can we not just call a synapse API to suspend the user?

can we not just call a synapse API to suspend the user?

Not yet.

I'm not entirely comfortable landing this feature if there's no way to use it...

Well, we have a chicken and egg problem.

Our process is:

Write a MSC (done).

Write a prototype as a spam-check module for Synapse (WIP, close to done).

Write a front-end (this).

Test it (blocked by 2 & 3).

Once we're satisfied that it works, port the prototype to Synapse proper.

Testing shouldn't be blocked by a frontend implementation, otherwise we'd get nothing done :p

This sitting on a branch/here is fine while the rest of the feature is worked out, particularly considering this will be needing design review as well.

For the test here it's moderately important that we don't just intercept random endpoints and instead use the API provided by Synapse to do things: this means getting an admin API to suspend a user.

We cannot land this without suitable tests, so to unblock this the tests need to be written. It is currently the only blocker for this PR.

I'm currently doing my best to find a way to write a test that matches your criteria without having to:

throw away several weeks worth of work by three people;

start several more weeks of work in a different direction that doesn't match our requirements.

Is there any chance you could find an alternative solution?

The alternative is we don't land this, I'm afraid. Tests are a hard requirement and I can't single-handedly bypass the requirements we've set out for our testing infrastructure.

A plausible test is one which installs the spam checker module onto a dedicated synapse (since we can support running different flavours of Synapse already) and trigger that with a message of some kind.

I do not believe that any of the proposed solutions I've raised are multiple weeks worth of work. Please engage the web team sooner to avoid having to reconsider several people's work.

also, apologies, the other blocker I missed is the CSS issue.

A plausible test is one which installs the spam checker module onto a dedicated synapse (since we can support running different flavours of Synapse already) and trigger that with a message of some kind.

Very well, if that's what it takes. What's the procedure to create a dedicated Synapse for use with the front end's cypress tests?

We have a "consent" template already that I think you should be able to largely copy/paste. Visiting #element-dev:matrix.org would be better than asking here though, to spread the support load and get an answer more quickly.

cypress/integration/9-room-status-bar/account-suspension.ts

src/components/views/voip/CallView.tsx

t3chguy · 2022-06-06T06:57:20Z

Here's a screenshot with a more reasonable 1280x1024:

Until you open the right panel, surely?

Yoric · 2022-06-06T07:49:06Z

Here's a screenshot with a more reasonable 1280x1024:

Until you open the right panel, surely?

You are right.

Note that the problem already shows up prior to this patch, though.

I would suggest fixing this for good in a followup issue, as I clearly do not have a fine enough understanding of the CSS of Element Web for this purpose, plus this feels like a bit too much scope extension for this PR.

Yoric · 2022-06-06T09:10:41Z

Note: Latest cypress error seems to be a timeout in unrelated code.

turt2live · 2022-06-07T06:10:14Z

cypress/integration/9-room-status-bar/account-suspension.ts

+        cy.get(".mx_RoomView_body .mx_EventTile").contains(".mx_EventTile[data-scroll-tokens]", text);
+
+        // Give an error a little time to show up. It shouldn't.
+        cy.wait(1_000);


we don't use this syntax:

Suggested change

cy.wait(1_000);

cy.wait(1000);

Will do, but may I suggest amending this policy? 16_777_384 is much more readable than 16777384.

I personally disagree on that, tbh :p

I'm not sure what others on the team think, but I suspect since we haven't done so up until now we aren't likely to remember to do it, so will end up fighting the linter instead of working with it.

turt2live · 2022-06-07T06:12:37Z

cypress/integration/9-room-status-bar/account-suspension.ts

+
+// Pretend to send a message but inject an error response.
+function sendWithErrorResponse(text, response) {
+    cy.intercept("http://localhost:*/_matrix/client/r0/rooms/*/send/m.room.message/*", req => {


sorry, I really think this needs to be an admin API call and not something we intercept. We should be testing that we behave correctly when Synapse tells us the account is suspended, not when we lie to it (otherwise we'd write an entirely different, less helpful, test).

That feels like it would block an ongoing project.

Could we proceed as follows?

Land it with the intercept.

Perform the tests that we cannot perform until this feature has landed.

Design an admin API.

Spend a few weeks (or however long is required) bikeshedding, implementing and landing the admin API(s) with back-end.

Once we have an admin API, add a second test (or replace the existing test with a new one) that uses the admin API?

It would block the project, yes. Landing partial or incomplete code isn't viable as an option: we've literally never returned to temporary code, despite several promises that we will. We no longer take the chance, sorry.

cypress/integration/9-room-status-bar/account-suspension.ts

turt2live · 2022-06-07T06:16:29Z

src/i18n/strings/en_EN.json

+    "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. To learn more, visit <detailsLink>this page</detailsLink>.": "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. To learn more, visit <detailsLink>this page</detailsLink>.",
+    "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. Please contact the administrator of %(homeserverDomain)s.": "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. Please contact the administrator of %(homeserverDomain)s.",


as already mentioned by @t3chguy, the status bar itself looks broken with this change, so we'd need to fix that before landing this PR.

The options for that are:

Wait for someone on the web team to get to it in the backlog (slow, and likely way too slow for your timelines).

Get your hands dirty and learn some CSS 😇

Find someone who is able to help with writing that CSS and have them contribute ideas/guidance/code to this PR

Personally, I'd encourage option 2, particularly if you're expecting to be in the Element Web area for future changes.

As already responded to @t3chguy, the status bar itself is already broken prior to this change.

I would really, really, really like to avoid scope creep. Can we please postpone this to a followup issue?

Even if it was broken before, we can't reasonably land something which has a near-100% chance of causing the bug. It just doesn't line up with our code quality requirements, sorry.

turt2live · 2022-06-07T06:16:59Z

src/i18n/strings/en_EN.json

+    "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. To learn more, visit <detailsLink>this page</detailsLink>.": "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. To learn more, visit <detailsLink>this page</detailsLink>.",
+    "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. Please contact the administrator of %(homeserverDomain)s.": "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. Please contact the administrator of %(homeserverDomain)s.",


for a future review iteration: design will need to be consulted for the message copy here.

Who exactly?

can either get copy review internally from Write Club, or we can request design review once this hits a later stage of code review.

Who's/what's Write Club?

Search the corp space :)

andybalaam · 2023-11-09T11:32:30Z

It looks like progress here has halted for now. Thank you for your work on this. The code and the Netlify build will continue to exist, so this can still be used to validate the MSC if needed, but I'm going to close the PR until it's being actively worked on.

Yoric requested a review from a team as a code owner May 31, 2022 09:14

Yoric force-pushed the yoric/msc3823 branch from 9ee031b to b2b7c1f Compare May 31, 2022 09:15

Yoric added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label May 31, 2022

Yoric marked this pull request as draft May 31, 2022 09:18

Yoric force-pushed the yoric/msc3823 branch 2 times, most recently from 988688f to cd2e636 Compare May 31, 2022 09:28

Yoric force-pushed the yoric/msc3823 branch from cd2e636 to c7d71ac Compare May 31, 2022 10:53

Yoric marked this pull request as ready for review May 31, 2022 10:59

turt2live suggested changes May 31, 2022

View reviewed changes

t3chguy and others added 2 commits June 1, 2022 10:00

Merge branch 'develop' into yoric/msc3823

31fddee

WIP: Applied feedback

3dc90b2

github-actions bot deployed to Netlify June 1, 2022 09:25 View deployment

WIP: Applied feedback

87062c5

Yoric force-pushed the yoric/msc3823 branch from d948192 to 87062c5 Compare June 1, 2022 10:52

WIP: Trying to get cypress test to run

feab369

Yoric requested a review from turt2live June 2, 2022 07:17

turt2live suggested changes Jun 2, 2022

View reviewed changes

Yoric added 4 commits June 3, 2022 10:43

WIP: Now with tests

58af5fb

WIP: Lint

b6edb8a

WIP: Oops, removing bogus file

23dc77f

WIP: Feedback

44310a0

Yoric force-pushed the yoric/msc3823 branch from cc6cad9 to 44310a0 Compare June 3, 2022 08:45

Yoric requested a review from turt2live June 3, 2022 08:46

turt2live removed their request for review June 3, 2022 15:34

Yoric added 2 commits June 3, 2022 17:39

WIP: Don't stop Synapse during the tests

3e964aa

WIP: Style nits

d5e422b

Yoric added 2 commits June 6, 2022 10:43

WIP: Hunting down a heisenbug in CI

7af04fc

WIP: Lint

e72d240

Yoric requested a review from turt2live June 7, 2022 06:02

turt2live suggested changes Jun 7, 2022

View reviewed changes

Yoric mentioned this pull request Jun 17, 2022

CI: Supporting building custom Docker images for cypress #8862

Closed

andybalaam closed this Nov 9, 2023

		"Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. To learn more, visit <detailsLink>this page</detailsLink>.": "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. To learn more, visit <detailsLink>this page</detailsLink>.",
		"Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. Please contact the administrator of %(homeserverDomain)s.": "Your message wasn't sent. Your account is suspended: you cannot send messages, invites or join new rooms. Please contact the administrator of %(homeserverDomain)s.",

Implement a message for user account suspension, as per MSC3823. #8722

Implement a message for user account suspension, as per MSC3823. #8722

Conversation

Yoric commented May 31, 2022 • edited by github-actions bot Loading

✨ Features

Yoric commented May 31, 2022

Yoric commented May 31, 2022

turt2live left a comment

Choose a reason for hiding this comment

turt2live commented May 31, 2022

Yoric commented Jun 2, 2022

Yoric commented Jun 2, 2022

t3chguy commented Jun 2, 2022

Yoric commented Jun 2, 2022 • edited Loading

Yoric commented Jun 2, 2022

turt2live left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoric Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yoric Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t3chguy commented Jun 6, 2022

Yoric commented Jun 6, 2022

Yoric commented Jun 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybalaam commented Nov 9, 2023

Yoric commented May 31, 2022 •

edited by github-actions bot

Loading

Yoric commented Jun 2, 2022 •

edited

Loading

Yoric Jun 3, 2022 •

edited

Loading

Yoric Jun 15, 2022 •

edited

Loading