Client initiated logout and new client auth flow #2122

kschiffer · 2020-03-11T06:28:46Z

Summary

References #1422, #1086
Follow up of #2012

This PR will add client initiated logout to the oauth server. It will enable logout propagation from the webui clients to the oauth provider, basically tying the authentication of our official clients (console, account app) to the authentication of the auth provider.

Changes

Add logout_redirect_uris to client schema and update dev:init mage target to set this value for the console
Add default redirect URI config to the auth server (defaults to console for now)
Refactor oauthclient util for the new auth flow
Refactor oauth server logout endpoint and routing for the new auth flow
Remove unnecessary authenticated views from the oauth app
Remove login view from console and automatically redirect to oauth login instead
Remove landing view, since there are now no unauthenticated views for the console anymore
Add a utility to create frontend defined error objects
Refactor withAuth util component to also handle insufficient rights issues
Refactor init logic in the store; moving insufficient rights check to the withAuth utility
Improve styling and logic of error view
Minor related styling fixes

Notes for Reviewers

The implementation is inspired by OpenID connect's client initiated logout functionality.

We will use the OAuth server frontend now solely to perform authentication and remove the (very limited) authenticated view that we had before. All necessary user management will be done by the account app client, using an access token.
The oauth server exposes a logout endpoint that takes an access token and optional post logout redirect URI as argument. The oauth provider will then terminate all sessions associated with the user that the access token belongs to. Later on, we might want to associate the session that was used to generate the access token with it and terminate only this session.
- CSRF protection on the oauth logout endpoint is not necessary since the endpoint expects the access token to be present
- The post_logout_redirect_uri needs to be previously registered in the client (similar to the login redirect URI) and defines, where the logout endpoint will redirect the user after successful logout. Right now, it will just redirect back to the console root, but it could be used for specific post logout views
- In case of standalone (no redirect) logins into the oauth server, we will define a default redirect (currently the console, but the idea is to use the account app, once it is merged)
  - Alternatively, we could also disable logins that are not client initiated
Logging in and out of the oauth provider will be intiated via our official clients (console, account app))
- Console and account app will automatically initiate the auth flow when there is no access token present, or the access token is invalid, without any intermediate client login view
- To log out, the clients will clear their current access token from the local storage and query their (CSRF protected) logout endpoint, which will remove the auth cookie and return the defined logout endpoint of the oauth server using the access token and redirect URI. The client can then redirect to this URI to also terminate the session within the oauth server (which is what we will do by default)

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

export TTN_LW_IS_OAUTH_DEFAULT_REDIRECT_URI="http://localhost:8080/console"
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

Very sorry for the text-wall but the auth specifics are quite intricate, so I wanted to provide a clear summary.

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

Note that this doesn't address the issue of being logged in with multiple OAuth clients. If I'm logged in to the consoles of different clusters and logout from one console, the other one is still logged in, which may be confusing.

htdvisser · 2020-03-11T10:32:55Z

api/client.proto

+  // The allowed logout redirect URIs against which client initiated logout
+  // requests are checked. If the authorization request does not pass a redirect
+  // URI, the first one from this list is taken.
+  repeated string logout_redirect_uris = 10 [(gogoproto.customname) = "LogoutRedirectURIs"];


Changing the numbers of existing fields breaks API. Use a unused number for new fields.

htdvisser · 2020-03-11T10:34:02Z

cmd/ttn-lw-stack/commands/is_db_create_oauth_client.go

