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

MSC 1466 - Soft Logout #1467

Merged
merged 3 commits into from
Dec 10, 2018
Merged

MSC 1466 - Soft Logout #1467

merged 3 commits into from
Dec 10, 2018

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston changed the title Add 1466 proposal - Soft Logout MSC 1466 proposal - Soft Logout Aug 1, 2018
@erikjohnston erikjohnston changed the title MSC 1466 proposal - Soft Logout MSC 1466 - Soft Logout Aug 1, 2018
@ara4n
Copy link
Member

ara4n commented Aug 1, 2018

+1 to this existing. should we think of it as a “re-auth” API though rather than a soft logout? A logout which doesn’t delete your local state is horrid. But how about a “re-auth” API which clients should use to display a new auth popup over the top of the logged in user... and then vape the local storage if that auth fails?

@erikjohnston
Copy link
Member Author

erikjohnston commented Aug 1, 2018

+1 to this existing. should we think of it as a “re-auth” API though rather than a soft logout? A logout which doesn’t delete your local state is horrid. But how about a “re-auth” API which clients should use to display a new auth popup over the top of the logged in user... and then vape the local storage if that auth fails?

We can certainly change the name of the property, though its worth noting that one of the use cases for this is so that we log people out due to e.g. server hitting its resource limit, and so they would be unable to log back in again. Not sure if it would therefore be confusing to call it "re-auth" if they're actually properly being kicked off the server

@ara4n
Copy link
Member

ara4n commented Aug 1, 2018 via email

@erikjohnston
Copy link
Member Author

As described in the proposal this flag will allow clients to differentiate different the two cases and handle them differently in the UI, regardless of what we name the flag. Personally, I think "reauth" doesn't necessarily accurately describe what's potentially happening, but its a good a name as any I can think of.

@dbkr
Copy link
Member

dbkr commented Aug 1, 2018

Summarising from the Matrix chat, my plan on Riot was to make this the way it handles logout anyway. Personally I think if you wanted to request the client delete key material, that would probably be a separate 'remote wipe' thing.

@erikjohnston
Copy link
Member Author

Personally I think if you wanted to request the client delete key material, that would probably be a separate 'remote wipe' thing.

My hunch is to get it to work we'd end up having to have a flag on the 401 responses anyway, so I think we're in agreement (other than perhaps what to call the flag)

@dbkr
Copy link
Member

dbkr commented Aug 2, 2018

Yep, I suspect it would be a flag in the same place so it's basically just what the flag is called and the default if the flag isn't there.

@erikjohnston
Copy link
Member Author

Does anyone have any opinions on what the default should be?

@aaronraimist
Copy link
Contributor

aaronraimist commented Oct 29, 2018

For me ideally it would work something like the Google login/logout screen. When you log out it would show you both an option to log back in to the same account and an option to login to a new account by default on every log out.

@ara4n
Copy link
Member

ara4n commented Oct 29, 2018

I suggest the default should be soft_logout=false for compatibility with what we have today. I seem to be alone in wanting to call it re-auth, so ignore that.

@dbkr said:

Summarising from the Matrix chat, my plan on Riot was to make this the way it handles logout anyway. Personally I think if you wanted to request the client delete key material, that would probably be a separate 'remote wipe' thing.

I'm really not sure we should be leaving key data hanging around by default, but only if the server has explicitly said "please re-authenticate the user without logging them out" (i.e. a "soft logout").

@erikjohnston
Copy link
Member Author

I think the conclusion was to default to soft_logout=false if its not specified, so:

@mscbot fcp merge

If people have concerns with that then shout. I think we have enough alignment on the UX such that whatever we decide to do there it won't affect the API shape.

@mscbot
Copy link
Collaborator

mscbot commented Oct 29, 2018

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

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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 info 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 Oct 29, 2018
A new parameter is added to the JSON body of 401 responses, called
`soft_logout`. This is a boolean flag (defaulting to `false`) that signals to
the client whether it should keep local data and simply prompt to reauth (when
`true`) or to destroy the current data like it does today (when `false`).
Copy link
Member

Choose a reason for hiding this comment

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

Under what conditions would soft_logout be set to true? I'm guessing changing the user's password would be one situation? Are there others?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, or to allow us to time out old session, etc. Another case is when you want to force users to go through the login flows again, e.g. as an easy way to get a user to accept new ToCs.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. It would be nice to list some examples in the Motivation section. But other than this and the typo, lgtm.

@mscbot
Copy link
Collaborator

mscbot commented Nov 16, 2018

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

@mscbot mscbot added finished-final-comment-period and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Nov 16, 2018
@mscbot
Copy link
Collaborator

mscbot commented Nov 21, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants