Codecov Report

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

@@             Coverage Diff             @@
##             main   #22723       +/-   ##
===========================================
+ Coverage   45.36%   65.87%   +20.50%     
===========================================
  Files         244     1073      +829     
  Lines       13333   116281   +102948     
  Branches     2719     2931      +212     
===========================================
+ Hits         6049    76596    +70547     
- Misses       6983    35435    +28452     
- Partials      301     4250     +3949     

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 :(

@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.

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

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.

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.

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.

@lfrancke
Ah, I misremembered. Thanks for clarifying.

I'll approve and merge this one. The code LGTM. Thanks for your time!

Thank you very much!