Skip to content

Conversation

brosenberg42
Copy link
Member

@brosenberg42 brosenberg42 commented Jul 24, 2023

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 32 files at r1.
Reviewable status: 1 of 32 files reviewed, 10 unresolved discussions (waiting on @brosenberg42)


docs/docs/OpenID-Connect-Guide.md line 1 at r1 (raw file):

**NOTICE:** This software (or technical data) was produced for the U.S. Government under contract,

Overall, this was very helpful. Good job.

Please explain what needs to be done to enable components to register with the WFM. I was not expecting to encounter:

openmpf-ocv-face-detection-1  | RuntimeError: The following error occurred while trying to register component: HTTP Error 401: : b''
openmpf-ocv-face-detection-1  | The WFM_USER and WFM_PASSWORD environment variables need to be changed.

component-executor.py is still using basic auth.


docs/docs/OpenID-Connect-Guide.md line 15 at r1 (raw file):

secret should be provided. Those values should be set in the `OIDC_CLIENT_ID` and
`OIDC_CLIENT_SECRET` environment variables. During the registration process the provider will
likely request a redirect URI. The redirect URI should be set to the base URI for Workflow Manger

Check spelling: Manger


docs/docs/OpenID-Connect-Guide.md line 26 at r1 (raw file):

claim with a specific value to determine if the user is authorized to access Workflow Manager and
with what role. The `OIDC_USER_CLAIM_NAME` and `OIDC_ADMIN_CLAIM_NAME` environment variables
specify the name of claim that must be present. The `OIDC_USER_CLAIM_VALUE` and

Check grammar: name of "the" claim


docs/docs/OpenID-Connect-Guide.md line 54 at r1 (raw file):

- `OIDC_ADMIN_CLAIM_VALUE` (Optional): Specifies the required value of the claim specified in
    `OIDC_ADMIN_CLAIM_NAME`. If the claim is a list, only one of the values in the list must match.
- `OIDC_SCOPES` (Optional): A comma-separated list the scopes to be requested from the OIDC

Check grammar: A comma-separated list "of" the scopes to be


docs/docs/OpenID-Connect-Guide.md line 62 at r1 (raw file):

- `OIDC_REDIRECT_URI` (Optional): Specifies the URL the user's browser will be redirected to after
    logging in to the OIDC provider. If provided, the URL must end in `/login/oauth2/code/provider`.
    This would generally be used when the host name Workflow Manager uses to connect to the OIDC

Check grammar: when the host name "that" Workflow Manager


docs/docs/OpenID-Connect-Guide.md line 95 at r1 (raw file):

    `http://<docker-gateway-ip>:9090/realms/<realm-name>`

5\. Create the client Workflow Manager will use to authenticate users:

Check grammar: Create the client "that" Workflow Manager will use to authenticate users


docs/docs/OpenID-Connect-Guide.md line 97 at r1 (raw file):

5\. Create the client Workflow Manager will use to authenticate users:

- The "Client type" needs to be set to "OpenID Connect".

Missing a step. Instruct the user to select "Clients" on the left and then click the "Create client" button.


docs/docs/OpenID-Connect-Guide.md line 98 at r1 (raw file):

- The "Client type" needs to be set to "OpenID Connect".
- "Client authentication" must be enabled.

Mention each step where each of these settings must be enabled. For example, "Client type" is on the "General Settings step. "Client authentication" is on the "Capability config" step. "Valid redirect URIs" is on the "Login settings" step.


docs/docs/OpenID-Connect-Guide.md line 118 at r1 (raw file):

- In the "Client scopes" menu add a mapper to the "roles" scope.
- Use the "groups" prefined mapper

Check spelling: prefined

Add period at end.


docs/docs/OpenID-Connect-Guide.md line 147 at r1 (raw file):

curl -d grant_type=client_credentials -u '<client-id>:<client-secret>' 'http://<docker-gateway-ip>:9090/realms/<realm-name>/protocol/openid-connect/token'

The response JSON will contain a token in the "access_token" property. That token need to

Check grammar: That token "needs" to "be" included

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 31 of 32 files at r1, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @brosenberg42)

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 27 of 32 files reviewed, 10 unresolved discussions (waiting on @jrobble)


docs/docs/OpenID-Connect-Guide.md line 1 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Overall, this was very helpful. Good job.

Please explain what needs to be done to enable components to register with the WFM. I was not expecting to encounter:

openmpf-ocv-face-detection-1  | RuntimeError: The following error occurred while trying to register component: HTTP Error 401: : b''
openmpf-ocv-face-detection-1  | The WFM_USER and WFM_PASSWORD environment variables need to be changed.

component-executor.py is still using basic auth.

Done.


docs/docs/OpenID-Connect-Guide.md line 15 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check spelling: Manger

Done.


docs/docs/OpenID-Connect-Guide.md line 26 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check grammar: name of "the" claim

Done.


docs/docs/OpenID-Connect-Guide.md line 54 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check grammar: A comma-separated list "of" the scopes to be

Done.


docs/docs/OpenID-Connect-Guide.md line 62 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check grammar: when the host name "that" Workflow Manager

Done.


docs/docs/OpenID-Connect-Guide.md line 95 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check grammar: Create the client "that" Workflow Manager will use to authenticate users

Done.


docs/docs/OpenID-Connect-Guide.md line 97 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Missing a step. Instruct the user to select "Clients" on the left and then click the "Create client" button.

Done.


docs/docs/OpenID-Connect-Guide.md line 98 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Mention each step where each of these settings must be enabled. For example, "Client type" is on the "General Settings step. "Client authentication" is on the "Capability config" step. "Valid redirect URIs" is on the "Login settings" step.

Done.


docs/docs/OpenID-Connect-Guide.md line 118 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check spelling: prefined

Add period at end.

Done.


docs/docs/OpenID-Connect-Guide.md line 147 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Check grammar: That token "needs" to "be" included

Done.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 32 of 32 files at r3, all commit messages.
Reviewable status: 32 of 33 files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


docs/docs/OpenID-Connect-Guide.md line 77 at r3 (raw file):

    authenticate REST clients. The OIDC configuration endpoint must exist at the value of this
    environment variable with `/.well-known/openid-configuration` appended. If both environment
    variables are provided, `OIDC_JWT_ISSUER_URI` will be used.

How can I test JWT_ISSUER_URI?

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 32 of 33 files reviewed, 1 unresolved discussion (waiting on @jrobble)


docs/docs/OpenID-Connect-Guide.md line 77 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

How can I test JWT_ISSUER_URI?

Both environment variables are supported in order to be consistent with WFM. To test setting both, either run two instances of Keycloak on different ports or set up two different realms on one Keycloak instance. Configure one to only allow user log in (step 5) and configure the other to only support REST client authentication (steps 10, 11).

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brosenberg42)


docs/docs/OpenID-Connect-Guide.md line 77 at r3 (raw file):

Previously, brosenberg42 wrote…

Both environment variables are supported in order to be consistent with WFM. To test setting both, either run two instances of Keycloak on different ports or set up two different realms on one Keycloak instance. Configure one to only allow user log in (step 5) and configure the other to only support REST client authentication (steps 10, 11).

Thanks. It works as expected.

It would help to provide an example of the WFM env. var. settings and component env. var. settings for the following scenarios:

  1. Using OIDC_ISSUER_URI for user login and the components.
  2. Using OIDC_ISSUER_URI for user login and OIDC_JWT_ISSUER_URI for the components.

Initially , I was unsure of what needed to be set when I used OIDC_JWT_ISSUER_URI.


docs/docs/OpenID-Connect-Guide.md line 159 at r3 (raw file):

- Capability config:
    - The client needs to have "Client authentication" and "Service accounts roles" enabled.
    - Use the "Service account roles" tab to add the client to one of the roles created in step 5.

I think you mean: "to one of the roles created in step 6."


docs/docs/OpenID-Connect-Guide.md line 169 at r3 (raw file):

- Capability config:
    - The client needs to have "Client authentication" and "Service accounts roles" enabled.
    - Use the "Service account roles" tab to add the client to one of the roles created in step 5.

I think you mean: "to one of the roles created in step 6."

@brosenberg42
Copy link
Member Author

docs/docs/OpenID-Connect-Guide.md line 77 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Thanks. It works as expected.

It would help to provide an example of the WFM env. var. settings and component env. var. settings for the following scenarios:

  1. Using OIDC_ISSUER_URI for user login and the components.
  2. Using OIDC_ISSUER_URI for user login and OIDC_JWT_ISSUER_URI for the components.

Initially , I was unsure of what needed to be set when I used OIDC_JWT_ISSUER_URI.

There is some confusion here. I thought the "If both environment variables are provided, OIDC_JWT_ISSUER_URI will be used." part of the explanation was sufficient, so I told you how to test OIDC_JWT_ISSUER_URI with WFM. To test this on the component set OIDC_ISSUER_URI to a random string and set OIDC_JWT_ISSUER_URI to the actual URI. The component can use either environment variable regardless of which ones WFM uses.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @brosenberg42)


docs/docs/OpenID-Connect-Guide.md line 77 at r3 (raw file):

Previously, brosenberg42 wrote…

There is some confusion here. I thought the "If both environment variables are provided, OIDC_JWT_ISSUER_URI will be used." part of the explanation was sufficient, so I told you how to test OIDC_JWT_ISSUER_URI with WFM. To test this on the component set OIDC_ISSUER_URI to a random string and set OIDC_JWT_ISSUER_URI to the actual URI. The component can use either environment variable regardless of which ones WFM uses.

As discussed, please update to:

`OIDC_JWT_ISSUER_URI` or `OIDC_ISSUER_URI` (Required):`

Also, please add a short section, even just a sentence or two, in ## Configuration explaining why a user would use the OIDC_JWT_ISSUER_URI property and how it can be used in conjunction with OIDC_ISSUER_URI.

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jrobble)


docs/docs/OpenID-Connect-Guide.md line 77 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

As discussed, please update to:

`OIDC_JWT_ISSUER_URI` or `OIDC_ISSUER_URI` (Required):`

Also, please add a short section, even just a sentence or two, in ## Configuration explaining why a user would use the OIDC_JWT_ISSUER_URI property and how it can be used in conjunction with OIDC_ISSUER_URI.

Done.


docs/docs/OpenID-Connect-Guide.md line 159 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I think you mean: "to one of the roles created in step 6."

Done.


docs/docs/OpenID-Connect-Guide.md line 169 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I think you mean: "to one of the roles created in step 6."

Done.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


docs/docs/OpenID-Connect-Guide.md line 84 at r4 (raw file):

- `OIDC_CLIENT_ID` (Required): The client ID that the component service will use when registering
    the component with Workflow Manager.
- `OIDC_CLIENT_SECRET` (Required): The client secret that the component service will use when

I think it's worth mentioning that when using OIDC, that the WFM_USER and WFM_PASSWORD env. variables don't need to be set. They will be ignored.

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 29 of 33 files reviewed, 1 unresolved discussion (waiting on @jrobble)


docs/docs/OpenID-Connect-Guide.md line 84 at r4 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I think it's worth mentioning that when using OIDC, that the WFM_USER and WFM_PASSWORD env. variables don't need to be set. They will be ignored.

They were never required because they had default values. The issuer uri variables are what cause WFM_USER and WFM_PASSWORD to be ignored.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

@brosenberg42 brosenberg42 merged commit a75506d into develop Nov 30, 2023
@brosenberg42 brosenberg42 deleted the feat/oidc-support branch November 30, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants