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.

Sign up for GitHub

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

Jump to bottom

[ELY-2584] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters #1984

Merged
merged 1 commit into from
Jun 26, 2024
Merged

[ELY-2584] Add the ability to specify that the OIDC Authentication Request should include request and request_uri parameters #1984

merged 1 commit into from
Jun 26, 2024

Conversation

PrarthonaPaul
Copy link
Contributor

@PrarthonaPaul PrarthonaPaul commented Sep 26, 2023 •
edited
Loading

@PrarthonaPaul PrarthonaPaul mentioned this pull request Sep 26, 2023
Merged
@PrarthonaPaul PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from 93dc491 to 3ce624a Compare September 28, 2023 16:02
Skyllarr
Skyllarr reviewed Oct 2, 2023
View reviewed changes
Copy link
Contributor

@Skyllarr Skyllarr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@PrarthonaPaul Looks good! I've just had a quick look at it and added some minor comments, I'll look at it more and its affiliated wildfly PR later this week

http/oidc/pom.xml Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientCredentialsProvider.java Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved
http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java Outdated Show resolved Hide resolved
PrarthonaPaul
PrarthonaPaul commented Oct 5, 2023
View reviewed changes
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved
http/oidc/pom.xml Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/JWTClientCredentialsProvider.java Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved
http/oidc/src/test/java/org/wildfly/security/http/oidc/KeycloakConfiguration.java Outdated Show resolved Hide resolved
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcRequestAuthenticator.java Outdated Show resolved Hide resolved
@PrarthonaPaul PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from fd7a498 to 0c6f43d Compare June 21, 2024 14:44
darranl
darranl reviewed Jun 21, 2024
View reviewed changes
http/oidc/src/main/java/org/wildfly/security/http/oidc/OidcJsonConfiguration.java
@@ -38,15 +38,18 @@
"resource", "public-client", "credentials",
"use-resource-role-mappings", "use-realm-role-mappings",
"enable-cors", "cors-max-age", "cors-allowed-methods", "cors-exposed-headers",
"expose-token", "bearer-only", "autodetect-bearer-only",
"connection-pool-size",
"expose-token", "bearer-only", "autodetect-bearer-only", "connection-pool-size",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily as part of this PR but we seem to have String constants being repeated - should these reference common constants defined somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe some (if not all) are constants inside Oidc.java class. So, we might be able to use those instead.

@darranl
Copy link
Contributor

darranl commented Jun 21, 2024

Generally the PR looks good but one piece missing for me is that it is proposed that this feature comes to WildFly at the Preview stability level but when it comes to the JSON handling that appears that it will be active at all stability levels?

@PrarthonaPaul
Copy link
Contributor Author

Generally the PR looks good but one piece missing for me is that it is proposed that this feature comes to WildFly at the Preview stability level but when it comes to the JSON handling that appears that it will be active at all stability levels?

No, The Json will also fail if the stability level is not set correctly.
This is handled within the WildFly project using a list called NON_DEFAULT_ATTRIBUTES, which stores all the non_default attributes and if any of them are specified while at the incorrect stability level, then the configuration fails.
https://github.com/wildfly/wildfly/pull/17219/files#diff-86efa3b1346d91d796976c39723408c6e72585a020050e4ae8b96e13fb1787ecR193-R201
This was added in WildFly 32 with the scope attribute.

@PrarthonaPaul PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from 7102a14 to 044a01f Compare June 24, 2024 17:18
@PrarthonaPaul PrarthonaPaul force-pushed the ELY-2584 branch 2 times, most recently from 016e793 to 25d6a18 Compare June 25, 2024 18:53
[ELY-2584] Add the ability to specify that the OIDC Authentication Re…
8b0c237 
…quest should include request and request_uri parameters.
fjuma
fjuma approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Adding my approval on this one. The majority of the comments have been addressed and I don't think there's anything remaining that should block merging this one.

@PrarthonaPaul Would you be able to create issues to track any remaining tests that we're planning on adding or any other comments that haven't been addressed yet?

Thanks very much for all the updates on this one!

@PrarthonaPaul
Copy link
Contributor Author

PrarthonaPaul commented Jun 26, 2024 •
edited
Loading

Thanks @fjuma @rsearls
I have created this issue (https://issues.redhat.com/browse/ELY-2776) to track the additional tests and https://issues.redhat.com/browse/ELY-2778 to track the changes for the string literals inside OidcJsonConfiguration.

@fjuma fjuma merged commit 1c6246a into wildfly-security:2.x Jun 26, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Skyllarr Skyllarr Skyllarr left review comments

@darranl darranl darranl left review comments

@fjuma fjuma fjuma approved these changes

@rsearls rsearls rsearls approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PrarthonaPaul @rsearls @darranl @fjuma @Skyllarr