[ELY-2574] Add ability to configure additional scope for authentication request #1925

PrarthonaPaul · 2023-06-28T19:16:06Z

https://issues.redhat.com/browse/ELY-2574

fjuma · 2023-06-28T19:17:51Z

Thanks @PrarthonaPaul. Since this PR is against the wildfly-elytron project, the title, commit message, and description should be referencing an ELY issue.

fjuma · 2023-06-28T20:00:31Z

@PrarthonaPaul OidcTest is currently failing, see https://github.com/wildfly-security/wildfly-elytron/actions/runs/5404971876/jobs/9819885760?pr=1925.

Were you able to get OidcTest running successfully locally?

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java

PrarthonaPaul · 2023-06-28T20:13:42Z

@PrarthonaPaul OidcTest is currently failing, see https://github.com/wildfly-security/wildfly-elytron/actions/runs/5404971876/jobs/9819885760?pr=1925.

Were you able to get OidcTest running successfully locally?

Yes I was able to run OidcTest as well as a full build with tests successfully.

fjuma · 2023-06-28T20:16:00Z

@PrarthonaPaul OidcTest is currently failing, see https://github.com/wildfly-security/wildfly-elytron/actions/runs/5404971876/jobs/9819885760?pr=1925.
Were you able to get OidcTest running successfully locally?

Yes I was able to run OidcTest as well as a full build with tests successfully.

This is the failure from the CI log:

Tests run: 11, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 44.589 sec <<< FAILURE! - in org.wildfly.security.http.oidc.OidcTest
testScope(org.wildfly.security.http.oidc.OidcTest)  Time elapsed: 0.012 sec  <<< ERROR!
java.lang.RuntimeException: ELY23025: Must set 'auth-server-url' or 'provider-url'
	at org.wildfly.security.http.oidc.OidcClientConfigurationBuilder.internalBuild(OidcClientConfigurationBuilder.java:148)
	at org.wildfly.security.http.oidc.OidcClientConfigurationBuilder.build(OidcClientConfigurationBuilder.java:182)
	at org.wildfly.security.http.oidc.OidcTest.performAuthentication(OidcTest.java:175)
	at org.wildfly.security.http.oidc.OidcTest.testScope(OidcTest.java:167)

Might be good to check if maybe you have some local changes that weren't pushed.

PrarthonaPaul · 2023-06-28T20:22:15Z

@PrarthonaPaul OidcTest is currently failing, see https://github.com/wildfly-security/wildfly-elytron/actions/runs/5404971876/jobs/9819885760?pr=1925.
Were you able to get OidcTest running successfully locally?

Yes I was able to run OidcTest as well as a full build with tests successfully.

This is the failure from the CI log:
Tests run: 11, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 44.589 sec <<< FAILURE! - in org.wildfly.security.http.oidc.OidcTest
testScope(org.wildfly.security.http.oidc.OidcTest)  Time elapsed: 0.012 sec  <<< ERROR!
java.lang.RuntimeException: ELY23025: Must set 'auth-server-url' or 'provider-url'
	at org.wildfly.security.http.oidc.OidcClientConfigurationBuilder.internalBuild(OidcClientConfigurationBuilder.java:148)
	at org.wildfly.security.http.oidc.OidcClientConfigurationBuilder.build(OidcClientConfigurationBuilder.java:182)
	at org.wildfly.security.http.oidc.OidcTest.performAuthentication(OidcTest.java:175)
	at org.wildfly.security.http.oidc.OidcTest.testScope(OidcTest.java:167)
Might be good to check if maybe you have some local changes that weren't pushed.

rerunning tests right now.
Implemented the other changes. Will push once I figure out what is causing the error.
Thank you for the feedback!

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java

fjuma · 2023-07-18T21:31:27Z

@PrarthonaPaul Looks like the commits need to be cleaned up a bit so that they all reference the ELY issue and looks like things can be squashed too.

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientSecretCredentialsProvider.java

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcBaseTest.java

fjuma

Thanks for the updates @PrarthonaPaul! I've added some feedback.

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java

http/oidc/src/main/java/org/wildfly/security/http/oidc/Oidc.java

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java

fjuma · 2024-03-19T21:32:47Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java

