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

MSC3741: Revealing the useful login flows to clients after a soft logout #3741

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Feb 23, 2022

@turt2live turt2live marked this pull request as ready for review February 23, 2022 06:59
@turt2live turt2live added client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal proposal-in-review social login work related to improving login via external identity providers labels Feb 23, 2022
@turt2live turt2live changed the title Revealing the useful login flows to clients after a soft logout MSC3741: Revealing the useful login flows to clients after a soft logout Feb 23, 2022
## Alternatives

No viable alternatives known.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are those login flows actually specific to the access token? I thought they were specific to the way the user registered. As such the client could also use UIA on /login to get the flows supported for the user. This would solve getting the supported login method for the initial login as well. Currently UIA is not supported on /login, but it would solve multiple issues if it were including this one.

UIA could be as simple as a /login request with only a userid and the server returns the usual UIA response instead of this being an invalid request. Those flows should only include supported flows for that account of course. Then the client just does UIA as normal.

Copy link
Member Author

Choose a reason for hiding this comment

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

Flows are specific to the user, but the token identifies the user.

Other MSCs can look at revamping the auth system more generally.

Comment on lines +23 to +29
Quite simply, the client provides the now-expired access token to the
[`/login`](https://spec.matrix.org/v1.2/client-server-api/#login) endpoint - just as it would any
other request.

Currently servers (should) be checking to see if the provided access token is an appservice user,
for [appservice login](https://spec.matrix.org/v1.2/client-server-api/#appservice-login), however
under this new approach the server would need to add a check for past users too.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried about this from the point-of-view of the server-side implementation. Basically it's adding yet another knob to the already-baroque login flow - in particular it needs to deal with a bunch of different login mechanisms, as well as tokens which expire due to soft-logout and tokens which have refresh tokens. Login really isn't a place where you want lots of conditionals and configuration-dependent flows.

More generally, I'm reluctant to spend more time tweaking the existing auth system instead of moving to an OAuth2 world.

Can we not solve the problem by replacing soft-logout with refresh tokens (MSC2918), which is something we need to do anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Refresh tokens cause soft logout, which is the problem at hand. When the token expires the user gets soft logged out, which makes the soft logout flow a lot more common, particularly on servers like matrix.org where it's not as tightly configured to handle an organization-only SSO (with no password auth) as we've seen with traditional usage of soft logout.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though, yes, ideally a replacement system fixes this entirely. My understanding is that we're years away from that though.

Copy link
Member

Choose a reason for hiding this comment

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

Refresh tokens cause soft logout, which is the problem at hand

yeahhh, but MSC2918 also specifies a mechanism for renewing access other than sending another /login request.

So what I'm really saying is: rather than specifying ways to extend /login, why not use /refresh ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not following. /refresh won't work if the token is already expired, so the client needs a new one. At that point it's not a refresh but rather replacement.

Also, not keen to add login flows to another endpoint if we want to remove that system.

Copy link
Member

Choose a reason for hiding this comment

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

/refresh won't work if the token is already expired

that's not my understanding.

At that point it's not a refresh but rather replacement.

/refresh issues you a new access token.

Copy link
Member

Choose a reason for hiding this comment

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

Also, not keen to add login flows to another endpoint if we want to remove that system.

not following this. What do you mean by "add login flows to another endpoint"? and which system are we removing?

Copy link
Member Author

Choose a reason for hiding this comment

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

while implementing refresh tokens I got "refresh token expired" far too often, so either the server-side implementation is wrong or we need to fix our reauthentication flows.

If the intent is that refresh tokens never expire, I can certainly take a look into the underlying cause (making this MSC obsolete), though if the intent is that refresh tokens expire then we need some way of appropriately re-authenticating the user. At the moment, this is done with a /login request using the same device ID - if we want /refresh to handle re-authentication too, it'll need to understand login flows which feels nonsensical.

Copy link
Member

Choose a reason for hiding this comment

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

refresh tokens should never expire (unless you configure a refresh_token_lifetime).

I'm now realising that maybe I don't understand your usecase, though. Why are you using soft-logout in the first place?

We should probably take this elsewhere to hammer out.


## Proposal

Quite simply, the client provides the now-expired access token to the
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
Quite simply, the client provides the now-expired access token to the
Quite simply, under this proposal the client should provide the now-expired access token to the

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal social login work related to improving login via external identity providers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants