fix: use OIDC standard claims, if result from mapping is null

[strehle]

With #1925 there was a fix already, however using custom claims can lead to error situations. With this fix, that error situations are prevented if standard claims in id_token, e.g. custom attribute is not in token, but family_name and given_name are also in the token as string. This fix is for OIDC IdP integration, therefore it makes sense to use as fallback also OIDC standard claims.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186311897

The labels on this github issue will be updated when the story is started.

[peterhaochen47]

I have concerns from an operator UX perspective.

Example of the exact impact of this PR (possibly in combination with the prior PR #1925 ):

Example attributeMappings in an external IDP config set by the operator:

    "attributeMappings" : {
...
      "given_name" : "preferred_first_name",
    # do not actually wish to map the phone number claim into UAA
...
    }

Example external token:

{
...
"given_name": "Bartholomew",
"phone_number": "123456789"
# does not actually have "preferred_first_name" claim for some reason (e.g. config mistake in their IDP "claim export" setting); the "preferred_first_name" would have been "Bart" if it was exported correctly
...
}

Resulting UAA token after this PR:

{
"given_name": "Bartholomew",
"phone_number": "123456789"
}

It seems to me that this PR would be "doing too much" for the operators by offering too much fallback mechanism.

If their intention is to have this mapping "given_name" : "preferred_first_name" and the mapped claim preferred_first_name is not found in the external token, it should be made obvious to them that their intended setting did NOT work, rather than offering them layers of opaque fallback mechanism to mask the problem. If I were the operator, I would rather see a NULL value for given_name in UAA token in this case so I know I need to fix this.

And if the operater's intention is to not import the phone number into UAA (by not adding an attributeMappings entry), then I would not want to import it for them.

Overall, if I were an operator, I would much rather have the actual UAA behaviors be completely deterministic based on my config, rather than having a lot of fallback & outcomes that I do not intend.

[strehle]

Ok so the 2 fall back I see, however can we agree that we should have an option to get the OIDC name if no mapping is configured

Currently all admin have to configuration mappings and we have seen that these things can go wrong.

What we want have in future. There is scope in configuration already and in OIDC you can ask for scopes email and/or profile
-> scopes="openid email profile"

From a standard OIDC IdP you will get then automatically email, email_verified, given_name, family_name because these are the standard names, so in such cases a user attribute mapping should come automatically, ok ?

[strehle]

@peterhaochen47 now only with one default, if custom mapping is null or if there is no custom mapping.

With that externalGroupMapping is not needed at all if profile scope is used and OIDC IdP behaves OIDC compliant

[peterhaochen47]

Ok, now I agree that some defaulting based on the OIDC standard claims is fair. I think we can provide the following default values at the IDP config level (defaulting at the config level might also simplify our code here):

config.attributeMappings.email => email
config.attributeMappings.given_name => given_name
config.attributeMappings.family_name => family_name
config.attributeMappings.phone_number => phone_number
config.attributeMappings.email_verified => email_verified

Aka, if the operators DO NOT configure a mapping (left blank), then we use the above defaults.

But if the operators DO configure a mapping, but the mapping fails to get a valid claim from the external token, then I would argue that we should let it fail (and not fallback), so that the original intentions of the operators are respected. Going back to the above example:
Example attributeMappings in an external IDP config set by the operator:

    "attributeMappings" : {
...
      "given_name" : "preferred_first_name",
    # do not explicitly map the phone number claim into UAA
...
    }

Example external token:

{
...
"given_name": "Bartholomew",
"phone_number": "123456789"
# does not actually have "preferred_first_name" claim for some reason (e.g. config mistake in their IDP "claim export" setting); the "preferred_first_name" would have been "Bart" if it was exported correctly
...
}

Then I think the following resulting UAA token is fair:

{
"given_name": "NULL",
"phone_number": "123456789"
}

Since the operator did NOT set config.attributeMappings.phone_number, so we default it to the standard claim phone_number. But since the operator DID set "given_name" : "preferred_first_name" so I think we should respect that choice and be transparent that their intended config did not work.

[strehle]

ok, then cancel this