oidc-wp / openid-connect-generic

WordPress plugin to provide an OpenID Connect Generic client
https://wordpress.org/plugins/daggerhart-openid-connect-generic/
256 stars 155 forks source link

Initial login via OIDC triggers conscent on scopes - profile scope is open to user to untick - breaks matching #440

Open Glowsome opened 1 year ago

Glowsome commented 1 year ago

Is your feature request related to a problem? Please describe. When the plugin uses the 'profile' scope on initial login it is given a consent screen, where profile can be ticked off. This breaks matching if it is based off an attribute from that scope ( like preferred_username)

Describe the solution you'd like When the option 'Link existing users' in the plugin is checked, and the matching attribute is not provided over OIDC,it should error or potentially even deny access, as no consent was given to the attributes the application needs.

Describe alternatives you've considered IF we were to create a custom scope on the IDP ( with auto-consent) it would not break matching. -> however not every organization will/can be convinced of facilitating this. IF matching is done over the 'profile'-scope and the consent is declined, the specified attribute for matching is not sent via OIDC then there is no alternative.

Additional context This behaviour is seen on Microfocus's AccessManager where the scope 'profile' is by default enabled to be uncheck by users on initial login/authentication and the consent screen.

timnolte commented 1 year ago

I'm not really sure that I'm OK with making this sort of change. There is fallback logic intended to go down a path in order to successfully create users. When it comes to other organizations, as you mention it, requiring a non-standard custom scope may not be an option. There may even be some IDPs that wouldn't allow a custom scope to be created, which could make things more problematic.

I'm inclined to look through the OIDC specs to see if there is recommended guidance there. Honestly, the plugin is really only designed to be used with the Authorization Code Flow and not any sort of consent flow. I sort of feel like this is a mismatch, or even misconfiguration, with the IDP.

I'd also say that this use case seems like one where you should be using the openid-connect-generic-user-creation-test hook to handle this as you want. https://github.com/oidc-wp/openid-connect-generic/wiki/Hooks---Actions-&-Filters#openid-connect-generic-user-creation-test

Glowsome commented 1 year ago

@timnolte i have a feeling that you are interpreting this issue i am facing wrongly. The whole consent thing is described in a code-flow in https://openid.net/specs/openid-connect-core-1_0.html in section 3.1.1

So this is an IDP-thing, but on most IDP's both the scopes 'email' and 'profile' are publicly available for any client which identifies itself in the correct way / is registered with the IDP.

Default setting for the scope 'profile' is that (still keep in mind that i am scoping on the IDP/vendor i have running) is that a user, when presented on an initial login (as in when the plugin was installed/configured) a user can under these conditions untick the 'profile' scope on the consent-screen.

As multiple applications (in a running production-environment) use this scope it is not a simple case of "why don't you just solve it to take that right away".

The effect in this is that matching will fail as the attribute to match with is not present, the plugin does its best in solving this issue, and will create a new account based off the data that is being offered.

As matching is vital in environments where one is switching from other authentication-mechanisms to the OIDC-plugin (my business-case is ~3k users)

I would like to brainstorm with you on how to react on the cases where consent is not given by a/the user.

If i were to speak my mind on how to deal with this then the solution would be to - when a match cannot be made due to the attribute to match on was not available from the IDP should produce a deny access on the wordpress/plugin side/

timnolte commented 1 year ago

@Glowsome so I understand what you are talking about. My issue, and this is the issue with all WordPress plugins, is that what you are proposing is changing a big part of the way the plugin functions and may have major impacts on existing implementations.

If you look at the specifications in section 3.1.2.1 it mentions that consent requirements are something that can be sent in the original request using the prompt parameter with a value of consent, and there is a specific required Error response, consent_required, the IDP would send back to the Client to indicate that consent wasn't granted.

Section 3.1.2.4 does talk about the consent process, as well as Later in section 5.4 it does state the case you have outlined:

In some cases, the End-User will be given the option to have the OpenID Provider decline to provide some or all information requested by RPs.

