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

OAuth2 doc changes #1504

Merged
merged 20 commits into from
Jan 18, 2023
Merged

OAuth2 doc changes #1504

merged 20 commits into from
Jan 18, 2023

Conversation

MarcialRosales
Copy link
Contributor

@MarcialRosales MarcialRosales commented Oct 20, 2022

Update management ui documentation:

  • added new setting : oauth_initiated_logon_type
  • oauth_client_secret setting is now optional

Update oauth2 documentation:

  • added new setting : preferred_username_claims

@MarcialRosales MarcialRosales changed the title Explain the two logon modes for Management UI OAuth2 doc changes Nov 4, 2022
management.oauth_provider_url = https://my-uaa-server-host:8443/uaa
</pre>

> IMPORTANT: Since RabbitMQ 3.10, RabbitMQ uses `authorization_code` grant type. `implicit` flow has been
deprecated.
> IMPORTANT: management.oauth_client_secret is an optional setting. It is only required when your authorization server requires it
Copy link
Contributor

Choose a reason for hiding this comment

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

Also in the previous sentence, again avoid past tense when writing user docs so..... implicit flow has been
deprecated.....should be.......implicit flow is deprecated.

@@ -352,7 +357,7 @@ endpoints require the token to be passed in the `token` query string parameter.

At a minimum, RabbitMQ requires the the `openid` scope because it uses some claims in the *id token* to determine the username and to display this username on the top right corner of the management UI. The *id token* is returned by the Authorization server if the `openid` scope is included in the authorization request.

RabbitMQ reads the `user_name` claim from the *id_token*. If the token does not contain the `user_name`, RabbitMQ uses the `sub` claim.
RabbitMQ reads the `user_name` claim from the *id_token*. If the token does not contain the `user_name`, RabbitMQ uses the `sub` claim.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I think the change here is just removing a space

@@ -373,6 +378,8 @@ such as:
* <*resource_server_id*>`.tag:administrator`
* <*resource_server_id*>`.read:*/*/*`

We use the setting `management.oauth_scopes` to configure the scopes. It is a space-separated field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will a customer/user know what My understanding is you enter a scope followed by a space so: scope1 scope2 scope3

If that is correct then there probably is no need to clarify further, if I can understand more technical people definitely should :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is !! :)

### Identity-Provider initiated logon

By default, the Management UI uses OAuth 2.0 authorization code flow to authenticate and authorize users.
However, there are scenarios where users preferred to be automatically redirected to RabbitMQ without getting
Copy link
Contributor

@pstack2021 pstack2021 Nov 8, 2022

Choose a reason for hiding this comment

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

add "RabbitMQ" before the words "Management UI" in the first sentence just to be clear what UI it is at all times.

Add the word "the" after "uses" in the first sentence above

RabbitMQ exposes a new setting called `management.oauth_initiated_logon_type` whose default value `sp_initiated`.
To enable an **Identity-Provider initiated logon** we set it to `idp_initiated`.

When we set `management.oauth_initiated_logon_type` to `idp_initiated` the minimum required configuration is
Copy link
Contributor

@pstack2021 pstack2021 Nov 8, 2022

Choose a reason for hiding this comment

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

Avoid using "we" in docs where possible so ....When you set management.oauth_initiated_logon_type to idp_initiated,......

remember to add a comma after idp_initiated as I have done above

To enable an **Identity-Provider initiated logon** we set it to `idp_initiated`.

When we set `management.oauth_initiated_logon_type` to `idp_initiated` the minimum required configuration is
`oauth_enabled: true` and `oauth_provider_url`. The other settings related to OAuth are not required.
Copy link
Contributor

@pstack2021 pstack2021 Nov 8, 2022

Choose a reason for hiding this comment

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

have we control over what this setting is called @MarcialRosales, can we change it to: oauth_activate: true?? If it can't be done now then this should be considered for PHASE 2 of inclusive language updates in code, see more details here: #1551

Also with regard to this: "When we set management.oauth_initiated_logon_type to idp_initiated the minimum required configuration is oauth_enabled: true and oauth_provider_url. "

So does the user need to configure these settings now after management.oauth_initiated_logon_type is set to idp_initiated ??? If so, we should provide instructions to do this, thanks.

When we set `management.oauth_initiated_logon_type` to `idp_initiated` the minimum required configuration is
`oauth_enabled: true` and `oauth_provider_url`. The other settings related to OAuth are not required.
The `oauth_provider_url` should be the web portal address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps give an example of the above in documentation, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point ! Thanks

`oauth_enabled: true` and `oauth_provider_url`. The other settings related to OAuth are not required.
The `oauth_provider_url` should be the web portal address.

Once we set `management.oauth_initiated_logon_type` to `idp_initiated`, the management UI exposes the endpoint `/login` which accepts `content-type: application/x-www-form-urlencoded` and it expects the JWT token in the `access_token` form field.

Copy link
Contributor

@pstack2021 pstack2021 Nov 8, 2022

Choose a reason for hiding this comment

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

Should the above be updated to: "After you set management.oauth_initiated_logon_type to idp_initiated and oauth_enabled: true and oauth_provider_url are configured, the management UI exposes the endpoint /login which accepts content-type: application/x-www-form-urlencoded and it expects the JWT token in the access_token form field.

I have no idea what the above means, do we need to explain it in more simple terms? thanks

@@ -109,10 +109,10 @@ rather than `legacy-token-key`.

For backend applications which uses **Client Credentials flow** you create a **Client** with:
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comma after Client Credentials flow

add the word "can" in between "you" and "create"

@@ -109,10 +109,10 @@ rather than `legacy-token-key`.

For backend applications which uses **Client Credentials flow** you create a **Client** with:

* **Access Type** : `confidential`
* **Access Type** : `public`
* With all the other flows disabled: Standard Flow, Implicit Flow, Direct Access Grants
* With **Service Accounts Enabled** on. If it is not enabled you do not have the tab `Credentials`
Copy link
Contributor

@pstack2021 pstack2021 Nov 8, 2022

Choose a reason for hiding this comment

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

Again enable/disable terminology used here which we want to avoid going forward, see the table here for more details: https://source.vmware.com/portal/pages/global-marketing/terminology-changes
For the reference to "disabled" in the 3rd bullet point above, can we change it to "Turn off Standard Flow, Implicit Flow, and Direct Access Grants "

Are: Standard Flow, Implicit Flow, Direct Access Grant UI entities? if so, lets tag them appropriately, I think bold is being used to tag UI entities in OSS RabbitMQ docs right now, please use that here if so, thanks

4th bullet point: Service Accounts Enabled should change to Service Accounts Activated in PHASE 2 of inclusive language updates in code entities, more details about that in this issue: #1551

Also, a suggested reword of the 4th bullet point, how about: Service Accounts Enabled is turned on, if it is not, you won't have the Credentials tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot do anything about Service Accounts Enabled because that is exactly how it is called in Keycloak.

* With all the other flows disabled: Standard Flow, Implicit Flow, Direct Access Grants
* With **Service Accounts Enabled** on. If it is not enabled you do not have the tab `Credentials`
* In tab `Credentials` you have the client id secret
* In tab `Credentials` you have the client id

Copy link
Contributor

Choose a reason for hiding this comment

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

In the Credentials tab,......


## <a id="use-access-tokens" class="anchor" href="#use-oauth-tokens">Use access tokens</a>
The Management UI can be configured with one of these two login modes:
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to "The RabbitMQ Management UI....."


The following subsections demonstrate how to use access tokens with any messaging protocol and also to access the management ui and rest api.
* [Service-Provider initiated logon](#service-provider-initiated-logon) - This is the default and traditional OAuth 2.0 logon mode. The user comes to the Management UI and clicks on the button "Click here to logon" which initiates the logon. The logon process starts in RabbitMQ, the *Service Provider*.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested reword for this sentence: "The user comes to the Management UI and clicks on the button "Click here to logon" which initiates the logon. " CHANGE TO:
"In the RabbitMQ Management UI, click "Click here to logon" to initiate the logon. "


The following subsections demonstrate how to use access tokens with any messaging protocol and also to access the management ui and rest api.
* [Service-Provider initiated logon](#service-provider-initiated-logon) - This is the default and traditional OAuth 2.0 logon mode. The user comes to the Management UI and clicks on the button "Click here to logon" which initiates the logon. The logon process starts in RabbitMQ, the *Service Provider*.
* [Identity-Provider initiated logon](#identity-provider-initiated-logon) - This is a logon mode meant for web portals. Users navigate to RabbitMQ with a token already obtained by the web portal on behalf of the user.
Copy link
Contributor

@pstack2021 pstack2021 Nov 8, 2022

Choose a reason for hiding this comment

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

So avoid using past tense and use present where possible in documentation - How about: "You can use this logon mode for web portals. Users can navigate to RabbitMQ with an existing token which is retrieved from the web portal on behalf of the user."

I don't really know what the final sentence above means in the context of "Users can...." and then "on behalf of the user" so are you saying a "user can navigate to RabbitMQ with an existing token which is retrieved from the web portal and they can use that token to logon" so it is the user that is navigating that is actually logging on?? If so how about this for a reword: "You can navigate to RabbitMQ with an existing token which you previously retrieved from the web portal, you can then use this token to logon."????

Copy link
Contributor

@pstack2021 pstack2021 left a comment

Choose a reason for hiding this comment

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

Hi @MarcialRosales I will complete the review tomorrow, I hope my comments are ok and helpful, thanks.

site/oauth2.md Outdated
RabbitMQ needs to figure out the username associated to the token so that it can display it in the management ui.
By default, RabbitMQ will first look for the `sub` claim and if it is not found it uses the `client_id`.

Most authorization servers return the user's GUID in the `sub` claim rather than the actual user's username or email address, anything the user can relate to. When the `sub` claim does not carry a *user-friendly username*, you can configure one or several claims to extract the username from the token.
Copy link
Contributor

Choose a reason for hiding this comment

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

" anything the user can relate to."....what does this part of the sentence mean in this context? should the word the word "or" be added before this text then it would makes sense???

MarcialRosales and others added 16 commits January 18, 2023 09:15
- Changed http endpoint for idp-initiated login
- Preferred username claims
- oauth_client_secret optional
@MarcialRosales some minor updates for Identity-Provider initiated logon mode, I hope they are ok.
Some updates as a result of my review @MarcialRosales, can you have a quick look through them to ensure they are correct, thank you.
Some more updates for me review, again can you quickly look through them to ensure all is good, thanks @MarcialRosales
@michaelklishin michaelklishin force-pushed the oidc_idp_initiated_login branch from ed50c32 to 18c82d2 Compare January 18, 2023 16:08
@michaelklishin michaelklishin merged commit e063a8f into live Jan 18, 2023
@michaelklishin michaelklishin deleted the oidc_idp_initiated_login branch January 18, 2023 16:09
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.

3 participants