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

Updated request.GetAuthType to handle multi-value auth headers #1379

Merged
merged 2 commits into from Jul 18, 2016
Merged

Updated request.GetAuthType to handle multi-value auth headers #1379

merged 2 commits into from Jul 18, 2016

Conversation

VladimirKhvostov
Copy link
Contributor

GetAuthType method in https://github.com/github/git-lfs/blob/master/httputil/request.go#L174 only takes first WWW-Authenticate header into account. If server sends the following headers:

WWW-Authenticate: Bearer
WWW-Authenticate: Negotiate
WWW-Authenticate: NTLM

the code returns "basic", which is definitely not correct in this case.

This change fixes this issue. With this change we will also handle Negotiate authentication.

Bug: #1377


if strings.HasPrefix(strings.ToLower(auth), "ntlm") {
return "ntlm"
for _, headerName := range []string{WwwAuthenticateHeader, LfsAuthenticateHeader} {
Copy link
Contributor

Choose a reason for hiding this comment

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

The inner contents of this loop look good. I wonder if we could make things a little bit clearer by making removing the WwwAuthenticateHeader and LfsAuthenticateHeader consts in favor of a single:

var (
        AuthenticateHeaders = []string{"Www-Authenticate", "Lfs-Authenticate"}
)

since we never use them individually, only in this form on line 184.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I was thinking about this, but was not sure what is the right way to do this in GO lang. Thanks for teaching me best practices.

@ttaylorr
Copy link
Contributor

Thanks for re-opening this PR 😄 . Few more comments from me, but this looks great!

Like I said, feel free to not go with my test-case suggestion. I'd be fine merging this in either way.

@ttaylorr
Copy link
Contributor

Great, thanks! 👍

@ttaylorr ttaylorr merged commit 407b13f into git-lfs:master Jul 18, 2016
@technoweenie
Copy link
Contributor

Thanks for the patch! I made a few changes in 29e8876 so that the httputil package doesn't export values like BasicAuthType or AuthenticateHeaders. Those are only used for the internals of the httputil package, so they don't need to be exported.

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.

None yet

3 participants