fix: handle dot-containing claim names in nested claim resolution - #1375
Conversation
Replace explode('.') with greedy longest-prefix matching that tries the
full remaining path as a literal key first, then progressively shorter
dot-prefixed segments. This correctly handles URL-based claim names
(e.g. "https://idp.example.com/claims/groups") and object keys with
literal dots (e.g. "user.role") as permitted by OIDC Core §5.1.2.
Backward compatible: existing dot-separated nested paths resolve
identically since the algorithm falls through to the same splits.
Fixes nextcloud#1373
Related: nextcloud#1100
Signed-off-by: Strobel Pierre <strobelpierre@gmail.com>
adbf700 to
06b4715
Compare
|
@strobelpierre Lgtm. Thank you (and Claude 😁) @andreblanke @dragonpil Can you confirm the backward compatibility is preserved in this PR for all your use cases? |
Claude and I thank you too, haha 🤝❤️ |
|
@strobelpierre I don't know what's gonna become the standard but here is my advice: Keep the "generated with AI/whatever you use" footer in PR comments and in issues. This is more honest and has less chances to trigger readers that don't like AI and will immediately detect AI generated content from the style and the length. |
I completely agree with you, but on another project I had a PR that was fine and added a real feature, and I left the mention claude code that I use to formalize the PR format and also to code. The PR was rejected on the grounds that they don't accept code generated by AI, even though all the tests were fine. There are gatekeepers who automatically reject it. |
|
@julien-nc Looks good to me as well. Thanks for the ping. |
I hope this does not turn into a war between AI supporters and detractors. |
|
@andreblanke Thanks a lot for the super fast feedback! |
|
Hello there, 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.) |
|
Sorry for the late Feedback. Looks good for my usecases. |
Summary
When
--resolve-nested-claims=1is enabled,getClaimValues()usesexplode('.')to split claim paths. This breaks for:https://idp.example.com/claims/groupsgets split at hostname dots{"user.role": "admin"}gets interpreted as nested navigationApproach: greedy longest-prefix matching
Replace
explode('.')with a recursive resolver (resolveNestedClaim()) that:This preserves full backward compatibility — existing dot-separated nested paths resolve identically since the algorithm falls through to the same splits.
Examples
https://idp.example.com/claims/groups{"https://...groups": ["admin"]}["admin"]https://idp.example.com/attrs.role{"https://...attrs": {"role": "admin"}}"admin"https://idp.example.com/attrs.user.role{"https://...attrs": {"user.role": "admin"}}"admin"custom.nickname{"custom": {"nickname": "alice"}}"alice"(backward compat)a.b{"a.b": "flat", "a": {"b": "nested"}}"flat"(literal key precedence)Fixes #1373
Related: #1100
Test plan