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

Split up oauth package into oauth and account (Account app scaffolding) #3325

Merged
merged 7 commits into from
Oct 26, 2020

Conversation

kschiffer
Copy link
Member

@kschiffer kschiffer commented Oct 6, 2020

Summary

This PR takes care of splitting up the oauth package into the oauth package (token exchange and authorization) to account app (authentication and account management), while retaining the config structure to stay conform to our compatibility commitment.

References #1422

Changes

  • Split up oauth app and account app
    • Extract authentication logic from oauth package to new account package
    • Rename oauth.js entrypoint to account.js
    • Rename oauth webui folder to account
    • Rename static assets
    • Update module import statements
    • Update various configs accordingly
      • package.json module name mapper for jest
      • webpack config for module aliases (@oauth to @account)
      • eslint alias config
      • Retain stack config structure
    • Rename github commit topic from oauth to account
  • Some related reducer fixes
  • Updating go tests accordingly

Testing

I did manual testing and updated the unit tests.

Regressions

This is a pretty extensive PR so there might be issues introduced to numerous aspects:

  • Authentication, login and logout
  • Token exchange and authorization
  • Broken redirects
  • Broken views

Notes for Reviewers

Here's a little recap of the plan since this is going on for a while now:

  • We want to introduce the "Account" app to be a dedicated app dealing with user authentication, profile settings, session management, client authorization management and other concerns revolving around the user account
  • The oauth package will remain in place but shrinked down to merely authorization concerns (oauth flow, token exchange, client authorization)
  • The Account app will not be an OAuth client (like the Console) but authorize API action via the session token. Session cookie authorization was added via Enable authorization via session cookie by kschiffer · Pull Request #2564 · TheThingsNetwork/lorawan-stack · GitHub
  • The frontend of the oauth server will be reduced to merely the authorization screen (to authorize OAuth clients)
  • Since we cannot break existing configuration, I decided to use the is.oauth.* configuration for both the oauth and account package.
    • This means that both web servers use the same /oauth base route by default
    • It is intended, however, to overwrite the default to use /account instead
    • On the frontend we'll also use the same entry point account.js for both frontends
    • We'll also use the same "branding" for both frontends, since we want the experience to unified through the "Account" app, meaning that the remaining authorization screen of the oauth server will be handled by the account.js endpoint as well.
  • This PR is a necessary scaffolding to be able to rebase the already done frontend work of the Account app onto a working basis

Note that the target branch is again feature/1422-account-app, so this is not targeting master/develop and is preliminary work for upcoming account app related PRs. The point of this is again to reduce review diffs, chunking things down into bits that are scoped and easy to review until eventually merging feature/1422-account-app into v3.10 when all parts have been merged into it.

It's necessary to update the config variables to mount this in development mode:

export export TTN_LW_IS_OAUTH_UI_JS_FILE="libs.bundle.js account.js"

Main reviewers: @bafonins @htdvisser
@adriansmares please double-check whether these changes introduce any issues regarding federated auth.

Checklist

  • Scope: The referenced issue is addressed, there are no unrelated changes.
  • Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md. The target branch is set to master if the changes are fully compatible with existing API, database, configuration and CLI.
  • 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/console This is related to the Console in progress We're working on it bump/minor Needs new minor version for release ui/web This is related to a web interface tooling Development tooling labels Oct 6, 2020
@kschiffer kschiffer added this to the October 2020 milestone Oct 6, 2020
@kschiffer kschiffer self-assigned this Oct 6, 2020
@kschiffer kschiffer force-pushed the feature/1422-account-app-split branch from 4be1798 to cce8047 Compare October 7, 2020 11:31
@github-actions github-actions bot added c/identity server This is related to the Identity Server compat/config This could affect Configuration compatibility labels Oct 7, 2020
@kschiffer kschiffer force-pushed the feature/1422-account-app-split branch from cce8047 to 4228efc Compare October 7, 2020 11:41
@kschiffer kschiffer marked this pull request as ready for review October 7, 2020 12:04
@kschiffer kschiffer removed the in progress We're working on it label Oct 7, 2020
@kschiffer kschiffer force-pushed the feature/1422-account-app-split branch from 4228efc to c5ce5b9 Compare October 7, 2020 12:54
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.

Did a quick initial review. Will take a closer look another time.

cmd/internal/shared/identityserver/config.go Outdated Show resolved Hide resolved
pkg/oauth/server.go Outdated Show resolved Hide resolved
pkg/oauth/server.go Show resolved Hide resolved
pkg/oauth/server.go Show resolved Hide resolved
pkg/account/user.go Show resolved Hide resolved
pkg/account/session/observability.go Outdated Show resolved Hide resolved
pkg/account/observability.go Outdated Show resolved Hide resolved
@htdvisser htdvisser self-requested a review October 7, 2020 13:21
@johanstokking
Copy link
Member

Here's a little recap of the plan since this is going on for a while now:
[...]

Totally agreed with this.

Copy link
Contributor

@adriansmares adriansmares left a comment

Choose a reason for hiding this comment

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

Took a look at the changes to pkg/oauth and I consider this to be compatible with the federated auth (which will probably migrate to pkg/account).

@htdvisser htdvisser removed their request for review October 12, 2020 12:28
@htdvisser
Copy link
Contributor

Nothing to add to my previous review

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.

Go stuff LGTM

@kschiffer kschiffer force-pushed the feature/1422-account-app-split branch from d53df45 to a094cdf Compare October 26, 2020 12:11
@kschiffer kschiffer merged commit 7862894 into feature/1422-account-app Oct 26, 2020
@kschiffer kschiffer deleted the feature/1422-account-app-split branch October 26, 2020 12:14
@kschiffer kschiffer mentioned this pull request Jan 20, 2021
5 tasks
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/console This is related to the Console c/identity server This is related to the Identity Server compat/config This could affect Configuration compatibility tooling Development tooling ui/web This is related to a web interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants