[WFLY-16532] Preview - Add the ability to configure additional scope values for an authentication request #527
Conversation
|
@PrarthonaPaul Thanks for the PR! It's good to add the link to the WFLY issue in the PR description. |
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
1f68579
to
71134be
Compare
Oops, sorry. It is added now. Thanks! |
| @@ -0,0 +1,129 @@ | |||
| == Adding the ability to configure additional scope for authentication request | |||
There was a problem hiding this comment.
s/authentication request/an authentication request
| and allows a user to login to a web application using credentials established | ||
| by an OpenID provider. | ||
| Currently, when sending an authentication request to the OpenID provider, one | ||
| of the required parameters with the authentication flow is "scope". However, for |
There was a problem hiding this comment.
s/authentication flow/authorization code flow
| by an OpenID provider. | ||
| Currently, when sending an authentication request to the OpenID provider, one | ||
| of the required parameters with the authentication flow is "scope". However, for | ||
| now, that value is hardcoded as just "openid". |
There was a problem hiding this comment.
s/that value is.../the Elytron OIDC HTTP authentication mechanism hardcodes this value to just "openid".
| of the required parameters with the authentication flow is "scope". However, for | ||
| now, that value is hardcoded as just "openid". | ||
|
|
||
| The specifications indicate that there are other scope values which may be included in |
There was a problem hiding this comment.
s/specification/OpenID Connect specification
Would be good to add the link to the relevant section as well:
https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
There was a problem hiding this comment.
Would also be good to check if the specification mentions anything about the format of the scope values.
There was a problem hiding this comment.
Would also be good to check if the specification mentions anything about the values that are allowed for the scope values.
| now, that value is hardcoded as just "openid". | ||
|
|
||
| The specifications indicate that there are other scope values which may be included in | ||
| the authentication request. This new feature adds the ability to configure the `scope` attribute |
| /subsystem=elytron-oidc-client=my-oidc-client:write-attribute(name=scope, value=openid) | ||
| ``` | ||
|
|
||
| * It must also be configured using the `oidc.json` file as follows: |
There was a problem hiding this comment.
Here, it would be good to make it clear that it should also be possible to configure the scope attribute via the deployment configuration as well.
|
|
||
| * It must also be configured using the `oidc.json` file as follows: | ||
| ``` | ||
| "scope" : "<clinet id>%20offline_access%20openid" |
There was a problem hiding this comment.
Would be good to look at how a user would specify a list of scope values. Looking at other examples in the elytron subsystem, it's common to specify a space separated list. It would be good to mention the format that will be expected in the subsystem and deployment configuration.
The above example should then be updated accordingly. The %20 there is necessary for the authentication request itself (it encodes a space character) but we don't need the user to have to worry about that themselves.
That's something the OIDC authentication mechanism should handle when forming the authentication request using the specified values.
|
|
||
| == Test Plan | ||
|
|
||
| * Wildfly Elytron test suit: Test cases implemented for functionality. |
There was a problem hiding this comment.
s/Wildfly/WildFly
s/test suit/test suite
It would be good to provide some additional details here.
|
|
||
| * Wildfly Elytron test suit: Test cases implemented for functionality. | ||
|
|
||
| * WildFly test suite: Ensuring the correct scope if chosen and used when the `scope` attribute is |
There was a problem hiding this comment.
Would be good to mention that tests will be added for the case where the scope is configured in the subsystem configuration and for the case where it's configured in the deployment configuration.
|
|
||
| == Community Documentation | ||
|
|
||
| Documentation will be added to https://github.com/wildfly/wildfly/blob/main/docs/src/main/asciidoc/_admin-guide/subsystem-configuration/Elytron_OIDC_Client.adoc[Elytron OpenID Connect Client Subsystem Configuration]. |
There was a problem hiding this comment.
s/Documentation/Documentation for the new scope option
fjuma
changed the title
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
71134be
to
03f4124
Compare
| === Testing By | ||
| // Put an x in the relevant field to indicate if testing will be done by Engineering or QE. | ||
| // Discuss with QE during the Kickoff state to decide this | ||
| * [ ] Engineering |
There was a problem hiding this comment.
We can select Engineering here.
| * It must be possible to configure this attribute using the following command: | ||
|
|
||
| ``` | ||
| /subsystem=elytron-oidc-client/secure-deployment=my-secure-deployment:write-attribute(name=scope, value="openid") |
There was a problem hiding this comment.
It would be good to indicate that this is example configuration. It would also be good to update the example value. Since this attribute is meant to indicate additional scope values, they wouldn't need to specify "openid" here.
| * It must also be configured by specifying it in the deployment. This can be done using the `oidc.json` file inside the `WEB-INF` directory as follows: | ||
|
|
||
| ``` | ||
| "scope" : "myClient, myclient@redhat.com, offline_access, openid" |
There was a problem hiding this comment.
Would be good to indicate that this is an example.
If a comma separated list is being used in the JSON configuration, it would probably make sense to use a comma separated list in the subsystem configuration as well.
There was a problem hiding this comment.
In terms of subsystem configuration, were you thinking of a ListAttributeDefinition
(ex. value=[openid,offline_access]) or something else?
| "scope" : "myClient, myclient@redhat.com, offline_access, openid" | ||
| ``` | ||
|
|
||
| * The OpenID Connect Specifications contain more details on https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims[optional scope values] and https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess[using scope values to requst Offline Access.] |
There was a problem hiding this comment.
Would be good to also include some of the details here (e.g., what format is expected for the scope values passed in the Authentication Request).
There was a problem hiding this comment.
Applied changes up to this comment.
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
03f4124
to
086bbf2
Compare
| * A new attribute named `scope` will be added to the `secure-deployment` resource under the `elytron-oidc-client` subsystem, which will be used | ||
| to specify additional scope values. These values will be used by the Elytron HTTP OIDC authentication mechanism. | ||
|
|
||
| * It must be possible to configure this attribute using cli commands. For example: | ||
|
|
||
| ``` | ||
| /subsystem=elytron-oidc-client/secure-deployment=my-secure-deployment:write-attribute(name=scope, value="openid, offline_access") | ||
| ``` |
There was a problem hiding this comment.
After looking at this comment, I wonder if it would make more sense to name the attribute something like additional_scopes, to make it clear that openid doesn't need to be specified and is included by default.
There was a problem hiding this comment.
That's a good point. I can change it to that. Thanks!
As for the other comment about ListAttributeDefinition, I was thinking something similar to how elytron-oidc-client/secure-deployment/cors-allowed-headers is configured.
I saw on Elytron_OIDC_client doc that this was also a list of attribute values.
And on the cli commands I was able to configure it as follows:
/subsystem=elytron-oidc-client/secure-deployment=my-secure-deployment:write-attribute(name=cors-allowed-headers, value="WWW-Authenticate, My-custom-exposed-Header")
There was a problem hiding this comment.
After looking at this comment, I wonder if it would make more sense to name the attribute something like
additional_scopes, to make it clear thatopeniddoesn't need to be specified and is included by default.
I think we should think some more about this. Looking at Quarkus, they use "scopes" so might make sense for us to use that as well. I'd need to check whether "openid" is assumed to be included in this list.
There was a problem hiding this comment.
Okay, I will hold off on the change and keep it as scope for now.
| * It must also be configured by specifying it in the deployment. This can be done using the `oidc.json` file inside the `WEB-INF` directory as follows: | ||
|
|
||
| ``` | ||
| "scope" : "myClient, myclient@redhat.com, offline_access, openid" |
There was a problem hiding this comment.
In terms of subsystem configuration, were you thinking of a ListAttributeDefinition
(ex. value=[openid,offline_access]) or something else?
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
086bbf2
to
5622c0a
Compare
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
5622c0a
to
1fbf386
Compare
| by an OpenID provider. | ||
| Currently, when sending an authentication request to the OpenID provider, one | ||
| of the required parameters with the authentication code flow is "scope". However, for | ||
| now, the Elytron OIDC HTTP authentication mechanism hardcodes this value to just "openid". |
There was a problem hiding this comment.
s/to just "openid"/to just "openid" and only allows additional scopes to be specified via a "scope" query parameter.
|
|
||
| === Hard Requirements | ||
|
|
||
| * A new attribute named `scope` will be added to the `secure-deployment` resource under the `elytron-oidc-client` subsystem, which will be used |
There was a problem hiding this comment.
We should mention secure-server here too.
| * It must be possible to configure this attribute using cli commands. For example: | ||
|
|
||
| ``` | ||
| /subsystem=elytron-oidc-client/secure-deployment=my-secure-deployment:write-attribute(name=scope, value="openid offline_access") |
There was a problem hiding this comment.
I think for the examples here and below, we can remove "openid". We can mention that this will automatically be added when forming the authentication request if not present.
|
|
||
| * The OpenID Connect Specifications contain more details on https://openid.net/specs/openid-connect-core-1_0.html#ScopeClaims[optional scope values] and https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess[using scope values to requst Offline Access.] | ||
|
|
||
| * Scope values are to be saved as a list of space separated values inside quotes as seen in the examples above. |
There was a problem hiding this comment.
s/Scope values are to be saved as a list of space separated values inside quotes as seen in the examples above./The scope value must be a String of space separated values as seen in the examples above.
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
1fbf386
to
e829e18
Compare
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
e829e18
to
18a785c
Compare
PrarthonaPaul
changed the title
… values for an authentication request
PrarthonaPaul
force-pushed
the
WFLY-16532
branch
from
18a785c
to
0379617
Compare
PrarthonaPaul
changed the title
https://issues.redhat.com/browse/WFLY-16532