MSC1692: Terms of service at registration

[turt2live]

Rendered

---

Implementation: matrix-org/synapse#4004

FCP tickyboxes

---

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.

[turt2live]

Also I just found #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.

[erikjohnston]

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.

[Half-Shot]

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.

[turt2live]

@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.

[turt2live]

@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.

[Half-Shot]

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)

[turt2live]

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.

[erikjohnston]

@turt2live is my understanding correct that Synapse already implements this (UGH)? And if so does the current MSC match the implementation?

[turt2live]

@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 #3012

[mscbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[mscbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[richvdh]

an empty auth dict

This can't be right. I'm assuming it means an auth dict which looks like:

  "auth": {
      "type": "m.login.terms",
      "session": "xxxxxx"
  }

(ie, nothing other than the type and session).

[turt2live]

err, yes. UIA is complicated, and I can believe this part of the MSC was written with confusion.

It'd be the same as dummy auth: https://spec.matrix.org/v1.10/client-server-api/#dummy-auth

[richvdh]

Spec PR: matrix-org/matrix-spec#1812

[turt2live]

Merged 🎉