simplesamlphp / simplesamlphp-module-oidc

A SimpleSAMLphp module for OIDC OP support.
Other
45 stars 22 forks source link

Ignore unrecognized scopes (3.1.2.1 of spec) #187

Closed pradtke closed 1 year ago

pradtke commented 1 year ago

"Scope values used that are not understood by an implementation SHOULD be ignored" (per spec)

Previously we were returning an error.

codecov[bot] commented 1 year ago

Codecov Report

Merging #187 (3c0a572) into master (0948322) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #187      +/-   ##
============================================
+ Coverage     38.83%   38.85%   +0.02%     
  Complexity      855      855              
============================================
  Files           105      105              
  Lines          2951     2952       +1     
============================================
+ Hits           1146     1147       +1     
  Misses         1805     1805              
Impacted Files Coverage Δ
lib/Utils/Checker/Rules/ScopeRule.php 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cicnavi commented 1 year ago

Just for information why on why it was previously raising error on invalid scope, particularly for "Authorization Code Flow" OIDC core spec for 3.1.2.6. Authentication Error Response states:

An Authentication Error Response is an OAuth 2.0 Authorization Error Response message returned from the OP's Authorization Endpoint in response to the Authorization Request message sent by the RP. If the End-User denies the request or the End-User authentication fails, the OP (Authorization Server) informs the RP (Client) by using the Error Response parameters defined in Section 4.1.2.1 of OAuth 2.0 [RFC6749]. (HTTP errors unrelated to RFC 6749 are returned to the User Agent using the appropriate HTTP status code.) Unless the Redirection URI is invalid, the Authorization Server returns the Client to the Redirection URI specified in the Authorization Request with the appropriate error and state parameters. Other parameters SHOULD NOT be returned. In addition to the error codes defined in Section 4.1.2.1 of OAuth 2.0, this specification also defines the following error codes

In OAuth2 4.1.2.1 Error Response there is the following error parameter available:

invalid_scope The requested scope is invalid, unknown, or malformed.

Did you encounter a situation in which this implementation caused some issues or?

pradtke commented 1 year ago

We are helping a university integrate a SaaS product that only uses OIDC, and the client is requesting offline_access with no option to adjust the requested scopes. I found the offline_access part of the spec to be some what confusing - especially since we already (I'm pretty sure) issue a refresh token that can be used for offline access - and the client does not send a prompt value. I saw the part of the spec that mentioned ignoring unknown scopes and thought this would be the easier way to address the issues. I'm open to other ideas.

cicnavi commented 1 year ago

Ahhh, ok, then this is related to #154. The thing is, we always release refresh token, but we should only release it if the client requests it using 'offline_access' scope...

I would rather vote to add support for 'offline_access' scope, than to ignore unknown scope. The thing is, from our experience, the explicit error on invalid scope is valuable in scenarios of scope eviction... For example, we had a situation in which we supported custom scope for some time, then decided to quit supporting it... I would rather have an implementer to see the error with explanation on why he can't use some scope instead of to simply stop receiving corresponding claims... (of course, all RPs were informed upfront about that, but still...).

Or, if you need a quick fix, I would rather have this specific scope ignored in the checker, so we can implement it properly later. Or maybe it would work if you add it to the default supported scope. There is scope 'openid' there already I think.. Actually, I guess this case is in a way similar to scope 'openid', since if the scope 'openid' is present, then the ID token is released...

pradtke commented 1 year ago

Makes sense. I can see the benefit of client knowing it request an invalid scope. I'll take a look at some of your suggestions