Accept list of strings in audience claim in token auth

[sagikazarmark]

Fixes #3735

[milosgajdos]

The PR looks great. I'm a bit on the fence about introducing a new type just to serialize a simple struct field though. I understand you're trying to be a good citizen and make the encode/decode faster but it feels like premature optimisation to me. 🤔

[sagikazarmark]

It's not about serializing a simple field, but supporting both lists and single values in the token at the same time. If I replace string with []string, tokens with a single string value will fail.

What do you suggest?

[milosgajdos]

Ah, yes you are right. I've misread that 😬 Let me think this through -- I was thinking, since it's localised to token package we could simply call that type Audience, but I understand you want to make it reusable should the need arise. Leave this with me for a bit.

[sagikazarmark]

BTW here is the JWT implementation that I used as an example: https://github.com/golang-jwt/jwt/blob/main/types.go#L100

So it's not like I invented anything new here. :)

[milosgajdos]

I believe you :) but it's also kinda interesting seeing the golang-jwt calling their type ClaimsStrings; I'm mulling over something like AudienceStrings 🤔

[sagikazarmark]

I can rename it if that's the only thing keeping you from accepting the PR. :)

But in general, I like to name types based on what they do, not how they are used.
The type allows deserializing a string into a string list.

An instance of that type is going to be the audience.

I don't know if ClaimStrings is used for anything else, but there are other types in there with similar behavior that are used for multiple claims.

Anyway, let me know how to proceed and I'll update the PR.

[milosgajdos]

PR looks 👌 It's just me being silly here over naming...everytime I see Weak* I get ghosts of my java past haunting me

[sagikazarmark]

The alternative is getting rid of the entire implementation and replacing it with the JWT package. ;)

[milosgajdos]

Alright, lets call this type AudienceList -- there is no need to leak implementation details here in the name of the type. If we need the same type elsewhere we can type alias it or come up with some other way to deal with this. Once we rename the type I'm good with the change.

[sagikazarmark]

done

[codecov-commenter]

Codecov Report

Base: 57.18% // Head: 57.10% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (d6ea77a) compared to base (78b9c98).
Patch coverage: 68.29% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3742      +/-   ##
==========================================
- Coverage   57.18%   57.10%   -0.08%     
==========================================
  Files         105      106       +1     
  Lines       10842    10844       +2     
==========================================
- Hits         6200     6193       -7     
- Misses       3954     3961       +7     
- Partials      688      690       +2     

Impacted Files	Coverage Δ
registry/auth/token/token.go	57.48% <0.00%> (ø)
registry/auth/token/types.go	68.75% <68.75%> (ø)
registry/auth/token/util.go	85.71% <83.33%> (-0.65%)	⬇️
contrib/token-server/token.go	34.00% <100.00%> (ø)
registry/storage/linkedblobstore.go	65.13% <0.00%> (-2.69%)	⬇️
registry/storage/registry.go	88.29% <0.00%> (-0.39%)	⬇️
registry/storage/tagstore.go	76.47% <0.00%> (-0.18%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[corhere]

LGTM