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

Fixes #23234: Hash API tokens #4971

Merged

Conversation

amousset
Copy link
Member

@amousset amousset commented Aug 9, 2023

https://issues.rudder.io/issues/23234

See https://www.notion.so/rudderio/Hash-des-tokens-d-API-ddb9ec15820e4a81875b2a235d3f32e5 for details.

Technically, in the webapp:

  • Nothing changes in the LDAP schema, we only change the content of the token field
  • Centralize token management (hash, generation) in ApiToken. This will ease future modifications and ensure consistency.
  • Changes in the API:
    • On get request, return an empty token field if it is hashed.
    • On put/post that modify the token, return the clear-text password in the token field to pass it to the user

The frontend changes are straightforward:

  • New window based on modals, allowing to copy the clear-text token, opened after account creation or regeneration of a token.
  • Changes in the token column:
    • Don't display it if there are not v1 tokens (and use the earned space for displaying creation date)
    • Move the regeneration button to the actions column

Fixes #23234: Hash API tokens
@amousset
Copy link
Member Author

amousset commented Aug 9, 2023

PR updated with a new commit

Fixes #23234: Hash API tokens
@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

@amousset
Copy link
Member Author

PR updated with a new commit

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Lgtm (comments were made and addressed in previous pr)

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

Tested:

  • using old token
  • creating new token
  • migrating token
  • user token
  • new token with API ACLS

Everything works great, message are 👍 , and all code related topic were previously addressed.

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/4971
-- Your faithful QA
Kant merge: "To be is to do."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/72344/console)

@amousset
Copy link
Member Author

OK, squash merging this PR

@amousset amousset merged commit 4c1ee7f into Normation:branches/rudder/8.0 Aug 16, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants