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

client_secret should be optional when using authorization code flow #6201

Closed
wants to merge 2 commits into from

Conversation

MarcialRosales
Copy link
Contributor

@MarcialRosales MarcialRosales commented Oct 20, 2022

RabbitMQ Management UI is a considered a public client application in the context of OAuth 2.0 spec. That means that it is exposed and hence any credentials or secrets cannot be securely stored, such as client_secret.
Authorization Code flow with PKCE does not need the client application to send its client_secret to get the access_token once it obtained an authorization code. PKCE is used instead.

Note: When using UAA, registered clients must be configured with the flag allowpublic: true otherwise the client assumes it is a confidential client and it expects a client_secret to get the access token.

We have not entirely removed the management.oauth_client_secret setting from the management plugin. Instead, only when it is configured, it is used by the plugin. This is so that there is an authorization server that requires it. But it is not mandatory any more.

Validate that we can use Authorization code flow with PKCE without client_secret against:

  • UAA
  • KeyCloak
  • Auth0 (manual)
  • Azure AD (manual)

(manual means it was manually validated using rabbitmq-oauth2-tutorial

This PR also includes:

  • Javascript formatting improvements
  • Remove some unnecessary code and files
  • Improve scripts that runs selenium tests
  • Add selenium test that verifies oauth authentication against keycloak

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

Further Comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc.

@mergify mergify bot added the make label Oct 21, 2022
    Also:
    -Javascript formatting improvements
    -Clean up some necessary code and files
    -Improve scripts that runs selenium tests
    -Add selenium test that verifies oauth authentication against keycloak
@MarcialRosales
Copy link
Contributor Author

This PR has been merged onto #6015

@MarcialRosales MarcialRosales deleted the oidc-drop-client-secret branch January 31, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant