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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 4 commits into from

Conversation

PrarthonaPaul
Copy link
Contributor

@PrarthonaPaul PrarthonaPaul commented Sep 26, 2023 •
edited by wildfly-bot bot

@wildfly-ci
Copy link

Hello, PrarthonaPaul. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 2 times, most recently from 6138fc5 to e7ba60f Compare September 28, 2023 13:56
@PrarthonaPaul PrarthonaPaul marked this pull request as ready for review September 28, 2023 13:57
@wildfly-bot wildfly-bot bot requested a review from fjuma September 28, 2023 13:57
@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 3 times, most recently from e57cd77 to d1b0fb5 Compare October 3, 2023 20:07
Skyllarr
Skyllarr reviewed Oct 6, 2023
View reviewed changes
Copy link
Contributor

@Skyllarr Skyllarr left a comment

Choose a reason for hiding this comment

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

@PrarthonaPaul Looks good! I just added some minor comments

elytron-oidc-client/src/main/resources/schema/wildfly-elytron-oidc-client_3_0.xsd Outdated
<xs:annotation>
<xs:documentation>
<![CDATA[
The password for the client key. This is required if 'client-keystore' or 'client-keystore-file has been specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul Total minor, missing ' at the end of 'client-keystore-file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!

elytron-oidc-client/src/main/resources/schema/wildfly-elytron-oidc-client_3_0.xsd Outdated
<xs:documentation>
<![CDATA[
The content encryption algorithm used to encrypt the request object. The default value of this parameter
is null, which indicates that the request object will not be encrypted. The "request-object-encryption-algorithm"
Copy link
Contributor

Choose a reason for hiding this comment

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

Total minor nitpick, maybe we should use the ' everywhere or " everywhere when referencing other attributes. Above for 'client-keystore' and 'client-keystore-file' you used ' and here for request-object-encryption-algorithm you used the "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Changed it all to `

...dc-client/src/main/java/org/wildfly/extension/elytron/oidc/ProviderAttributeDefinitions.java
new SimpleAttributeDefinitionBuilder(ElytronOidcDescriptionConstants.REQUEST_OBJECT_CONTENT_ENCRYPTION_ALGORITHM, ModelType.STRING, true)
.setAllowExpression(true)
.setValidator(new StringLengthValidator(1, Integer.MAX_VALUE, true, true))
.build();
Copy link
Contributor

Choose a reason for hiding this comment

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

@PrarthonaPaul Just a minor, if the REQUEST_OBJECT_CONTENT_ENCRYPTION_ALGORITHM attribute requires configuration of REQUEST_OBJECT_ENCRYPTION_ALGORITHM attribute as well, you might be able to use this here also:

.setRequires(ElytronOidcDescriptionConstants.REQUEST_OBJECT_ENCRYPTION_ALGORITHM)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!
Added it to both REQUEST_OBJECT_ENCRYPTION_ALGORITHM and REQUEST_OBJECT_CONTENT_ENCRYPTION_ALGORITHM

...dc-client/src/main/java/org/wildfly/extension/elytron/oidc/ProviderAttributeDefinitions.java Outdated
new SimpleAttributeDefinitionBuilder(ElytronOidcDescriptionConstants.AUTHENTICATION_REQUEST_FORMAT, ModelType.STRING, true)
.setAllowExpression(true)
.setDefaultValue(new ModelNode("oauth2"))
.setAllowedValues("oauth2", "request", "request_uri")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor, we should make constants out of oauth2, request and request_uri . You can place them to ElytronOidcDescriptionConstants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed!
Thank you for your feedback

@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 2 times, most recently from c7a0033 to 0dd0234 Compare October 6, 2023 20:29
@bstansberry bstansberry added Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements labels Oct 9, 2023
@bstansberry
Copy link
Contributor

/ok-to-test

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Oct 9, 2023
fjuma
fjuma reviewed Oct 30, 2023
View reviewed changes
...dc-client/src/main/java/org/wildfly/extension/elytron/oidc/ProviderAttributeDefinitions.java Outdated
protected static final SimpleAttributeDefinition REQUEST_OBJECT_SIGNING_ALGORITHM =
new SimpleAttributeDefinitionBuilder(ElytronOidcDescriptionConstants.REQUEST_OBJECT_SIGNING_ALGORITHM, ModelType.STRING, true)
.setAllowExpression(true)
.setDefaultValue(new ModelNode("none")) // plaintext jwt to be sent
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the Elytron Oidc constant instead of the string "none".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I dont have them in OIdc anymore, can I use the AlgorithmIdentifier.None?

@wildfly-bot wildfly-bot bot requested a review from fjuma November 1, 2023 19:25
@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 3 times, most recently from 054e8f5 to a99104c Compare November 8, 2023 14:26
@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 3 times, most recently from 9a3137f to 05c177e Compare February 27, 2024 17:46
@PrarthonaPaul PrarthonaPaul marked this pull request as draft February 27, 2024 17:55
@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 2 times, most recently from 440a11a to 9145b14 Compare March 19, 2024 17:03
@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 2 times, most recently from 04fa03f to b18b922 Compare March 19, 2024 17:17
@PrarthonaPaul PrarthonaPaul marked this pull request as ready for review March 19, 2024 17:20
fjuma
fjuma requested changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@fjuma fjuma left a comment •
edited

Choose a reason for hiding this comment

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

@PrarthonaPaul I've just added a comment on the Elytron PR about an alternative approach we could take for handling unsupported attributes in the oidc.json deployment descriptor file. I think we can handle that from OidcActivationProcessor instead of adding new logic to Elytron to handle unsupported attributes:

wildfly-security/wildfly-elytron#1925 (comment)

@wildfly-bot wildfly-bot bot requested a review from fjuma April 15, 2024 14:28
@PrarthonaPaul PrarthonaPaul force-pushed the WFLY-17143 branch 3 times, most recently from e1eea7e to c0526b9 Compare April 17, 2024 15:43
fjuma
fjuma reviewed May 8, 2024
View reviewed changes
...client/src/main/java/org/wildfly/extension/elytron/oidc/ElytronOidcClientSubsystemModel.java Outdated
@@ -17,8 +17,9 @@ enum ElytronOidcClientSubsystemModel implements SubsystemModel {
VERSION_1_0_0(1, 0, 0),
VERSION_2_0_0(2, 0, 0),
VERSION_3_0_0(3, 0, 0), // WildFly 32.0-present
VERSION_4_0_0(4, 0, 0), // WildFly 34.0-present
Copy link
Contributor

Choose a reason for hiding this comment

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

WildFly 33

@wildfly-bot wildfly-bot bot requested a review from fjuma May 9, 2024 14:41
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

@fjuma fjuma Awaiting requested review from fjuma

Assignees
No one assigned
Labels
deps-ok Dependencies have been checked, and there are no significant changes Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements
Projects
None yet
Milestone
No milestone
5 participants
@PrarthonaPaul @wildfly-ci @bstansberry @fjuma @Skyllarr