In section 5.5 it does talk about using the claims request attribute in order to possibly mark certain claims, such as a username, as essential however it doesn't seem to indicate that the IDP would necessarily return an error if the specific claim was denied by user consent. Additionally, the claims parameter is an optionally supported feature for IDPs.

Looking specifically at the Authorization Code Flow implementation guide it provides no clear guidance on the sort of handling we are doing with matching existing accounts.

I'll have to do some more thinking on this, as I stated already, my concern is about changing the expected functionality for existing implementations of the plugin. I'm off-hand inclined to provide another option that one would turn on to make a match required. However, as I've mentioned, this can already be done with the hooks available as needed.

Glowsome commented 1 year ago

@timnolte Is it worth to investigate a path where one would add an option (like a checkbox) which when (default) not ticked the plugin follows the current behaviour, but when ticked the logic changes to facilitate matching as i proposed?

timnolte commented 1 year ago

@Glowsome well yes, that's what I was talking about in my last comment. The thing though is that accomplishing this change in logic is already possible via the hooks we provide by anyone that wants to change that flow.

Honestly, unless there is a lot of demand for this as a fundamental change, I feel like there is little incentive to add all of that complexity into the code that I would then have to manage and support. From my standpoint there is more value in adding items that are in the OIDC specs that aren't supported or handled then making this sort of change.

Glowsome commented 1 year ago

@timnolte This does not all have to come down on your shoulders alone, as with the enhancement for the acr_values i am up to exploring this path, just wanted to make sure it is in line with the way you see development of the plugin.

On the other hand - easy way out (for now) would be to make a custom scope with auto-consent which holds the attributes the plugin requires.

Glowsome commented 1 year ago

@timnolte been debugging a bit with the consent thingy ( with use of OIDC Debugger And if i look at a/the results when i explicitly untick profile on a/the consent-screen the response also references it. image

In this request i have asked for openid email and profile as scopes. As openid is mandatory it drops off , but as you can see without having given consent to the profile -scope it will not return it. This leaves email as scope returning in the result.

I think if we were to check for what scopes are actually returned compared to the requested scopes in configuration it is a safe way to determine if a/the user has given consent or not. From that fact on i think it should be possible to at that point already determine if matching would work or not. This ofc. is seen from the situation where a scope was configured- expecting the retrieved information is needed/mandatory for correct functionality in the plugin's matching.

timnolte commented 1 year ago

@Glowsome excellent debugging. I will have to do some testing against some of my available IDPs. What is concerning is that neither of the OIDC documents, OpenID Connect Basic Client Implementer's Guide & OpenID Connect Core 1.0, I've read through provide any indication that an IDP would respond with the scopes allowed. That doesn't mean we couldn't at the very least leverage a returned scopes check if it is available. So we'd have to check if scope is returned, if so then check which scopes were returned and proceed if a "required" scope was returned. In the event that no scope is returned in the response then we'd proceed as we always have.

Glowsome commented 1 year ago

@timnolte i can - i f you want put my IDP as 'testsubject' ( the vendor states its implemented to full oidc-spec) :)

timnolte commented 1 year ago

@Glowsome I checked the debug logs of my Keycloak IDP responses and I don't see scope in the responses. I will say that any IDP implementation can meet full specs while still doing things that are not in the spec, so long as they aren't against the spec. I don't believe the specs are super explicit about that.

timnolte commented 1 year ago

@Glowsome so checking the responses in the plugin debug logs from my Amazon Cognito instance I'm also not seeing scope in the responses. I'm going to create a client configuration on both to use with the OIDC Debugger and confirm.

timnolte commented 1 year ago

@Glowsome so yeah, I've ran through both Keycloak & Amazon Cognito with the OIDC Debugger and neither return the scope attribute. I went looking and according to the Microfocus Access Manager documentation it also gives no indication that it would return scope in the response. I'm very curious how that managed to be returned if their own documentation doesn't even mention it.