Unify OIDC provider claim mapping

[sagikazarmark]

There are 3 pending PRs adding support for overriding an official claim based on another existing claim in the token:

• 1634

• 1691

• 1776

Each provides a different configuration model and a different implementation. Ideally, they should follow the same pattern.

In the configuration, each introduces a new config key (following different naming conventions). I would either like to see a unified naming convention, or better yet, a new section for mapping config, something like this:

claimMapping:
    preferred_username: other_user_name
    email: mail
    groups: "cognito:groups"

On the implementation side I like the idea to restrict changing the claim, if it's already present. Standard OIDC claims must always take precedence. (Both #1691 and #1776 are in violation of that requirement at the moment)

Existing configuration (like usernameKey) should be deprecated and the new config structure should take precedence.

On the implementation side, I like the style of #1634, we should probably pursue that.

Last, but not least: tests are must have.

@xtremerui , @cyrilix , @Lemmons can you please collaborate on this one? I'd like to hear your opinions as well and preferably reach a consensus before merging those PRs. Thanks!

[Lemmons]

This seems quite reasonable, and I agree having a consistent way to override mappings makes sense. Unfortunately it also represents a bit extra work compared to what any of us have proposed in our prs.

[sagikazarmark]

I imagine the first step could be adding the new config and deprecating any existing keys, then rewriting the existing PRs.

If someone is up for the task, feel free to take it. If not, I'll try to pick it up sometimes next week.

[xtremerui]

I totally agree to make the configuration into a section. Here at Concourse team we have also prioritized the work of moving from our own fork of Dex to upstream (which means get our PRs merged ASAP), we'd like to take this task if @Lemmons and @cyrilix don't mind.

I'd cherry pick the commits from overriding email key #1691 and groups claim key #1776 into #1634. Then I will update #1634 to follow the suggestion that @sagikazarmark mentioned.

How does this sound?

[sagikazarmark]

Sounds great! Thanks a lot!

[Lemmons]

thanks @xtremerui, that sounds good to me!

[cyrilix]

@xtremerui ok

[rbtr]

@sagikazarmark I understand wanting to design the most flexible and complete feature possible, but it feels like you're bike-shedding instead of accepting any of the several valid implementations already proposed so that this feature is available to your users who need it sooner rather than later :)

e.g. I would like https://github.com/dexidp/dex/pull/1631, which has been largely ignored for the past 6 mos. And I would much prefer to have it merged so that we have the feature available before we do all this trying to fix it before it's broken. You could make one of these existing implementations available while you come up with the perfect generic solution, rather than blocking these valid use-cases while you design.

[sagikazarmark]

@rbtr relax, an implementation has already been provided for this in #1634. It needs a few final touches, but other than that it's good to go.

Sadly, dex's API and configuration is already quite chaotic, and I'd much rather avoid adding to that chaos. Sorry, if it's not fast enough for you.

[haarchri]

did you guys have a timeline ? because we really love to see this feature as of mail claim in our oidc upstream ;)

[rbtr]

@sagikazarmark that's good to hear! Again, I understand the intention with making sure things are as good as they can be. But hopefully you can understand that watching legitimate solutions languish can be frustrating. As an end-user, I would prefer to have a good solution to my specific problem, rather than the perfect solution to my class of problems. But as a developer, I'd rather make the general case solution than a snowflake. So I understand the trade-off between those will vary according to your priorities. Just wanted to see if I could motivate some progress here.

[sagikazarmark]

@rbtr I understand the frustration, we are doing everything we can, but sadly Dex was on a life-support lately. Gentle nudging always helps, especially when a PR is waiting for review because we can't always keep track of the status of every open PR, calling out maintainers for not merging a PR for 6 months...not really. :\

@haarchri @xtremerui was pretty responsive (and helpful) so far, so I'd imagine he will make the requested changes by next week and then we can tag a release. (Admittedly, that's long overdue now)

[sagikazarmark]

Fixed in #1634

[sagikazarmark]

I'm gonna tag a new release tomorrow.

[haarchri]

@sagikazarmark is the new Release 2.25.0 ? which implements the function ?