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

fix user access token deletion (fix #186) #205

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

converge
Copy link
Contributor

- What I did

As reported on #186, user token deletion is failing to delete user access tokens. The issue is caused by the missing username at the hubClient.AuthConfig.Username struct field, the confirmation step looks for the username to confirm, but the username information is empty/not set.

- How I did it

At initialization, the auth, err := store.GetAuth() is called, I got it in advance, and provided the authorization data to the struct.

- How to verify it

  1. create a new user token via hub-tool token create
  2. delete the user token via hub-tool token rm uuid-token
  3. the confirmation step will now show the username, and the username can be confirmed.

- Description for the changelog

Fix user token deletion by adding missing authorization data to the hubClient.

- A picture of a cute animal (not mandatory)

Signed-off-by: João Vanzuita <joaovanzuita@me.com>
@converge converge changed the title fix user access token deletion fix user access token deletion (fix #186) Apr 12, 2022
fmt.Fprintf(streams.Out(), ansi.Warn("WARNING: This action is irreversible.")+`
By confirming, you will permanently delete the access token.
Deleting a token will invalidate your credentials on all Docker clients currently authenticated with this token.
By confirming, you will permanently revoke and delete the access token.
Copy link
Collaborator

Choose a reason for hiding this comment

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

About rephrasing the warning message, the original intent was to match Docker Hub UX:
Screenshot 2022-04-13 at 09 55 07

Copy link
Collaborator

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

Logic looks good to me, just a small change needed in the warning message 👍
Thanks for your PR @converge 🙏

Signed-off-by: João Vanzuita <joaovanzuita@me.com>
@converge
Copy link
Contributor Author

Logic looks good to me, just a small change needed in the warning message 👍
Thanks for your PR @converge 🙏

Tks again for the review, these little details make a good difference in the overall user experience.

@converge converge requested a review from silvin-lubecki April 13, 2022 19:33
Copy link
Collaborator

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@silvin-lubecki silvin-lubecki merged commit 0fee32c into docker:main Apr 14, 2022
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