matrix-org
MSC1692: Terms of service at registration
Implementation: https://github.com/matrix-org/synapse/pull/4004
Notes from 2018:
This proposal is the first to try a new process where we avoid creating excess issues with locked discussions. Discussion about this proposal should happen on this PR, preferably inline where possible.
Riot/synapse is going ahead and trying out the registration portion of this proposal to fix a UX bug. Given UI auth has fallback support, this should not break any clients and can be easily modified in the future if this proposal were to change. Implementation is required as a proof of concept for the proposal anyways.
Also I just found https://github.com/matrix-org/matrix-doc/issues/1231 which doesn't appear to have a conclusion to it. This proposal also alters how https://github.com/matrix-org/matrix-doc/issues/1252 works.
This looks mostly sane I guess, though I don't really know much about registration/login.
One thing that I'm unsure of is the behaviour when we need the user to agree to an updated ToS. Locking down the APIs and returning 403s has a number of issues:
- Server side its a bit annoying to keep track of which APIs are safe to allow the user access to and which aren't. Denying everything but sync doesn't terribly worthwhile given the likelihood that clients will want to make API call when actually rendering things to client (e.g. avatars)
- Breaking people's messaging app is unlikely to make people particularly happy. I think we need to make sure that clients get proactively told that action needs to be taken, and ideally allow being notified in advance (long before the API gets blocked).
I also think it'd make more sense to return a 401 with a specific error code. That way "dumb" clients just log out and the user can log in again and accept the updated ToS. Smarter clients can interpret the 401 and show a "accept ToS" screen.
I agree with almost everything here, except the need for a new endpoint.
I strongly disagree with /terms unless someone can explain what makes it special enough to warrant it's own endpoint in favour of developing PUT /account_data with the optional ability to request auth first. It would definitely benefit future proposals, even if it doesn't save any effort for this proposal.
It seems like it's going to take the same amount of effort whichever way this goes, so why not make this endpoint generic now to save some effort for future proposals.
@erikjohnston
Breaking people's messaging app is unlikely to make people particularly happy. I think we need to make sure that clients get proactively told that action needs to be taken, and ideally allow being notified in advance (long before the API gets blocked).
There's nothing stopping the homeserver from saying a policy is not required and then later requiring it. The proposal should be fairly supportive of whatever the homeserver admin wants to do by not being overly opinionated.
I also think it'd make more sense to return a 401 with a specific error code. That way "dumb" clients just log out and the user can log in again and accept the updated ToS. Smarter clients can interpret the 401 and show a "accept ToS" screen.
Logging the user out sounds really bad for dumb clients. If that dumb client has e2e support (which is not impossible) then that user could lose data. This is probably another great case where a soft logout would work, although it would probably need to be carefully executed.
@Half-Shot
I strongly disagree with /terms unless someone can explain what makes it special enough to warrant it's own endpoint in favour of developing PUT /account_data with the optional ability to request auth first. It would definitely benefit future proposals, even if it doesn't save any effort for this proposal.
I'd like to see a strong use case for other cases where UI auth is helpful on the account data. As previously mentioned several times, I do not see the benefit in adding such a feature where it results in complicated client code.
Sure. I think I have failed to explain where this could also be useful, so I will write up a through explanation this evening. (This can serve as a placeholder)
I've made some changes to this based on the feedback above:
- Multiple languages are now supported
- An ID is introduced for each policy for clients to reference
- The login UI auth is fixed to represent the actual specification. In the future, we should consider actually cutting over to using UI auth on login and not doing half the job (which is proposed here).
- Misc wording and language changes
I have not changed the proposal regarding introduction of a new endpoint. I've spent the better part of the week thinking about whether or not it is worth it, and I'm still not convinced that having UI auth on account data makes sense for primarily backwards compatibility reasons. Older clients would likely see the 401 as the user's token suddenly not being valid and log them out, which is a lot more dangerous than having another endpoint. Although older clients would not be accessing the terms metadata as they would not understand it, I don't think the proposal to add UI auth to account data would be worth it unless it was permitted on any event type - which could cause problems when someone inevitably blanket-enables it in their homeserver.
Ideally in a major version change of the spec we could support UI auth on most endpoints, not just account data, therefore adding an option for more security-conscious environments. For example, a homeserver may rightfully want to prevent the user from uploading content until they acknowledge that they actually own the content. Another scenario would be the homeserver requiring a user to refresh their login details after some time - previously we had a token refresh API however I don't believe it was supportive of alternative login flows, such as TOTP, being required to authenticate a user.
I also think it'd make more sense to return a 401 with a specific error code. That way "dumb" clients just log out and the user can log in again and accept the updated ToS. Smarter clients can interpret the 401 and show a "accept ToS" screen.
Logging the user out sounds really bad for dumb clients. If that dumb client has e2e support (which is not impossible) then that user could lose data. This is probably another great case where a soft logout would work, although it would probably need to be carefully executed.
The soft logout MSC has now landed fwiw, so e2e clients should now be able to more gracefully handle getting logged out. Given that, I would really strongly prefer responding with 401 if they haven't accepted the new ToS with suitable error codes. Smart clients will do the right thing, while dumb clients will simply log out, thus forcing the user to go and accept the new terms. (Hopefully the majority of people will accept the updated ToS before it becomes required.)
My major concern with providing blank /sync responses is that dumb clients will just silently break rather than inform the user that action is required. This would be particularly bad on mobiles given people often won't bother opening the app (and thus start to see errors) unless they receive a push notification, but they won't receive a push notification because when the app tries to fetch the message down /sync it doesn't see any new messages. For a messaging app, silently dropping message is basically one of the worst things that can happen.
Having said that, we should ensure that when a client gets logged out it actually does inform the user. Otherwise logging out will be just as bad.
I have not changed the proposal regarding introduction of a new endpoint. I've spent the better part of the week thinking about whether or not it is worth it, and I'm still not convinced that having UI auth on account data makes sense for primarily backwards compatibility reasons. Older clients would likely see the 401 as the user's token suddenly not being valid and log them out, which is a lot more dangerous than having another endpoint. Although older clients would not be accessing the terms metadata as they would not understand it, I don't think the proposal to add UI auth to account data would be worth it unless it was permitted on any event type - which could cause problems when someone inevitably blanket-enables it in their homeserver.
Agreed, as mentioned here I think it adds unnecessary complexity to both clients and the spec. I don't really see why people hate adding new APIs so much when they will require special casing in clients and servers anyway.
Ideally in a major version change of the spec we could support UI auth on most endpoints, not just account data, therefore adding an option for more security-conscious environments. For example, a homeserver may rightfully want to prevent the user from uploading content until they acknowledge that they actually own the content.
To be honest this sounds overly complicated for something that doesn't sound like it adds a huge amount of value. I'm not sure I've ever come across a site that doesn't require you to accept updated ToS upfront?
Another scenario would be the homeserver requiring a user to refresh their login details after some time - previously we had a token refresh API however I don't believe it was supportive of alternative login flows, such as TOTP, being required to authenticate a user.
This sounds like an orthogonal concern, as its about requiring you to refresh your session rather than certain actions requiring additional authentication.
Oh, and we probably want to think a little bit about how we do the different languages, as it'll start coming up a lot more in future APIs, and it'd be sad if we added something that works just for this stuff but not others.
OTHER THAN ALL THAT, yay this is looking good, and I'm happy we're wanting to move terms of service stuff into the registration and login flows \o/
The soft logout MSC has now landed fwiw, so e2e clients should now be able to more gracefully handle getting logged out. Given that, I would really strongly prefer responding with 401 if they haven't accepted the new ToS with suitable error codes. Smart clients will do the right thing, while dumb clients will simply log out, thus forcing the user to go and accept the new terms. (Hopefully the majority of people will accept the updated ToS before it becomes required.)
Although I have confidence in the soft logout MSC, I want to see it implemented in practice before doubling down on using it here. I'm more than happy to have this MSC be blocked on the implementation of soft logout if needed.
Ideally in a major version change of the spec we could support UI auth on most endpoints, not just account data, therefore adding an option for more security-conscious environments. For example, a homeserver may rightfully want to prevent the user from uploading content until they acknowledge that they actually own the content.
To be honest this sounds overly complicated for something that doesn't sound like it adds a huge amount of value. I'm not sure I've ever come across a site that doesn't require you to accept updated ToS upfront?
In a theoretical future proposal, one could argue that uploading files of type application/company_secrets requires you to scan your thumbprint or something. For this proposal, it is very much intended that someone be forced to sign terms of service before continuing, although in practice there's other documents that are less critical and don't require an in-your-face notification.
Oh, and we probably want to think a little bit about how we do the different languages, as it'll start coming up a lot more in future APIs, and it'd be sad if we added something that works just for this stuff but not others.
I'm not seeing how we can expand i18n support in a generic way across the whole spec, particularly when the data structures we want to express are unique. Implementing a generic i18n structure could maybe work, but I fear we'll end up with 3 ways to do it, like with what happened to pagination. Instead, being i18n-aware in future APIs can solve the problem of awkward APIs.
OTHER THAN ALL THAT, yay this is looking good, and I'm happy we're wanting to move terms of service stuff into the registration and login flows \o/
🎉
I don't think soft logout was ever linked here, so here it is: https://github.com/matrix-org/matrix-doc/issues/1466
It's currently sitting in a state of needing implementation. I'm still comfortable with blocking this MSC on its inclusion in the spec, but that does mean that soft logout needs to be implemented first.
Right, I've updated this to close off the points raised by people. Please take another look, particularly at the last 3 commits:
- https://github.com/matrix-org/matrix-doc/pull/1692/commits/9c8100b539b5655cf8ef8b82677d76b799c8d6b7
- https://github.com/matrix-org/matrix-doc/pull/1692/commits/e963a947cf455151fff699b0bbaf826403267f43
- https://github.com/matrix-org/matrix-doc/pull/1692/commits/cfb2bc00a56391cb5bcb7ab93537daadde137641
My only query is why we both have the required/soft logout and withheld: true sync stuff? If ToS are required then they get logged out? So when would events get withheld?
Sounds like an artifact of me forgetting something. I don't think there's a reason for us to withhold events anymore because we can just soft log people out.
I've moved much of this proposal out to https://github.com/matrix-org/matrix-doc/pull/3012 due to those bits being unimplemented and not having much confidence in them being appropriate for use. This MSC is now just the registration bits which have had several years worth of implementation proof to them.
Considering this MSC has passed at least the implementation test:
@mscbot fcp merge
This FCP proposal has been cancelled by https://github.com/matrix-org/matrix-doc/pull/1692#issuecomment-838785488.
Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:
- [ ] @dbkr
- [ ] @uhoreg
- [x] @turt2live
- [ ] @ara4n
- [ ] @anoadragon453
- [ ] @richvdh
- [ ] @erikjohnston
- [ ] @KitsuneRal
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.
right, another 3 years later I've addressed the latest round of feedback. Let's try to get this into the spec, finally.
@mscbot fcp merge
Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:
- [x] @clokep
- [x] @dbkr
- [x] @uhoreg
- [x] @turt2live
- [ ] @ara4n
- [ ] @anoadragon453
- [x] @richvdh
- [x] @tulir
- [x] @erikjohnston
- [x] @KitsuneRal
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.
@turt2live is my understanding correct that Synapse already implements this (UGH)? And if so does the current MSC match the implementation?
@turt2live is my understanding correct that Synapse already implements this (UGH)? And if so does the current MSC match the implementation?
Yes and yes. If you've had the unfortunate opportunity to review the original MSC here, the bits which Synapse doesn't implement were already broken out to https://github.com/matrix-org/matrix-spec-proposals/pull/3012
