Allows OIDC integration to handle the case where a user only has a single group

[lfrancke]

Comprehensive Summary of your change

See the linked issue for details on the issue.

This PR first tries to parse all claims as an array first and if that fails it will try to parse it as a string.
The internal UserInfo struct is unchanged.

Note

I tried to run the tests and I tried to build Harbor and run it locally but I failed miserably.

a) I couldn't get the tests to run (failed on a Postgres requirement)
b) I was able to build using make install but I couldn't get the docker images to actually run due to some permissions issue (the images seem to require root?)

I understand if you don't want to accept this PR untested and I might try again next week but for now...this is it :)

Issue being fixed

Fixes #15416

Please indicate you've done the following:

• Well Written Title and Summary of the PR
• Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
• Accepted the DCO. Commits without the DCO will delay acceptance.
• Made sure tests are passing and test coverage is added if needed.
• Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.86%. Comparing base (c8c11b4) to head (60c2e9c).
⚠️ Report is 615 commits behind head on main.

Files with missing lines	Patch %	Lines
src/pkg/oidc/helper.go	86.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #22723       +/-   ##
===========================================
+ Coverage   45.36%   65.86%   +20.49%     
===========================================
  Files         244     1073      +829     
  Lines       13333   116281   +102948     
  Branches     2719     2931      +212     
===========================================
+ Hits         6049    76586    +70537     
- Misses       6983    35447    +28464     
- Partials      301     4248     +3947     

Flag	Coverage Δ
unittests	65.86% <86.66%> (+20.49%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/pkg/oidc/helper.go	43.88% <86.66%> (ø)

... and 987 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lfrancke]

Thanks for running the tests. I'm not entirely sure what the actual failure is for the UTTEST run. There are a bunch of Errors in there but they don't seem to stop the build.

I'm not very familiar with this :(

[reasonerjt]

@lfrancke Thanks for this PR.

// According to RFC 7519, a claim can be either a string (single value) or an array (multiple values)

Could you please point out which section in the RFC has such statements? I tried to find it but only found has statement about aud claim

In the general case, the "aud" value is an array of case-
   sensitive strings, each containing a StringOrURI value.  In the
   special case when the JWT has one audience, the "aud" value MAY be a
   single case-sensitive string containing a StringOrURI value.

But this does not have to be applicable to groups claim IMHO. Even the wording in the RFC seems to imply that most cases a claim should be either a string or an array. And "aud" is a special case, which in general should be avoided.

[lfrancke]

I have to admit that this (the original issue) is where I took that claim from. I'm happy to remove the comment from the PR. I agree: I can't find any reference to it in the RFC itself.

FWIW I've also opened a support case with JumpCloud

[Vad1mo]

While the RFC751 is not clear, many OIDC implementations support both use cases. I looked into Dex, which supports both string for a single entry and array for multiple entries.

There is no harm in adding this fix, as it won't break backwards compatibility.

[Vad1mo]

patch verified

[reasonerjt]

many OIDC implementations support both use cases. I looked into Dex, which supports both string for a single entry and array for multiple entries.

@Vad1mo
Could you elaborate?

I really doubt it, I tested with Dex, which was years ago, but I recall Dex sets an array in "groups" claim, not single string.

My point is, if we have multiple mainstream OIDC providers which set single string in the "groups" claim I'm fine with this PR. But if it's only JumpCloud we probably wanna keep it in the fork, b/c it seems to me that this is an implementation issue in JumpCloud.

I don't think we should update the code again when we see some oidc provider tries to put a map in the "groups" claim.

[lfrancke]

It's not only JumpCloud. The original issue was opened by someone else using "IdentityServer 4". PingFederate does the same and I'm 99% certain that I've seen others with the same behavior.

There does not seem to be a spec for the groups claim either so to me it seems reasonable to support "what's out there".

We solved it for us by just assigning a second dummy group so it's not a big problem but it'll make the lives of others easier at very little cost.