Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for multiple custom claim paths to find roles in oidc token #23139

Merged

Conversation

Markus-Schwer
Copy link
Contributor

This PR is for #23138 and contains a small code change to allow wildcards in the quarkus.oidc.roles.role-claim-path config property. Additionally, this changes the default value for the property from resource_access/client_id/roles to resource_access/*/roles when keycloak is used.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Markus-Schwer Thanks for opening the issue and proceeding with the PR, it is appreciated.

I'd however prefer your first idea, treat the claim path as a list and run findClaimWithRoles for every configured path in a loop.
The reason I prefer it is because getting all the roles associated with various clients should not be a default behavior for Keycloak as it can cause unexpected side-effects with the role based access control decisions (false positives/denials).
It would be correct IMHO to require users to list individual paths in such cases:
resource_access/a/roles,resource_access/b/roles - a bit verbose but also very specific, no unexpected side-effects will happen

Thanks

@Markus-Schwer
Copy link
Contributor Author

Hi @sberyozkin Thanks for the fast response and the feedback!
I will update the PR with the changes you suggested.

@Markus-Schwer Markus-Schwer force-pushed the wildcard-oidc-roles-claim-path branch from 1f33359 to c0c4d48 Compare January 24, 2022 19:11
@sberyozkin
Copy link
Member

@Markus-Schwer Thanks, LGTM, left one 1 minor comment, please also squash the commits

@sberyozkin sberyozkin changed the title Support for wildcards in OIDC roles-claim-path Support for multiple custom claim paths to find roles in oidc token Jan 24, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 25, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 3518656

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
JVM Tests - JDK 17 Build Failures Logs Raw logs
Native Tests - Security2 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/oidc/deployment 
! Skipped: extensions/keycloak-authorization/deployment extensions/oidc-client-filter/deployment extensions/oidc-client-reactive-filter/deployment and 13 more

📦 extensions/oidc/deployment

io.quarkus.oidc.test.CodeFlowDevModeTestCase.testAccessAndRefreshTokenInjectionDevMode line 48 - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8080/protected/tenant/tenant-config-resolver?state=b5a4cdd1-7b0a-4764-8716-d50e37b4da9c&session_state=693e3d6a-7c9d-4db3-b54c-e537f4fbdcb0&code=c80f0ed2-8f7c-4d7a-8bd5-17576320a7e6.693e3d6a-7c9d-4db3-b54c-e537f4fbdcb0.372aa86c-3bc7-45cd-9d58-36f7bfdb8718
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

⚙️ JVM Tests - JDK 11 Windows #

- Failing: integration-tests/oidc-wiremock 

📦 integration-tests/oidc-wiremock

io.quarkus.it.keycloak.CodeFlowAuthorizationTest.testCodeFlowUserInfo line 65 - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/code-flow-user-info-dynamic-github?state=6cc24ee2-ba2e-49e4-b491-0cb924dd0b6a&code=58af24f2-9093-4674-a431-4a9d66be719c.50437113-cd78-48a2-838e-b936fe458c5d.0ac5df91-e044-4051-bd03-106a3a5fb9cc
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/oidc/deployment 
! Skipped: extensions/keycloak-authorization/deployment extensions/oidc-client-filter/deployment extensions/oidc-client-reactive-filter/deployment and 13 more

📦 extensions/oidc/deployment

io.quarkus.oidc.test.CodeFlowDevModeTestCase.testAccessAndRefreshTokenInjectionDevMode line 48 - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8080/protected/tenant/tenant-config-resolver?state=eaa86614-cd8c-4245-bdd1-eb080682da88&session_state=3034fcf1-3642-40f8-a97d-f20408dbf2e6&code=95d28d11-4ec6-40e4-b879-41b6dcae8445.3034fcf1-3642-40f8-a97d-f20408dbf2e6.685a6a04-60a3-4a7f-825a-d133fee305ba
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

⚙️ Native Tests - Security2 #

- Failing: integration-tests/oidc-tenancy integration-tests/oidc-wiremock 

📦 integration-tests/oidc-tenancy

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testResolveTenantIdentifierWebAppDynamic - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/tenant/tenant-web-app-dynamic/api/user/webapp?state=b2c16dee-729b-4f13-97f9-345015f95178&session_state=0c382530-cb34-48ac-8514-0a2e24796e8c&code=263def93-1048-40ae-a906-288a61c9fe7b.0c382530-cb34-48ac-8514-0a2e24796e8c.c8519807-dbec-4edc-af63-f5192a1827af
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testSimpleOidcNoDiscovery - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <401>.

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testJwtTokenIntrospectionOnlyAndUserInfoCache - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <401>.

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testSimpleOidcJwtWithJwkRefresh - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <401>.

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testJwtTokenIntrospectionOnlyAndUserInfo - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <401>.

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testJwtTokenIntrospectionDisallowed - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <401>.

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testResolveTenantConfig - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Expected status code <200> but was <401>.

📦 integration-tests/oidc-wiremock

io.quarkus.it.keycloak.CodeFlowAuthorizationInGraalITCase.testCodeFlowUserInfo - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/code-flow-user-info-dynamic-github?state=d7c85d6e-83a5-48cf-affd-2e3815fd90e7&code=58af24f2-9093-4674-a431-4a9d66be719c.50437113-cd78-48a2-838e-b936fe458c5d.0ac5df91-e044-4051-bd03-106a3a5fb9cc
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:701)
	at com.gargoylesoftware.htmlunit.WebClient.loadDownloadedResponses(WebClient.java:2452)

@Markus-Schwer Markus-Schwer force-pushed the wildcard-oidc-roles-claim-path branch from 3518656 to 7870359 Compare January 25, 2022 07:00
@sberyozkin sberyozkin merged commit f8346d7 into quarkusio:main Jan 25, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Jan 25, 2022
@Markus-Schwer Markus-Schwer deleted the wildcard-oidc-roles-claim-path branch January 25, 2022 14:13
@gsmet gsmet modified the milestones: 2.8 - main, 2.7.0.Final Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants