Defer to Doorkeeper when scopes are missing #79
Conversation
- the line would fail because no client was available - No client was available because no client was included in the params - Instead of a 500 error occurring for a request that was malformed, a 4xx error should be returned instead - This fix ensures that such an error is returned Co-authored-by: Boo Chalhoub <ichalhoub@mavenlink.com>
Co-authored-by: Boo Chalhoub <ichalhoub@mavenlink.com> Signed-off-by: Boo Chalhoub <ichalhoub@mavenlink.com> Co-authored-by: Boo Chalhoub <ichalhoub@mavenlink.com>
- Instead of responding with a 4xx error, deferring to the implemented behavior within doorkeeper better fits with our use case Co-authored-by: Kevin Lai <kevin@mavenlink.com>
Co-authored-by: Kevin Lai <kevin@mavenlink.com>
…re-auth Defer to doorkeeper on invalid pre auth
|
|
||
| def scopes | ||
| pre_auth.scopes | ||
| rescue NoMethodError |
There was a problem hiding this comment.
To be clear - this PR is obviously meant to raise questions and NOT to be merged - we're curious if we need to guarantee the validity of pre_auth after resource_owner_authenticator is run. Are we supposed to verify that pre_auth is valid with a valid client before we get to this point?
If not, calling scopes can attempt to build_scopes in the PreAuthorization with a nil client.
There was a problem hiding this comment.
we're curious if we need to guarantee the validity of pre_auth after resource_owner_authenticator is run
Hmm not sure what you mean, can you rephrase?
There was a problem hiding this comment.
Thanks for looking at this @toupeira !
I might be missing something about this, so correct me if I'm wrong with my assumptions:
Doorkeeper::OpenIDConnect::Helpers::Controlleroverrides base Doorkeeper'sauthenticate_resource_owner!, calls it (withsuper), opens the resource owner returned, and runs OIDC specific behavior (promptanalysis +max_agecheck)- In order to do so, it currently requires that
pre_auth.clientis valid. Every other part of the PreAuthorization can be missing -redirect_uri,scopes, andresponse_typecan beniland it'd still work fine because onlyscopesis used in this context andscopesin PreAuthorization does aclientlookup if it's missing. - Doorkeeper's
authenticate_resource_owner!is calling doorkeeper configresource_owner_authenticator
Because of that:
- Would our
resource_owner_authenticatorneed to validatepre_auth.client?so that by the time we drop intoDoorkeeper::OpenIDConnect::Helpers::Controller:authenticate_resource_owner!we guarantee thatpre_auth.scopesdoesn't raise an exception trying to grabclientifscopesdoesn't exist.
I noticed that #80 puts it back in, so I think that answers the question.
That being said, I'm wondering if it's worth checking pre_auth.valid? rather than specifically pre_auth.client.
Would it make sense to ensure that response_type, client, redirect_uri are valid before we take action (they seem required in the OIDC RFC?) or is it something you want to leave to the base Doorkeeper gem itself when we finish and it drops down to the authorized_applications_controller or authorizations_controller?
Let me know if it's a concern I should put on that PR. Thanks!
There was a problem hiding this comment.
@cincospenguinos @urnf @isabellechalhoub thanks for raising this and submitting a PR, and please accept my apologies for the delay! Had to find some time to take a proper look :)
So I think what I'll do is simply add back this check for pre_auth.client which I removed in 3893ea1:
- next unless pre_auth.client && pre_auth.scopes.include?('openid')
+ next unless pre_auth.scopes.include?('openid')To be honest I'm not exactly sure why I removed it, but it should be safe to add back in and should fix the 500 error.
Your other changes are sensible as well, I submitted #80 with a slightly different approach and added some more specs. Will close this in favour of that one!
|
|
||
| def scopes | ||
| pre_auth.scopes | ||
| rescue NoMethodError |
There was a problem hiding this comment.
we're curious if we need to guarantee the validity of pre_auth after resource_owner_authenticator is run
Hmm not sure what you mean, can you rephrase?
As we explored our use case, we realized that we did not want to submit a 4xx error on missing parameters for openid connect within our Oauth flow. We refactored a bit and created a test case that proved that, if parameters were missing, doorkeeper-openid_connect would pass on its opportunity to interact with the flow and doorkeeper would take over.
The biggest change to pay attention to is the extraction of
pre_auth.scopesout to a separate private method. This method captures aNoMethodErrorthat gets thrown when pre_auth attempts to retrieve the scopes of an object that has no client, and therefore no application. This refactor is really a workaround--lacking the context concerning this flow, I decided that simply capturing the error and navigating around it would be the best way to go.If any of you have any suggestions or desires to see this area move in a different direction, let me know and my team and I will adjust our PR to reflect that.
Solves #78
This appears to be related to #71.