Introduce 'scope' option to oidc jwt in client credentials grant flow

[lordvlad]

In some cases, the jwt passed in a client-credentials grant flow requires a scop claim. This commit introduces the option.

Let me know if I should update the docs somewhere.

[quarkus-bot]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should preferably start with an uppercase character (if it makes sense!)
• title should not start with chore/docs/feat/fix/refactor but be a proper sentence

_{This message is automatically generated by a bot.}

[sberyozkin]

@lordvlad thanks for the PR, can you do a few minor updates please, there should be a constant for the scope in OidcConstants, unfortunately we don't recommend using lambdas at runtime, and please make the config list property name plural if you don't mind because we have it plural for the code flow configuration, just to keep it more consistent

Thanks

[lordvlad]

• plural
• use scope constant string
• remove lambda

[lordvlad]

• rerun code format

[lordvlad]

@sberyozkin thanks for the feedback.

Have another look now. Should I squash the commits now or will github do that on merge?

Also, why won't you allow lambdas? Issues in native mode?

[sberyozkin]

Hi @lordvlad Sorry for a delay, I've been wondering how to test this new property - we do have integration tests for various client JWT authentication options, also oidc-tenancy emulates Apple authentication case and actually parses the JWT authentication token but it would be difficult to extend it further.
Can you please give me a favor and start with adding the first test for JWT authentication in OidcCommonsUtilsTest ? Hopefully it should be easy to setup, You can copy some code from OidcUtils to parse the token into JsonObject and confirm the expected claims are set, gibe it a try please.

Should I squash the commits now or will github do that on merge?

Please squash once the test is complete

Also, why won't you allow lambdas? Issues in native mode?

My understanding it might have at least marginal performance cost

[sberyozkin]

@lordvlad I can help with the test if you'd like but hopefully it won't take a lot of your time

[sberyozkin]

@lordvlad I've pushed a basic test - we need to customize the separator, space should be default, please add another property similarly to how it is done for OidcTenantConfig, thanks

[lordvlad]

Hey. Thanks for the update, I'll try and update in the upcoming days.

…

On Thu, Jul 13, 2023, 18:21 Sergey Beryozkin ***@***.***> wrote: @lordvlad <https://github.com/lordvlad> I've pushed a basic test - we need to customize the separator, space should be default, please add another property similarly to how it is done for OidcTenantConfig, thanks — Reply to this email directly, view it on GitHub <#34686 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJJJ2L4UN7CZSK2ECNDWKTXQAOCLANCNFSM6AAAAAA2GO5G3A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[sberyozkin]

@lordvlad hi, np, I might add it, it is just that by default it is usually a space, thanks

[lordvlad]

@sberyozkin done. Extended tests. Had to adjust the test cause Set has no guarantee on order.

[sberyozkin]

Thanks @lordvlad, nicely done.

I've been actually thinking about this PR over the weekend, I wanted to ask a question, why is scope required for this token, as this token is not really meant to be reused anywhere, with the scope implying this token is actually about to be used beyond the authentication ? Is this just a custom provider specific requirement for the authentication token to contain scopes ?

The main reason I'm asking, we have to introduce 2 properties to cover a scope claim string, but I wonder if it is the right time to do what we do with the additional, possibly non-standard properties for starting and completing authorization code flow, for the extra oidc client parameters, etc. I think we can introduce an extra-params Map so the configuration would look like this:

quarkus.oidc.credentials.jwt.extra-params.scope=read,write

A bit more verbose but now we can cover whatever custom claims this token needs to have.

Does it sound reasonable to you ? I can update the PR...

[lordvlad]

It really seems to be very specific to this provider (netsuite), I haven't encountered that anywhere else yet.

Making it more generic makes absolute sense, sure! Though maybe extra-claims would be a better name?

[sberyozkin]

@lordvlad extra-claims makes sense in the context of configuring this token, sounds good

[sberyozkin]

@lordvlad Looks like you merging the main branch into this PR has caused problems. Unless I'm mistaken, the right way to continue is to recreate PR in a new branch, cherry-picking all commits made prior to the merge; you can probably avoid it but this is how I used to deal with similar cases, Guillaume was advising something along these lines.

[lordvlad]

Superceded by #34883