-
Notifications
You must be signed in to change notification settings - Fork 66
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
11861 Sign in Service Client Config Implementation #11901
Conversation
154c41f
to
34aba7c
Compare
34aba7c
to
cc279c2
Compare
All features working as described, LGTM! |
cc279c2
to
bfffcb5
Compare
Generated by 🚫 Danger |
bfffcb5
to
23f7777
Compare
23f7777
to
454630e
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.
Looks good! Tested, and everything works as described. Just one question
@@ -61,8 +61,12 @@ def revoking_token | |||
@revoking_token ||= access_token || refresh_token | |||
end | |||
|
|||
def client_config | |||
@client_config ||= SignIn::ClientConfig.find_by(client_id: session.client_id) |
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.
Do we want client_config
here (and similarly in other classes) to raise an error if not found like in TokenSerializer
?
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.
Oops, I missed this comment for some reason. I think we do want to raise an error when we don't find the client config (if we have many clients it will be a 'much bigger deal' if the client id is unexpected or wrong somehow. For example if someone deletes a profile and expects sessions to stop working after that)
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.
I'll add a new PR to make that change
8999c46
454630e
to
8999c46
Compare
Summary
Related issue(s)
Testing done
What areas of the site does it impact?
Acceptance criteria
bundle exec rails db:seed
first, to make sure the proper default Client Config entries are created in your localhost databaseSignIn::OAuthSession
directly, since the refresh expiration is so long)/sign_in/v0/introspect
/authorize
route does not work ifclient_id
is some arbitrary valueSignIn::ClientConfig
(there is intentionally not a way to do this in code. So look atdb/seeds/development.rb
and create a new entry similar to how it's done there), and confirm the Sign in Service auth with this config works as expected, based on how it was set up