+    public static OidcClientConfiguration buildWithoutUnsupportedAttributes(String unsupportedAttributesParam, InputStream is) {
+        OidcJsonConfiguration oidcJsonConfiguration = loadOidcJsonConfiguration(is);
+        try {
+            failIfUnsupportedAttribute(unsupportedAttributesParam, oidcJsonConfiguration);


Just wondering if it's possible to detect the unsupported attribute somehow during loadOidcJsonConfiguration rather than after the OidcJsonConfiguration has been created?

If we want it to be inside loadOidcJsonConfig to save parsing time, then don't think so, because loadOidcJsonConfig() uses adapterConfig = mapper.readValue(is, OidcJsonConfiguration.class);. And since scope is a jsonProperty defined in the OidcJsonConfiguration class, it would not fail.
But if we want to do something like

adapterConfig = mapper.readValue(is, OidcJsonConfiguration.class); check for scope inside adapterConfig and fail if scope is not null

then yes, we can do that.
But I am not sure if that simplifies the code any more. The reason why I added a new function is so that we still have a build(InputStream is) function.
If we handle the scope check inside loadOidcJsonConfiguration, then we will instead need to add something like loadOidcJsonConfigurationWithoutUnsuppportedAttributes(InputStream is, String unsupportedOidcAttributes)
Please feel free to let me know if that is cleaner though. I am okay with changing it.

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java

fjuma · 2024-03-20T12:55:47Z

@PrarthonaPaul I think hold off for a bit on making updates based on my latest comments. Am looking closer to see if there's a different way we could handle this.

fjuma · 2024-03-20T16:02:43Z

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcConfigurationServletListener.java

@@ -66,7 +67,7 @@ public void contextInitialized(ServletContextEvent sce) {
                if (is == null) {
                    oidcClientConfiguration = new OidcClientConfiguration();
                } else {
-                    oidcClientConfiguration = OidcClientConfigurationBuilder.build(is);
+                    oidcClientConfiguration = OidcClientConfigurationBuilder.buildWithoutUnsupportedAttributes(is, servletContext.getInitParameter(JSON_CONFIG_UNSUPPORTED_ATTRIBUTE_PARAM));


@PrarthonaPaul I've looked more closely into this and have an alternative suggestion for us to consider.

Instead of needing to update Elytron so that it can cope with attributes that are not supported by the default stability level in WildFly, I think we can add handling in OidcActivationProcessor in the elytron-oidc-client subsystem. In particular, I think we could do the following:

In OidcActivationProcessor, we can get the WEB-INF/oidc.json file from the deployment as follows: deploymentUnit.getAttachment(Attachments.DEPLOYMENT_ROOT).getRoot().getChild("WEB-INF/oidc.json").

That gives us a VirtualFile that we could then read the contents of to end up with a String.

We could then add handling to OidcActivationProcessor#deploy such that if the stability level isn't the community stability level (i.e., ! deploymentUnit.getStability().enables(Stability.COMMUNITY)), then if the contents contain the specific attributes that are only available at the community stability level, deployment should fail. Otherwise, the String can be used to set the JSON_CONFIG_CONTEXT_PARAM servlet context param so that when the OidcClientConfiguration is being built, we won't read from the oidc.json file a second time, we'll just make use of the String we already have.

WDYT of this approach?

I think it is cleaner if in WildFly we can do the stability level validation as stability levels are a WildFly concern not an Elytron concern.

If I am reading this suggestion correctly are we talking about being able to parse from the String directly?

Maybe as a follow up later on another option could be when the Processor in WildFly calls these APIs it can also pass in some predicate for validation?

But for now +1 on constraining to WildFly if we can.

Right, this suggestion is to parse from the String directly in OidcActivationProcessor in WildFly.

As a follow up, we could try to pass in a predicate that would be used for validation. We'd likely need to create a class that extends com.fasterxml.jackson.core.JsonFactory to be able to make use of the predicate when parsing in OidcClientConfigurationBuilder#loadOidcJsonConfiguration.

If we move the predicate option to a Jira issue we can follow up later and decide if we want to do that.

I've created https://issues.redhat.com/browse/ELY-2737 to track this.

I have updated the code for handling unsupported attributes for deployment config and removed the old code.

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java

fjuma · 2024-03-20T19:46:14Z

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcBaseTest.java

+        String scopes = jwtClaims.getClaimValueAsString("scope");
+        if (scopes != null) {
+            if (scopes.contains("email")) {
+                assertTrue(jwtClaims.getClaimValueAsString("email_verified").contains(String.valueOf(KeycloakConfiguration.EMAIL_VERIFIED)));


It seems that Keycloak will set the email and profile scopes by default so just wondering if checking only email_verified and profile are actually enough to successfully test that the configured scopes have been set properly. As an example, I tried tweaking testMultipleScopeValue locally to only set the openid scope. I would have expected the test to fail in this case however it actually ended up passing. Is there a claim that we could check that would only be set if a specific scope other than the default ones has been configured?

Looks like the address claim is a candidate for a claim that would confirm that the configured scopes are being used as expected, i.e., we could update tests to also specify the address scope. Then, the JWT claims will include the address claim as well. This claim isn't included by default so this would confirm that the configured scopes are actually getting used as expected.

I don't think we can do address, because org.keycloak.representations.idm.UserRepresentation
does not have a way to set that. Plus when I added address as a scope, the request timed out.
I saw this earlier with this API as well when I wanted to set offline_access as one of the scopes. It is kind of limiting in that sense.

However, I can change the default list of scopes for the client to just OpenID, so that the results of those scopes are only included if the config asks for it.

I tried adding address locally and was able to see the address added in the claims. So even if we can't actually set a value for it, we can still verify that it's present in the claims. This allows us to verify that setting the scope value actually resulted in the correct behaviour. With the test as is right now, we can't be sure that setting the scope actually had an effect since the current claims that are being tested will be present regardless of the scope configuration.

However, I can change the default list of scopes for the client to just OpenID, so that the results of those scopes are only included if the config asks for it.

Yes another option would be to change the default list of scopes if that's easy to do. I quickly tried that yesterday but wasn't able to get things working as expected but maybe I was missing something.

Yeah, I am trying that now and I am getting a class cast exception.
I am looking more into it to see what is causing that.

I was able to fix this. I added openid as the default scope and the others needed to be added as optional scopes.

Thank you for this update! The tests look great now!

One last comment (that won't block merging either) is that right now we're checking if the scope from the JWT claims contains a certain value and then checking that value. Instead, it would be better to use the expectedScope String instead. It should be possible to do that by passing the expectedScope to getCallbackHandler instead of passing the boolean value.

http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java

fjuma

Thanks very much @PrarthonaPaul!

PrarthonaPaul changed the title ~~[WFLY-16532] Add ability to configure additional scope for authentica…~~ [ELY-2574] Add ability to configure additional scope for authentication request Jun 28, 2023

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java Outdated Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcClientConfigurationBuilder.java Outdated Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java Outdated Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved

fjuma reviewed Jun 28, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

PrarthonaPaul commented Jul 6, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

PrarthonaPaul mentioned this pull request Jul 13, 2023

[WFLY-16532][Preview] add ability to configure additional scope for authentication request wildfly/wildfly#17018

Closed

fjuma reviewed Jul 18, 2023

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientSecretCredentialsProvider.java Outdated Show resolved Hide resolved

fjuma reviewed Jul 18, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcBaseTest.java Outdated Show resolved Hide resolved

fjuma requested changes Jul 18, 2023

View reviewed changes

fjuma reviewed Jul 19, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

fjuma reviewed Jul 19, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

PrarthonaPaul force-pushed the WFLY-16532 branch from 297ac11 to d0902d5 Compare July 19, 2023 17:44

fjuma reviewed Jul 19, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

fjuma reviewed Jul 19, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

fjuma reviewed Jul 19, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Show resolved Hide resolved

fjuma reviewed Jul 19, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

PrarthonaPaul force-pushed the WFLY-16532 branch from d0902d5 to c9a589b Compare July 20, 2023 14:06

fjuma requested changes Jul 26, 2023

View reviewed changes

fjuma reviewed Jul 26, 2023

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

PrarthonaPaul force-pushed the WFLY-16532 branch from 91442a1 to 620fd0d Compare March 19, 2024 16:43

fjuma requested changes Mar 19, 2024

View reviewed changes

fjuma reviewed Mar 19, 2024

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/ElytronMessages.java Outdated Show resolved Hide resolved

PrarthonaPaul force-pushed the WFLY-16532 branch from 620fd0d to 70e83ee Compare March 20, 2024 14:21

fjuma reviewed Mar 20, 2024

View reviewed changes

fjuma mentioned this pull request Mar 20, 2024

[WFLY-17143] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters wildfly/wildfly#17219

Open

fjuma reviewed Mar 20, 2024

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 20, 2024

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 20, 2024

View reviewed changes

http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 20, 2024

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/OidcTest.java Outdated Show resolved Hide resolved

fjuma reviewed Mar 20, 2024

View reviewed changes

PrarthonaPaul force-pushed the WFLY-16532 branch 5 times, most recently from d582ef7 to a3fa083 Compare March 21, 2024 18:10

fjuma reviewed Mar 21, 2024

View reviewed changes

http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java Outdated Show resolved Hide resolved

PrarthonaPaul force-pushed the WFLY-16532 branch 2 times, most recently from 7eded8d to 6353159 Compare March 21, 2024 20:07

[ELY-2574] Add the ability to configure scopes with elytron-oidc-client

fc56044

PrarthonaPaul force-pushed the WFLY-16532 branch from 6353159 to fc56044 Compare March 21, 2024 20:09

fjuma approved these changes Mar 21, 2024

View reviewed changes

fjuma added the +1 FJ label Mar 21, 2024

fjuma requested a review from darranl March 22, 2024 13:53

darranl approved these changes Mar 22, 2024

View reviewed changes

darranl added the +1 DAL label Mar 22, 2024

fjuma merged commit 44342cb into wildfly-security:2.x Mar 22, 2024
3 checks passed

fjuma mentioned this pull request Apr 3, 2024

[WFLY-16532][Preview] add ability to configure additional scope for authentication request wildfly/wildfly#17785

Closed

PrarthonaPaul mentioned this pull request Apr 3, 2024

WFLY-16532 [Preview] elytron-oidc-client: add ability to configure additional scope for authentication request wildfly/wildfly#17790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ELY-2574] Add ability to configure additional scope for authentication request #1925

[ELY-2574] Add ability to configure additional scope for authentication request #1925

PrarthonaPaul commented Jun 28, 2023 •

edited

fjuma commented Jun 28, 2023

fjuma commented Jun 28, 2023 •

edited

PrarthonaPaul commented Jun 28, 2023

fjuma commented Jun 28, 2023 •

edited

PrarthonaPaul commented Jun 28, 2023

fjuma commented Jul 18, 2023

fjuma left a comment

fjuma Mar 19, 2024

PrarthonaPaul Mar 20, 2024

fjuma commented Mar 20, 2024

fjuma Mar 20, 2024

darranl Mar 20, 2024

fjuma Mar 20, 2024

darranl Mar 20, 2024

fjuma Mar 20, 2024

PrarthonaPaul Mar 21, 2024

fjuma Mar 20, 2024

fjuma Mar 20, 2024 •

edited

PrarthonaPaul Mar 21, 2024

PrarthonaPaul Mar 21, 2024

fjuma Mar 21, 2024 •

edited

fjuma Mar 21, 2024

PrarthonaPaul Mar 21, 2024

PrarthonaPaul Mar 21, 2024

fjuma Mar 21, 2024

PrarthonaPaul Mar 21, 2024

fjuma left a comment

[ELY-2574] Add ability to configure additional scope for authentication request #1925

[ELY-2574] Add ability to configure additional scope for authentication request #1925

Conversation

PrarthonaPaul commented Jun 28, 2023 • edited

fjuma commented Jun 28, 2023

fjuma commented Jun 28, 2023 • edited

PrarthonaPaul commented Jun 28, 2023

fjuma commented Jun 28, 2023 • edited

PrarthonaPaul commented Jun 28, 2023

fjuma commented Jul 18, 2023

fjuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjuma left a comment

Choose a reason for hiding this comment

PrarthonaPaul commented Jun 28, 2023 •

edited

fjuma commented Jun 28, 2023 •

edited

fjuma commented Jun 28, 2023 •

edited

fjuma Mar 20, 2024 •

edited

fjuma Mar 21, 2024 •

edited