-
Notifications
You must be signed in to change notification settings - Fork 299
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
Split up oauth package into oauth and account (Account app scaffolding) #3325
Conversation
4be1798
to
cce8047
Compare
cce8047
to
4228efc
Compare
4228efc
to
c5ce5b9
Compare
There was a problem hiding this 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.
Totally agreed with this. |
There was a problem hiding this 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
).
Nothing to add to my previous review |
c5ce5b9
to
d53df45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go stuff LGTM
d53df45
to
a094cdf
Compare
d452e1d
to
918133c
Compare
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
oauth
package to newaccount
packageoauth.js
entrypoint toaccount.js
oauth
webui folder toaccount
package.json
module name mapper for jest@oauth
to@account
)oauth
toaccount
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:
Notes for Reviewers
Here's a little recap of the plan since this is going on for a while now:
oauth
package will remain in place but shrinked down to merely authorization concerns (oauth flow, token exchange, client authorization)oauth
server will be reduced to merely the authorization screen (to authorize OAuth clients)is.oauth.*
configuration for both theoauth
andaccount
package./oauth
base route by default/account
insteadaccount.js
for both frontendsoauth
server will be handled by theaccount.js
endpoint as well.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 mergingfeature/1422-account-app
intov3.10
when all parts have been merged into it.It's necessary to update the config variables to mount this in development mode:
Main reviewers: @bafonins @htdvisser
@adriansmares please double-check whether these changes introduce any issues regarding federated auth.
Checklist
README.md
. The target branch is set tomaster
if the changes are fully compatible with existing API, database, configuration and CLI.CHANGELOG.md
.CONTRIBUTING.md
, there are no fixup commits left.