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

Refine FetchToken Logic #2553

Merged
merged 1 commit into from
Jan 14, 2022
Merged

Refine FetchToken Logic #2553

merged 1 commit into from
Jan 14, 2022

Conversation

shizhMSFT
Copy link
Contributor

@shizhMSFT shizhMSFT commented Jan 11, 2022

Always do OAuth2 if creds is a refresh token, not a username / password pair.

Resolves #2554.

@tonistiigi
Copy link
Member

@AkihiroSuda

This code was based on https://github.com/containerd/containerd/blame/main/remotes/docker/authorizer.go#L299 . Does it have a similar case or has it been updated or is the issue buildkit specific?

I think one related issue in here was that historically Docker hub only implemented one of the methods and we tried to avoid a discarded request on the default path. I think this has been fixed though and now both methods are supported. Could you verify that?

@shizhMSFT
Copy link
Contributor Author

This code was based on https://github.com/containerd/containerd/blame/main/remotes/docker/authorizer.go#L299 . Does it have a similar case or has it been updated or is the issue buildkit specific?

Looks like the containerd code has been updated.

tonistiigi
tonistiigi previously approved these changes Jan 13, 2022
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Please squash the commits as everything in the first commit seems to be overwritten by the second one.

@tonistiigi tonistiigi dismissed their stale review January 13, 2022 05:26

Please squash

Signed-off-by: Shiwei Zhang <shizh@microsoft.com>
@shizhMSFT
Copy link
Contributor Author

@tonistiigi The commits have been squashed into one commit.

@tonistiigi tonistiigi merged commit 2dc3e74 into moby:master Jan 14, 2022
@shizhMSFT shizhMSFT deleted the fix_auth branch January 14, 2022 04:12
@crazy-max crazy-max added this to the v0.10.0 milestone Feb 4, 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.

RefreshToken is not properly attempted in FetchToken
3 participants