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

Check semicolon during the OIDC scope to permission conversion #36361

Conversation

sberyozkin
Copy link
Member

Fixes #36313.

Supports #35931.

OIDC scope claim is automatically converted to one or more StringPermissions.
Currently, if the scope is named read:data then the permission name will become read:data, but StringPermission which will be created from @PermissionAllowed("read:data") will have its name set to read, and its actions set to data which makes the comparison produce a false result.

This simple PR checks the semicolon during the OIDC scope to Permission conversion, if it is there, assumes that the first part is the name, the second part is the actions.
It does nothing for cases like :a or b: as it is not a conversion's concern if such a value makes sense or what it may mean - it should never happen in any case

Copy link
Member

@michalvavrik michalvavrik left a comment

Choose a reason for hiding this comment

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

Thanks! lgtm

@gastaldi gastaldi added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 9, 2023
@sberyozkin
Copy link
Member Author

Thanks @gastaldi @michalvavrik

@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 9, 2023

@michalvavrik JDK 11 failure is an oidc-db-state-token-manager test failure, related to the server not available on time, I believe at least 3 tests are there, one for different type of the reactive database client, I propose to limit it to 1 only to minimize random test resource related failures, please consider it in your next PR related to that extension, that extension can't control the portability aspect.

You added an Oracle DB test in the integration-tests so in deployment we can just get one of other more mainstream clients tested

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 9, 2023

Failing Jobs - Building ded3836

Status Name Step Failures Logs Raw logs Build scan
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 20

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/oidc-db-token-state-manager/deployment 
! Skipped: integration-tests/smallrye-jwt-oidc-webapp 

📦 extensions/oidc-db-token-state-manager/deployment

io.quarkus.oidc.db.token.state.manager.MsSqlDbTokenStateManagerTest.testCodeFlow - More details - Source on GitHub

com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 401 Unauthorized for http://localhost:8081/protected
	at com.gargoylesoftware.htmlunit.WebClient.throwFailingHttpStatusCodeExceptionIfNecessary(WebClient.java:744)
	at com.gargoylesoftware.htmlunit.WebClient.getPage(WebClient.java:499)

@sberyozkin sberyozkin merged commit 2700890 into quarkusio:main Oct 9, 2023
21 of 22 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Oct 9, 2023
@sberyozkin sberyozkin deleted the fix_oidc_scope_to_permission_conversion branch October 9, 2023 16:27
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 9, 2023
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.

OIDC scope to permission conversion does not initialize StringPermission actions
3 participants