@@ -93,6 +97,7 @@ var (
 				"name",
 				"secret",
 				"redirect_uris",
+				"logout-redirect-uris",


Field masks use underscores

htdvisser · 2020-03-11T10:55:29Z

pkg/oauth/user.go

-	return c.NoContent(http.StatusNoContent)
+	events.Publish(evtUserLogout(ctx, at.UserIDs, nil))
+	for _, session := range sessions {
+		if err = s.store.DeleteSession(ctx, &at.UserIDs, session.SessionID); err != nil {


I don't think this is what a user wants. If I logout using my phone, I don't want to also be logged out from my laptop.

Right, I went with the easy way for now. To be able to do this, we'd need to know the session that was used when obtaining the access token. To do this, we could either:

pass the session id to the console together with the access token

store the session id in the access token store when creating it

htdvisser · 2020-03-11T10:58:35Z

Re-requesting myself for a more in-depth review later.

johanstokking

Reviewed code owned files only, nothing to add to @htdvisser 's comment

bafonins · 2020-03-12T14:41:07Z

pkg/webui/console/store/reducers/user.js

@@ -51,11 +52,10 @@ const user = function(state = defaultState, { type, payload }) {
        ...state,
        rights: payload,
      }
-    case LOGOUT_SUCCESS:
+    case LOGOUT_FAILURE:


With this change, the user won't be logged out from the console automatically on an unauthenticated error. The are still places where logoutSuccess is dispatched, see L78 and L95

bafonins · 2020-03-12T14:48:17Z

pkg/webui/console/store/selectors/user.js

  const { ids = {} } = user

-  return ids.user_id
+  return user ? ids.user_id : undefined


not sure what you are trying to do here if user is undefined you'll get an error anyway.

const user = undefined >> undefined const { ids={} } = user >> TypeError: obj is undefined

Additionally, I am not sure if should even allow the user object to be undefined. If it is needed, then i think such implementation makes more sense:

const user = selectUser(state) || {} const { ids = {} } = user return ids.user_id

bafonins · 2020-03-12T14:49:49Z

pkg/webui/console/store/selectors/user.js

 }

 export const selectUserIsAdmin = function(state) {
  const user = selectUser(state)
-  return user.isAdmin
+
+  return user ? user.isAdmin : undefined


return user ? user.isAdmin : false

bafonins · 2020-03-12T16:01:12Z

pkg/webui/lib/components/with-auth.js

+      // Check whether the user has at least basic rights, without which it
+      // makes no sense to access the console
+      Boolean(user) &&
+      !isAdmin &&


the admin account can be suspended/rejected as well. the only allowed operation for a suspended admin account is to list and delete other accounts. Do we want to allow that?

Also, while checking this I also found out that you are not logged out of the console when you delete your self. Most probably this is related to https://github.com/TheThingsNetwork/lorawan-stack/pull/2122/files#r391668222

Also, see #2144

johanstokking · 2020-05-16T09:29:24Z

What is the status here?

kschiffer · 2020-05-17T04:30:32Z

Was superseded by #2146. I'll delete the branch.

…ge/open-source-master Merge open source

kschiffer added 19 commits March 11, 2020 14:15

is: Add logout redirect URI to clients

1f86765

dev: Update protos

6c59c75

dev: Add logout URIs to init mage target

05c1a64

oauth: Refactor logout to enable client initiated sign out

26f6e1c

util: Refactor oauthclient logout to provide OP logout URL

c5a6f51

console,oauth: Fix dropdown clickable area

8255cca

console,oauth: Fix error view styling

ec39867

console,oauth: Add frontend error creator util

2910745

console,oauth: Improve error util comments

45f2f0b

console: Update logout store logic

83711a8

console: Update user selector

3050c9e

console: Improve error view logic

398b9c4

console: Remove rights check from init store logic

fab0e2d

oauth: Remove oauth landing view

2c64dac

console: Move landing view logic to app view

40e399b

console,oauth: Update shared messages

e19c28a

dev: Update locales

53c204d

console: Update withAuth component to check for new auth flow

cd126cf

console: Remove login view

331772a

kschiffer added c/console This is related to the Console c/identity server This is related to the Identity Server ui/web This is related to a web interface labels Mar 11, 2020

kschiffer added this to the March 2020 milestone Mar 11, 2020

kschiffer requested review from htdvisser and bafonins March 11, 2020 06:28

kschiffer requested review from johanstokking, pgalic96 and rvolosatovs as code owners March 11, 2020 06:28

kschiffer self-assigned this Mar 11, 2020

htdvisser reviewed Mar 11, 2020

View reviewed changes

htdvisser self-requested a review March 11, 2020 10:58

johanstokking reviewed Mar 11, 2020

View reviewed changes

kschiffer closed this Mar 12, 2020

bafonins reviewed Mar 12, 2020

View reviewed changes

kschiffer mentioned this pull request Mar 13, 2020

Corellate user sessions with access tokens #2146

Merged

6 tasks

kschiffer deleted the feature/1422-global-logout branch May 17, 2020 04:30

rvolosatovs pushed a commit to rvolosatovs/lorawan-stack-fork that referenced this pull request Sep 15, 2020

Merge pull request TheThingsNetwork#2122 from TheThingsIndustries/mer…

7484a91

…ge/open-source-master Merge open source

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client initiated logout and new client auth flow #2122

Client initiated logout and new client auth flow #2122

kschiffer commented Mar 11, 2020

htdvisser left a comment

htdvisser Mar 11, 2020

htdvisser Mar 11, 2020

htdvisser Mar 11, 2020

kschiffer Mar 11, 2020

htdvisser commented Mar 11, 2020

johanstokking left a comment

bafonins Mar 12, 2020

bafonins Mar 12, 2020

bafonins Mar 12, 2020

bafonins Mar 12, 2020

johanstokking commented May 16, 2020

kschiffer commented May 17, 2020

Client initiated logout and new client auth flow #2122

Client initiated logout and new client auth flow #2122

Conversation

kschiffer commented Mar 11, 2020

Summary

Changes

Notes for Reviewers

Checklist

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

htdvisser commented Mar 11, 2020

johanstokking 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

johanstokking commented May 16, 2020

kschiffer commented May 17, 2020