-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implements basic claims request parameter (Closes #11) #13
base: master
Are you sure you want to change the base?
Conversation
8627007
to
814b2f6
Compare
@willtcarey I have solved the conflict in this PR, sorry about that I missed it, could you please have a look? |
@willtcarey sorry for pinging you again, but this code runs on our production since 2 months without any issues, I really would love to see this PR merged 😊. Could you please have a look? |
Please @willtcarey could you have a look at my PR? |
@zedtux Not sure how this slipped under our radar for so long. We'll take a look ASAP. |
@nathancolgate or @willtcarey will you have a look soon please? 😇 |
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.
Oh hey, I had a pending review on this.
A few code cleanups for things we aren't using.
t.string :client_id, null: false | ||
t.string :nonce | ||
t.string :code, null: false | ||
t.text :scopes, null: false | ||
t.text :claims |
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.
This needs to happen in a new migration, which it does. So we should remove it here
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.
Completely agreed
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.
Oh, sorry, checking my code again and I see that here it will apply for new projects, while the new migration script db/migrate/20230721112432_add_claims_to_oidc_provider_authorization_if_missing.rb
will add that column for existing projects when the column doesn't exist already, so that it works in both cases.
We could keep it like this, but if you really want it I can remove it.
What about Rspec tests? Would you like them? In a separated PR maybe? |
def call(account) | ||
OpenIDConnect::ResponseObject::UserInfo.new( | ||
sub: account.send(OIDCProvider.account_identifier) | ||
).tap do |user_info| | ||
scopes.each do |scope| | ||
UserInfoBuilder.new(user_info, account).run(&scope.work) | ||
ResponseObjectBuilder.new(user_info, account).run(&scope.work) | ||
end | ||
end | ||
end |
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.
Does this make it so that we can add scopes for arbitrary data that doesn't already exist on the UserInfo
object?
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.
That's a weakness we've identified where we sometimes need to allow for scopes that have data that exists outsides of what's available on the UserInfo object
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.
Yes that's the goal of this ResponseObjectBuilder
thanks to its method_missing
method usage.
This PR closes #11 by implementing the basics of the OpenID claims request parameter allowing a consumer to request more claims in the
id_token
JWT by passing a JSON in theclaims
parameter in the Authorization creation request.The requested claim(s) must be part of the requested scopes so that requested claims belonging to non-requested scopes are logged as a warning and ignored.
This PR implements this parameter for both
id_token
anduserinfo
but only theid_token
has been tested.This PR doesn't support the
{ essential: true }
but supports hard coded value ornil
.Here is one of my Rspec test showcasing what this PR does: