Issue-1100: Nested & Fallback Claims

[dragonpil]

As described in Issue-1100 there is current no possibility to mappe values which are nested in other the the root scope. To enable this we change the way how Mapping-Attributes are used. They should understand the . as object seperator.

Sometimes you like to have a fallback in your mappings. So for displayname for example please take the nickname (if it exists), if it does not exists take the full name .... To support this, we change the way how Mapping-Attributes are used. They should understand the | as an alternative mapping.

WIP: Testing need to be done still fully, I dont know if I changed everything needed but on my side it works.

#1100

[julien-nc]

That was fast 🚀
Looks very good.

• You can run composer run cs:check and composer run cs:fix to make sure you respect the project code style
• A little bit of documentation in the README would be nice
• The OCA\UserOIDC\User\Validator\UserInfoValidator also needs adjustments to get the uid
• Also one question: Is it possible in some cases (depending on the IdP and its configuration) that a claim with a dot does not result in getting nested attributes in the token? Like "custom.plop" would give a token payload like

{
    "custom.plop": "the value",
}

If so, we need to make sure this does not break with this new feature. This would mean making the support for nested attributes (and fallback) optional and disabled by default. What do you think?

[julien-nc]

Oh and also if you can sign your commits, that would be awesome.

[dragonpil]

Hi @julien-nc,

thanks a lot for your super fast and helpful review comments!
I've hopefully addressed everything — and for better readability, I’ve done it in separate commits. I’ll squash them all together once we’re close to merging.

So, in short:

✅ All commits are now signed
✅ Code style is fixed
✅ Missing logic in UserInfoValidator has been added
✅ Your question about dotted claim names: you're absolutely right — depending on the IdP, tokens might include flat keys with dots like "custom.plop". That's why the nested claim resolution is now configurable and disabled by default to avoid breaking existing setups.
✅I've also added a section to the README explaining the feature and how to enable it via occ.

Let me know what you think — and thanks again!

[julien-nc]

Awesome, thanks for the changes. As this new nestedAndFallbackClaims setting is provider-specific and quite important, it deserves to be set in the UI in my opinion. If you allowed the repo's owners to push in your branch i can add that.

[julien-nc]

So we have one remaining issue with these changes. It was well spotted by Psalm 💙 .
We historically handle the case where the address is a JSON object in the token. Now that getClaimValue returns ?string we can't get an object anymore in the mapped address token attribute.
In Keycloak, for example, it's possible to produce JSON attributes:

What I mean is: Independently of how we parse the token payload, we might end up with a value that is something else than a string. Specifically for the address attr.

In short, I think we need to adjust the return type of getClaimValue to mixed because we don't know which type we return there, an attribute can as well be an object.

I pushed the change. Hope you don't mind.

[dragonpil]

Awesome, thanks for the changes. As this new nestedAndFallbackClaims setting is provider-specific and quite important, it deserves to be set in the UI in my opinion. If you allowed the repo's owners to push in your branch i can add that.

I just added you as contributor on my fork. Can you help me, how to allow the repo's owner to push in my branch?

[julien-nc]

I just added you as contributor on my fork. Can you help me, how to allow the repo's owner to push in my branch?

It's something you chose when creating the PR. I have the right to push in this PR's branch already. I did push a few commits.

done

[dragonpil]

verry cool @julien-nc, thanks a lot for your support.
Is something still open or should I squash all together?

[julien-nc]

You can squash. I'll read it all again and merge.

[julien-nc]

@dragonpil I realized that the setting was not checked in LoginController::login to compute the claim list that is sent in the authorization URL as GET param. Can you review the commit I pushed?

[dragonpil]

@dragonpil I realized that the setting was not checked in LoginController::login to compute the claim list that is sent in the authorization URL as GET param. Can you review the commit I pushed?

Hi julien-nc,
sorry for the delay. yes that's true... but in your commit I just see a change in the readme.md

[julien-nc]

Hmmm you squashed all your commits in one, then i pushed one on top which only contains a change in the LoginController.
7224904

[dragonpil]

Hmmm you squashed all your commits in one, then i pushed one on top which only contains a change in the LoginController. 7224904

ah ok... sorry used the compare button and just saw this:
https://github.com/nextcloud/user_oidc/compare/5af62e057760a43d94237b4ab854977af0f09825..63814bbd47c73835131ccd55f7cba8fe3f56cafe

so yes I reviewed your change and its fine.

[julien-nc]

Thanks again!

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)