Retrieval of the claim for config['oauth_password_claim']

restena-sw commented 1 week ago

Prerequisites

[X] I have searched for duplicate or closed feature requests
[X] I have searched for plugins that provide already provide the feature

Proposal

We are trying to use oauth_password_claim but consistently get the response Error: OAuth token request failed: Password claim (magicmushroom) not found. The word in parenthesis is the claim that contains the short-lived password.

A scope with the same name exists on the server, and is requested client-side with $config['oauth_scope'] = "email openid magicmushroom offline_access"

I first suggested a bug with the server software (simpleSAMLphp with module simplesamlphp-module-oidc) but the good people there walked me through the verification that the claims exist server-side and are returned just fine.

The catch here is: these scopes are returned in the "userinfo" endpoint; while Roundcube seems to look exclusively in the id_token (my reading of the source code, which may well be incorrect of course).

The folks at the server-side write:

"I assume the client uses authorization code flow in which Access Token is being released, so those claims will not be included in ID Token... It would be great if you could verify results from the userinfo endpoint.... You would need to get a hand on access token and use it in request to userinfo endpoint." - which I did, and I got the expected claims in userinfo. Source: https://github.com/simplesamlphp/simplesamlphp-module-oidc/issues/246

Can you confirm that Roundcube is using the authorization code flow? If so, it appears like it needs to be possible to configure (or infer) at which endpoint to look for the claims.

Motivation and context

Using the new non-XOAUTH2 feature for IMAP backend is probably only functional under specific circumstances. Making it work in a wider set of settings would be beneficial.

alecpl commented 1 week ago

Indeed, the claim is supposed to be part if the id_token.

Roundcube uses userinfo request only when the id_token does not contain the username. The userinfo request is not used on token refresh. The point of OIDC is that you don't use userinfo at all (less requests needed as claims are inside id_token).

That being said, we could allow the claim to be fetched using userinfo. Would you like to provide a pull request?

restena-sw commented 1 week ago

I investigated a bit further with my colleague from the webmail team. Indeed, we found that if you set the config item "oauth_identity_fields" to "null" then all the custom claims arrive in the id_token (not sure I understand the causality path that leads to this result; are they fetched from userinfo and then merged into id_token so we see them here? Or do they genuinely come into id_token because something in the request is different?).

That's actually okay, we can configure RC like that and then the claim is found.

But that leads to a secondary issue in that "email" is used as the IMAP login username. In our case, the email address is not the login username, so the login fails because the username doesn't match the password token. "sub" does contain the username, but we could not convince RC to use that in preference to the "email" claim.

We have a working prototype now which is a bit hacky... the server returns the content for "sub" inside the claim "email" so that everything matches up for the IMAP login. It's not the cleanest of approaches though.

Maybe an option for "use this claim for the username to log in as" would be useful?

alecpl commented 1 week ago

Maybe $config['oauth_identity_fields'] = ['sub']?

restena-sw commented 1 week ago

Well, no: sub is contained in the id_token, and so, according to what you write above: "Roundcube uses userinfo request only when the id_token does not contain the username." - it will then not query userinfo, and then the password goes missing. We have tried and tested exactly that, with exactly this result.

So, we can either have the correct username from "sub", or we can have the password from the password_claim config option, but not both simultaneously.

Leaving identity_fields = null will query userinfo, and retrieve both email and the password_claim, and subsequently try to use the email address as IMAP login, which is then the wrong username (but the correct password).

That's why our "cheating" with populating the username into the email field worked.

Sleeping a night over this, I think there is a clean solution to it that needs no code changes:

server-side, create a claim "imaplogin" alongside the one for password_claim.
Set the oauth_identity to "imaplogin".

The id_token will be fetched, "imaplogin" is not contained within, then userinfo is queried, and it finds both fields it needs.

We will go ahead with that, and I'll confirm here if this works.

Even if it does... it's a somewhat unintuitive combination of claims and a play with their existence-or-not in the initial token. I somehow imagine this should be somewhat more straightforward to configure; but I can't exactly say how.

alecpl commented 1 week ago

Indeed. Cleaner solution would be "if there's no password claim in the id_token use the userinfo".

ps. with my custom (Laravel-based) OIDC server I have no problem with adding claims into id_token.

restena-sw commented 1 week ago

Your last sentence intrigued me to check where custom claims "should" be. You could read the two most recent comments to https://github.com/simplesamlphp/simplesamlphp-module-oidc/issues/246 for entertainment.

So, indeed: the OIDC allows for custom claims to be in either the id_token or the userinfo endpoint; and OIDC Server software should allow the admin to specify where a custom claim goes.

If that were in place in my software, using userinfo would be optional again.

But even if it is not: it is actually possible for the client to indicate where it wants specific claims to be in the server's responses - I was pointed to https://openid.net/specs/openid-connect-core-1_0.html#ClaimsParameter . If I capture correctly your sentiment that you'd prefer userinfo not to be used at all, then on client-side, you could indicate that RC prefers to get both oauth_identity_field and oauth_password_claim in the id_token; and if a server honours that, consulting userinfo would never be necessary.

I guess the claims parameter would then need to look like this:

{
   "userinfo":
    {
     "email": null,
    },
   "id_token":
    {
     "<the content of oauth_identity_field>": null,
     "<the content of auth_password_claim>": null
    }
  }

("email" can't be added to id_token because the spec in 5.4 requires that particular claim to be in "userinfo" when the authorization code flow is used)

alecpl commented 1 week ago

For me the whole point if OIDC (over OAuth2) is that you can make one HTTP request instead of two, i.e. claims inside id_token. But I don't claim I'm an OIDC expert. You may be right about that sentence in 5.4, but it's strange it does not say MUST nor SHOULD.

restena-sw commented 1 week ago

I have read and written a few RFCs, and really like MUSTs and SHOULDs. So, unsurprisingly I agree that the wording is not what you'd typically want to see in an RFC. Still, it "sounds" quite normative, and I speculate that "are sent" is meant to be read in the spirit of "MUST be sent".

Anyway: I believe the spec allows the client to have that preference for a single round-trip by using the "claims" parameter to indicate that as many claims as possible are in the immediate id_token response, so that seems to be the best way forward to avoid userinfo to the biggest extent.

The underlying question is of course: why did the spec authors think that they need to fix the claims from those four pre-defined scopes into userinfo? Why can't those be sent in the id_token, too? That, I obviously don't know.

roundcube / roundcubemail