Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Riot is unable to deactivate and erase users with Synapse v1.13.0rc1 #13645

Closed
anoadragon453 opened this issue May 12, 2020 · 4 comments · Fixed by matrix-org/matrix-react-sdk#4584

Comments

@anoadragon453
Copy link
Member

Unfortunately a change in Synapse that now blocks User-Interactive Authentication session parameters changing within a session breaks deactivating and erasing a user in Riot: matrix-org/synapse#7471

I'm not sure whether this is a Synapse or Riot bug. The spec isn't entirely clear about whether a session can change mid-way through. Although I understand that it's important to consider that there may be other clients out there (and other parts of Riot) that this will break.

Fixing this in the Riot side is probably as easy as just starting a new UIA session when the "erase" checkbox is toggled.

@turt2live
Copy link
Member

possibly related: #8458

We start a session to figure out what the server supports so we can display the right UI (as a wizard-style setup here is not ideal). Starting a new session is probably what we're going to have to do, though it's super risky from our side (as the server could change its flows).

The spec definitely needs clarification here, and might need a few MSCs to better handle the situation.

@turt2live
Copy link
Member

@vector-im/design will have to consider the various flows here, as checking or unchecking the box will cause us to flip flop mid-session. Should we throw away the auth attempt? Disable the checkbox after some heuristic? etc

@turt2live
Copy link
Member

Right, so in discussion with the backend team: riot is indeed non-compliant because the spec clearly says the same request is supposed to be submitted (see Synapse issue for details on where we change the request).

However, we don't appear to be in a position to do an emergency release at the moment, and this requires a non-trivial amount of design work (probably). Having the exemption for all UIA endpoints, as done for /register already, would be appreciated so we can fix this with a bit more than zero notice.

@turt2live
Copy link
Member

and of course things move quickly - we might not need design's involvement as urgently and can keep this on the backburner for now. We can work around it in riot and release eventually (when we're safe and able to), and work on the spec/design sides as a tangent.

I've opened #13646 to track the longer tail of work on this. Also I'm pushing this down in the priority queue a bit as it's not as urgent as when opened.

@turt2live turt2live self-assigned this May 12, 2020
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue May 12, 2020
Fixes element-hq/element-web#13645

Every time the checkbox value changes we acquire a new session now. This avoids us asking the server to change its direction partway through the request.

This causes a bit of UI jerk as the dialog goes from auth -> loading -> auth, however it's better than the alternative of reworking the entire UIA structure to support the `authData` dict changing. Originally this commit consisted of a `disabled` flag on the `InteractiveAuth` component which carried through to the stage's component, however it turns out that stack doesn't respect changes to the `authData` prop, which means the session ID we eventually send down is wrong (`erase: false` instead of the one with `erase: true`). Therefore, we do some logic to ensure we remount `InteractiveAuth` completely.

Further work in this area is described in element-hq/element-web#13646
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants