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

MSC3939: Account locking #3939

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

MSC3939: Account locking #3939

wants to merge 11 commits into from

Conversation

babolivier
Copy link
Contributor

@babolivier babolivier commented Nov 24, 2022

@babolivier babolivier changed the title MSCXXXX: Account locking MSC3939: Account locking Nov 24, 2022
@turt2live turt2live added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Nov 24, 2022
@aceArt-GmbH

This comment was marked as resolved.

@babolivier
Copy link
Contributor Author

Seems to overlap a bit with #3823 ;)

See the Alternatives section :)

Also please write comments on MSCs in threads to prevent clottering the timeline 😅

@babolivier
Copy link
Contributor Author

babolivier commented Jul 3, 2023

PSA: I don't really wish to spend time maintaining this MSC anymore (as it was written in the context of work related to a role I no longer hold; plus I don't have write access to this branch anymore). This MSC should be considered unmaintained/abandoned unless someone else wishes to take it over (in which case, be my guest).

@MatMaul
Copy link

MatMaul commented Jul 5, 2023

Thanks for your work @babolivier. I am going to take this over.

@MatMaul
Copy link

MatMaul commented Aug 16, 2023

An implementation has been merged in synapse and will be available in synapse 1.91.

@turt2live
Copy link
Member

Aside from some editorial comments, I think this is largely ready to go.

SCT members should feel welcome to comment on the user directory thread.

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Nov 27, 2023

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

  • 401/403 behaviour
  • general clarity

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. disposition-merge labels Nov 27, 2023
@turt2live turt2live removed the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Nov 27, 2023
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
@dbkr
Copy link
Member

dbkr commented Nov 28, 2023

@mscbot concern 401/403 behaviour

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Nov 28, 2023
@dbkr
Copy link
Member

dbkr commented Nov 28, 2023

@mscbot concern general clarity

#3939 (comment) mostly

proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Show resolved Hide resolved
@dbkr
Copy link
Member

dbkr commented Nov 30, 2023

@mscbot concern general clarity

(duplicate because I spelt MSC wrong the first time and the bot doesn't understand edits)

@turt2live turt2live added this to Ready for FCP ticks in Spec Core Team Backlog Dec 21, 2023
@turt2live
Copy link
Member

This MSC is on a shortlist for considerations in Matrix 1.11's release cycle, and so I'm taking it on under my Element T&S hat in service of the Foundation.

@turt2live turt2live self-assigned this Mar 12, 2024
@turt2live turt2live requested a review from dbkr March 21, 2024 04:20
@turt2live
Copy link
Member

@dbkr I believe I've addressed your concerns, but please feel free to raise new threads as needed.

proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
proposals/3939-account-locking.md Outdated Show resolved Hide resolved
@turt2live turt2live requested a review from dbkr March 26, 2024 23:53
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@dbkr
Copy link
Member

dbkr commented Mar 28, 2024

@mscbot resolve general clarity

@mscbot
Copy link
Collaborator

mscbot commented Mar 28, 2024

Unknown concern 'general clarity'.

@dbkr
Copy link
Member

dbkr commented Mar 28, 2024

@mscbot resolve 401/403 behaviour

@mscbot
Copy link
Collaborator

mscbot commented Mar 28, 2024

Unknown concern '401/403 behaviour'.

@turt2live turt2live removed the unresolved-concerns This proposal has at least one outstanding concern label Apr 1, 2024
@turt2live
Copy link
Member

I've manually resolved the concerns for now.

proposals/3939-account-locking.md Outdated Show resolved Hide resolved
Clients SHOULD hide the normal UI from the user when informing them that their
account is locked, preventing general use of the account.

Clients SHOULD continue to `/sync` and make other API calls to more quickly detect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should the server behave when /sync is called? If it returns immediately with the 401 Unauthorized, and then the client immediately retries, it sounds like this would cause unnecessary traffic. So presumably the server should wait until the timeout is reached before sending the 401, unless the account gets unlocked before? Though I guess it should send the 401 immediately the first time that /sync gets called after the account gets locked, so that the user is notified immediately?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also found this bit weird since it says /sync and make other API calls. I assumed it means the client should make any API calls periodically, which could be /sync or /account/whoami or whatever.

If /sync is getting a new kind of 401 long polling, it definitely needs to be documented explicitly. If not, the section should probably be clarified that clients should just periodically call any API (but not in a tight loop like /sync)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the missing language is that clients should back off for some time, as if they were rate limited. I don't think /sync neccessarily gets a new long poll mechanism here, but clients do need to do something to discover when an account is no longer locked. Element Web for example will back off on /sync errors, then loop a /versions, /push_rules/, /sync call order - if all of these endpoints return success codes, the client resumes normal operation.

The bit about "other API calls" is mostly to acknowledge that clients do not linearly send requests. An account may be locked in the middle of a user scrolling through backlog: there's /messages, /send/m.receipt, and /sync all happening at that point - the client should recognize that an error on any of them is assumed to be an error on all of them.

(if this is clearer, happy to translate it to the MSC)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it would be great to spell out that clients should back off - it does sound rather intuitive but the special nature of /sync makes it easy to get confused.

Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
account is locked, preventing general use of the account.

Clients SHOULD continue to `/sync` and make other API calls to more quickly detect
when the lock has been lifted. If unlocked, the APIs will either return a different
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
when the lock has been lifted. If unlocked, the APIs will either return a different
when the lock has been lifted. However, clients should rate-limit their requests. If unlocked, the APIs will either return a different

to address the thread above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API disposition-merge kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period.
Projects
Spec Core Team Backlog
  
Ready for FCP ticks
Status: Scheduled - v1.10
Development

Successfully merging this pull request may close these issues.

None yet