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

Corellate user sessions with access tokens #2146

Merged
merged 4 commits into from
Mar 16, 2020

Conversation

kschiffer
Copy link
Member

Summary

References #1422 #1844

This PR will store the session ID together with the access token. This way, we can correlate the session ID being able to:

  • Implement proper client-initiated logout, by terminating only the user session that was used to create the access token
  • Implement federated logout, by deleting all access tokens that were issued using this session ID and thus forcing clients to logout

Changes

  • Update access token and auth code protos
  • Update oauth logic to pass and store the session id to the auth code and access token record

Notes for Reviewers

This PR is a follow up of #2122 and the first of a series of changes leading to a more robust auth flow as well as the introduction of the new account app (#1422).

DB migration is necessary after this change ttn-lw-stack is-db migrate

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.

@kschiffer kschiffer added c/identity server This is related to the Identity Server compat/db This could affect Database compatibility labels Mar 13, 2020
@kschiffer kschiffer added this to the March 2020 milestone Mar 13, 2020
@kschiffer kschiffer self-assigned this Mar 13, 2020
Copy link
Contributor

@htdvisser htdvisser left a comment

Choose a reason for hiding this comment

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

Please add some checks to pkg/identityserver/oauth_registry_test.go to verify that the session ID is stored and retrieved correctly.

Please add some checks to pkg/oauth/server_test.go to verify that the session ID is properly stored in the authorization code, and propagated on authorization code exchange and token refresh.

Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Code owned files LGTM

@kschiffer kschiffer force-pushed the feature/token-session-correlation branch from a21e477 to 7fbb41c Compare March 16, 2020 07:11
@kschiffer kschiffer force-pushed the feature/token-session-correlation branch from 7fbb41c to 361ce27 Compare March 16, 2020 07:40
@kschiffer kschiffer removed the request for review from pgalic96 March 16, 2020 07:41
@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 73.345% when pulling 361ce27 on feature/token-session-correlation into d672874 on master.

@kschiffer kschiffer merged commit 5720313 into master Mar 16, 2020
@rvolosatovs rvolosatovs deleted the feature/token-session-correlation branch March 17, 2020 15:31
KrishnaIyer pushed a commit that referenced this pull request Mar 18, 2020
…correlation

Corellate user sessions with access tokens
@johanstokking johanstokking added the bump/minor Needs new minor version for release label Mar 19, 2020
rvolosatovs pushed a commit to rvolosatovs/lorawan-stack-fork that referenced this pull request Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump/minor Needs new minor version for release c/identity server This is related to the Identity Server compat/db This could affect Database compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants