Client initiated logout / logout propagation to oauth server #2148

kschiffer · 2020-03-13T08:03:53Z

Summary

References #1422
Closes #1702

This PR will enable logout propagation from our official clients, meaning that the console logout will also result in a deletion of the session in the oauth server that was used to create the access token.

Changes

Add a LogoutRedirectURI field to the clients, used to check the validity of the requested redirect during the client-initiated logout
Update mage targets to set logout redirect URIs
Add a LogoutURL config to oauthclient util, used to trigger a logout of the oauth provider
Refactor oauthclient and oauth server for the new flow

Notes for Reviewers

In order to test the setup in dev environment, it is necessary to set two new settings accordingly:

export TTN_LW_CONSOLE_OAUTH_LOGOUT_URL="http://localhost:8080/oauth/logout"

Also the console client needs to be reinitiated: mage dev:dbStop dev:dbErase dev:dbStart dev:initStack

The flow works like this:

click to enlarge

The user clicks on a logout button inside the clients web ui
The web app will delete the access token
The client backend will destroy the auth cookie and return an op_logout_uri, containing the access token ID, as well as a desired post_logout_redirect_uri
(Based on whether logout propagation is desired, ) the web app will redirect the browser to the logout URI
The oauth server will retrieve the associated session from the passed access_token_id query parameter, delete the associated session of the access token and the access token itself
The oauth server will check whether the post_logout_redirect_uri has been previously registered and redirect back to this URI (if no post_logout_redirect_uri was passed, the first LogoutRedirectURI of the client will be used
The client can use the redirect URI to show a dedicated post logout view (but the console will simply redirect back to the root path)
(Since the access token was deleted, the console will redirect to the login screen, from which the auth flow can be started again)

I've kept the frontend changes in this PR to a minimum, so we can focus on the backend changes. I will follow up with a PR that does a proper refactor of the webui after this PR has been merged. This will include removing the login view, refactoring withAuth util component, removing unnecessary store logic and removing the landing view of the oauth app.

As discussed with @htdvisser, we also plan to to federated logout (meaning logging out all currently authenticated SSO clients associated with the respective oauth session).
This would necessitate adding an sso flag to the client model, which will be used to determine what other clients need to be logged out. Should I also integrate this into this PR or use another scoped one?

Checklist

Scope: The referenced issue is addressed, there are no unrelated changes.
Compatibility: The changes are backwards compatible with existing API, database and configuration, according to the stability commitments in README.md.
Testing: The changes are covered with unit tests. The changes are tested manually as well.
Documentation: Relevant documentation is added or updated.
Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

htdvisser · 2020-03-17T10:54:23Z

pkg/oauth/server.go

@@ -79,8 +79,9 @@ type FrontendConfig struct {

 // Config is the configuration for the OAuth server.
 type Config struct {
-	Mount string   `name:"mount" description:"Path on the server where the OAuth server will be served"`
-	UI    UIConfig `name:"ui"`
+	DefaultRedirectURI string   `name:"default-redirect-uri" description:"The default URI the OAuth server will redirect to after login succeeded"`


Can we leave the "default redirect URI" feature for another PR?

I think then it's best to disable standalone (not client-initiated) logins for the time being. Any other idea?

I'd prefer to keep this PR scoped to one thing, and do other stuff later.

Ok, I've reintroduced the old login handler which delets the active session, so we can show the current authenticated view of the oauth server again. Also updated the tests accordingly. We will have two logout handlers then in the interim.

pkg/oauth/server_test.go

pkg/oauth/user.go

pkg/oauth/server.go

pkg/oauth/user.go

pkg/web/oauthclient/logout.go

coveralls · 2020-03-19T10:11:18Z

Coverage decreased (-0.2%) to 73.143% when pulling c3ed357 on feature/1702-client-initiated-logout into 52e258b on master.

kschiffer · 2020-04-01T13:17:53Z

Blocking #1422

kschiffer · 2020-04-15T09:36:16Z

One thing I noticed:

Log in

While in the console, clear cache/local and session storage/cookies

Initiate an api request

Be redirected to the login

Login again

Try logging out

Get 403 forbidden for the /logout call

This was actually a bug related to the echo csrf middleware we're using, which spammed the browser with token cookies for all the routes that it was called on and hence mixing up tokens during lookup. This is now fixed.

htdvisser

Please also update doc/content/guides/getting-started/running-the-stack.md with the logout redirect URIs

htdvisser · 2020-04-15T09:59:12Z

CHANGELOG.md

@@ -22,6 +22,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `ns.down.data.schedule.attempt` and `ns.down.join.schedule.attempt` events, which are triggered when Network Server attempts to schedule a respective downlink on Gateway Server.
 - `ns.down.data.schedule.success` and `ns.down.join.schedule.success` events, which are triggered when Network Server successfully schedules a respective downlink on Gateway Server.
 - `ns.down.data.schedule.fail` and `ns.down.join.schedule.fail` events, which are triggered when Network Server fails to schedule a respective downlink on Gateway Server.
+- Console logout is now propagated to the OAuth provider.
+  - This requires a database migration (`ttn-lw-stack is-db migrate`) because of the added columns.
+  - To set the `logout-redirect-uris` for existing clients, the CLI client can be used, e.g.: `ttn-lw-cli clients update console --redirect-uris "https://localhost:1885/console" --redirect-uris "http://localhost:1885/console"`.


This is not a realistic example. Realistic redirect URIs end with /console/oauth/callback and you're not giving any example for --logout-redirect-uris.

Indeed wrong example. This should just be logout-redirect-uris instead of redirect-uris.

htdvisser · 2020-04-15T10:01:24Z

pkg/oauth/server.go

-	api.POST("/auth/logout", s.Logout, s.requireLogin)
+	api.POST("/auth/logout", s.Logout)


Won't this cause trouble in s.Logout for users that are not logged in?

Indeed. This is a leftover from before #2148 (comment)

htdvisser · 2020-04-15T10:04:08Z

pkg/oauth/user.go

+	if err != nil {
+		if !errors.IsNotFound(err) {


This can be combined to if err != nil && !errors.IsNotFound(err) {

htdvisser · 2020-04-15T10:06:22Z

pkg/oauth/user.go

+		if err = s.store.DeleteAccessToken(ctx, accessTokenID); err != nil {
+			return err
+		}
+		s.removeAuthCookie(c)


We don't know that the auth cookie corresponds with the session we're deleting. If it doesn't, we may leave a session active while the user thinks it's revoked.

I can't think of any realistic scenario where that would be the case. The user has obtained the access token using this auth cookie (session). Since our official clients tie them together, there should be no corruptions or mixups. So I think this is neglectible. In any case, we still have the old logout handler, so clients can decide for themselves.

As discussed, the logout will now

delete the access token

delete the session that was associated with the access token

delete the session associated with the current auth cookie of the user (if it differs from the sessions stored in the access token)

htdvisser

Go part LGTM with small comment.

htdvisser · 2020-04-17T09:47:36Z

pkg/oauth/user.go

+		if err = s.store.DeleteSession(ctx, &at.UserIDs, at.UserSessionID); err != nil {
+			if !errors.IsNotFound(err) {


Can combine err != nil && !errors.IsNotFound(err)

kschiffer added c/console This is related to the Console c/identity server This is related to the Identity Server needs/discussion We need to discuss this security This is important for security labels Mar 13, 2020

kschiffer requested a review from htdvisser March 13, 2020 08:03

kschiffer self-assigned this Mar 13, 2020

kschiffer force-pushed the feature/token-session-correlation branch 2 times, most recently from 7fbb41c to 361ce27 Compare March 16, 2020 07:40

kschiffer force-pushed the feature/1702-client-initiated-logout branch from 7116fa2 to 399f11f Compare March 16, 2020 11:27

kschiffer changed the base branch from feature/token-session-correlation to master March 16, 2020 11:28

kschiffer marked this pull request as ready for review March 17, 2020 04:54

kschiffer requested review from bafonins, johanstokking, pgalic96 and rvolosatovs as code owners March 17, 2020 04:54

htdvisser reviewed Mar 17, 2020

View reviewed changes

johanstokking removed their request for review March 17, 2020 12:06

kschiffer force-pushed the feature/1702-client-initiated-logout branch from 399f11f to 757a353 Compare March 18, 2020 08:44

kschiffer requested a review from htdvisser March 18, 2020 08:44

rvolosatovs removed their request for review March 18, 2020 09:34

kschiffer force-pushed the feature/1702-client-initiated-logout branch from 05843c1 to 6459389 Compare March 19, 2020 04:20

htdvisser mentioned this pull request Mar 19, 2020

Fix non-deterministic test in Identity Server #2191

Merged

kschiffer force-pushed the feature/1702-client-initiated-logout branch from 84b20c4 to 3891ba3 Compare March 19, 2020 09:55

johanstokking added this to the March 2020 milestone Mar 19, 2020

This was referenced Mar 24, 2020

Logging out does not work #1844

Closed

Introduce new account app (to replace oauth app) #1422

Closed

kschiffer added blocking Another issue or pull request is waiting for this and removed needs/discussion We need to discuss this labels Apr 1, 2020

kschiffer force-pushed the feature/1702-client-initiated-logout branch from 146af81 to 78da59d Compare April 15, 2020 09:33

kschiffer requested a review from bafonins April 15, 2020 09:42

htdvisser reviewed Apr 15, 2020

View reviewed changes

kschiffer force-pushed the feature/1702-client-initiated-logout branch from 78da59d to fa8339d Compare April 15, 2020 11:14

kschiffer requested a review from htdvisser April 15, 2020 11:14

kschiffer force-pushed the feature/1702-client-initiated-logout branch 3 times, most recently from 3f91ee9 to c3ed357 Compare April 17, 2020 06:47

htdvisser approved these changes Apr 17, 2020

View reviewed changes

kschiffer added 14 commits April 17, 2020 22:12

is: Add logout redirect URI field to clients

bcbb79b

dev: Add redirect uris to console in init mage target

f09d9b6

util: Add logout url config to oauthclient util

5161b95

dev: Update protos

b569e16

oauth: Refactor logout logic for client initiated logout

e09a034

util: Refactor oauthclient logout for new auth flow

1bb73ac

console: Update webui init and user logic for new auth flow

748afa6

console: Fix csrf management

8762f3f

console: Catch auth errors in frontend

1893b34

oauth: Update tests

7b487e7

dev: Update messages

35200b3

util: Update js sdk api definitions

9ccd837

doc: Update "Running the stack" guide

6a58819

dev: Update changelog

4fe6c89

kschiffer force-pushed the feature/1702-client-initiated-logout branch from c3ed357 to 4fe6c89 Compare April 17, 2020 13:13

kschiffer merged commit f959897 into master Apr 20, 2020

kschiffer mentioned this pull request Apr 22, 2020

Add missing documentation of oauth logout url config #2388

Merged

6 tasks

kschiffer mentioned this pull request May 13, 2020

Remove faulty login redirects in OAuth app #2526

Merged

6 tasks

johanstokking deleted the feature/1702-client-initiated-logout branch May 16, 2020 08:04

kschiffer mentioned this pull request May 18, 2020

Fix oauth CSRF cookie not using mount path #2560

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client initiated logout / logout propagation to oauth server #2148

Client initiated logout / logout propagation to oauth server #2148

kschiffer commented Mar 13, 2020 •

edited

htdvisser Mar 17, 2020

kschiffer Mar 18, 2020

htdvisser Mar 18, 2020

kschiffer Mar 19, 2020

coveralls commented Mar 19, 2020 •

edited

kschiffer commented Apr 1, 2020

kschiffer commented Apr 15, 2020

htdvisser left a comment

htdvisser Apr 15, 2020

kschiffer Apr 15, 2020

htdvisser Apr 15, 2020

kschiffer Apr 15, 2020

htdvisser Apr 15, 2020

htdvisser Apr 15, 2020

kschiffer Apr 15, 2020 •

edited

kschiffer Apr 17, 2020

htdvisser left a comment

htdvisser Apr 17, 2020

		api.POST("/auth/logout", s.Logout, s.requireLogin)
		api.POST("/auth/logout", s.Logout)

		if err = s.store.DeleteSession(ctx, &at.UserIDs, at.UserSessionID); err != nil {
		if !errors.IsNotFound(err) {

Client initiated logout / logout propagation to oauth server #2148

Client initiated logout / logout propagation to oauth server #2148

Conversation

kschiffer commented Mar 13, 2020 • edited

Summary

Changes

Notes for Reviewers

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 19, 2020 • edited

kschiffer commented Apr 1, 2020

kschiffer commented Apr 15, 2020

htdvisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer Apr 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htdvisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschiffer commented Mar 13, 2020 •

edited

coveralls commented Mar 19, 2020 •

edited

kschiffer Apr 15, 2020 •

edited