fixup! NXD blob/auth: Teach it to handle HTTP Basic Auth too

@rafael approached me and asked why URLs like

	https://gitlab-ci-token:XXX@hostname/group/project/raw/master/file

work in CURL, but not in Chrome under AJAX requests.

After investigation it turned out they neither work in WGET and give 302
redirect to http://localhost:8080/users/sign_in:

	kirr@deco:~$ wget https://gitlab-ci-token:XXX@lab.nexedi.com/kirr/test/raw/master/hello.txt
	--2018-06-04 13:14:04--  https://gitlab-ci-token:*password*@lab.nexedi.com/kirr/test/raw/master/hello.txt
	Resolving lab.nexedi.com (lab.nexedi.com)... 176.31.129.213, 85.118.38.162
	Connecting to lab.nexedi.com (lab.nexedi.com)|176.31.129.213|:443... connected.
	HTTP request sent, awaiting response... 302 Found
	Location: http://localhost:8080/users/sign_in [following]
	--2018-06-04 13:14:04--  http://localhost:8080/users/sign_in
	Resolving localhost (localhost)... 127.0.0.1, ::1
	Connecting to localhost (localhost)|127.0.0.1|:8080... failed: Connection refused.
	Connecting to localhost (localhost)|::1|:8080... failed: Connection refused.

This turned out to be due to most clients (in fine accordance with RFC2617 /
RFC7617) first send request without Authorization header set and retry it with
that header only if server challenges it to(*), and our authorization code was
only trying to handle HTTP basic auth if Authorization header was provided
without issuing any challenge on server side.

Fix it by checking Rails backend reply for 302, which it gives for
unauthorized non-raw requests, and on our side convert it HTTP Basic
auth challenge if raw request does not contain any token. This way it
now works with user:password in URLs for both WGET and Chrome.

If any tokens were provided we leave Rails auth response as is because
we handle user/password only for that "no token provided at all" case.

(*) see https://en.wikipedia.org/wiki/Basic_access_authentication for overview.
/cc @alain.takoudjou, @jerome

/reviewed-on !2

internal/api/auth.go

@@ -276,8 +276,8 @@ func (a *API) verifyDownloadAccess(project string, user *url.Userinfo, query str
 	// Request to auth backend to verify whether download is possible.
 	// - first option is via asking as `git fetch` would do, but on Rails
 	//   side this supports only basic auth, not private token.
-	// - that's why we auth backend to authenticate as if it was request to
-	//   get repo archive and propagate request query and header.
+	// - that's why we first ask auth backend to authenticate as if it was
+	//   request to get repo archive and propagate request query and header.
 	url := project + "/repository/archive.zip"
 	if query != "" {
 		url += "?" + query
@@ -308,13 +308,29 @@ func (a *API) verifyDownloadAccess(project string, user *url.Userinfo, query str
 	)
 	authProxy.ServeHTTP(authReply.RawReply, reqDownloadAccess)
 
-	// If not successful and userinfo is provided without query, retry
-	// authenticating as `git fetch` would do.
+	// If not successful and any token was not provided neither in
+	// query nor in header, retry authenticating as `git fetch` would do.
 	//
 	// The reason we want to do this second try is that HTTP auth is
 	// handled by upstream auth backend for git requests only, and we might
 	// want to use e.g. https://gitlab-ci-token:token@/.../raw/...
-	if authReply.RepoPath != "" || user == nil || query != "" {
+	if authReply.RepoPath != "" || query != "" || len(header) != 0 {
+		return authReply
+	}
+	if user == nil {
+		// backend gives "302 location: .../users/sign_in" when rejecting access.
+		// transform this to HTTP Basic Auth challenge if we don't have user/password already.
+		//
+		// reason: many clients - e.g. wget, chrome, even when given user/password,
+		// first send request without auth set and expect server to send them auth challenge.
+		// and only after the challenge they retry the request with Authorization header set.
+		if authReply.RawReply.Code == http.StatusFound &&
+			strings.HasSuffix(authReply.RawReply.HeaderMap.Get("location"), "/users/sign_in") {
+			r := httptest.NewRecorder()
+			r.Header().Set("WWW-Authenticate", "Basic realm=\"\"")
+			r.WriteHeader(http.StatusUnauthorized)
+			authReply.RawReply = r
+		}
 		return authReply
 	}
 

main_test.go

@@ -739,7 +739,9 @@ func download(t *testing.T, url, username, password string, h http.Header) (*htt
 	for k, v := range h {
 		req.Header[k] = v
 	}
-	client := &http.Client{}
+	client := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error {
+		return http.ErrUseLastResponse // don't follow redirects
+	}}
 	resp, err := client.Do(req)
 	if err != nil {
 		t.Fatal(err)
@@ -795,6 +797,14 @@ func (dl DownloadContext) ExpectCode(path string, code int) {
 	}
 }
 
+// download `path` and expect HTTP reply header to be set to `value`.
+func (dl DownloadContext) ExpectHeader(path, header, value string) {
+	resp, _ := dl.download(path)
+	if h := resp.Header.Get(header); h != value {
+		dl.t.Fatalf("Header %q: expected %q; got %q", header, value, h)
+	}
+}
+
 func TestBlobDownload(t *testing.T) {
 	// Prepare test server and "all-ok" auth backend
 	ts := archiveOKServer(t, "")
@@ -830,16 +840,26 @@ func TestPrivateBlobDownload(t *testing.T) {
 	ts := testhelper.TestServerWithHandler(nil, func(w http.ResponseWriter, r *http.Request) {
 		log.Println("UPSTREAM", r.Method, r.URL)
 		gitfetch := (strings.HasSuffix(r.URL.Path, "/info/refs") && r.URL.RawQuery == "service=git-upload-pack")
-		token_ok1 := r.URL.Query().Get("aaa_token") == "TOKEN-4AAA"
-		token_ok2 := r.Header.Get("BBB-TOKEN") == "TOKEN-4BBB"
-		cookie, _ := r.Cookie("_gitlab_session")
-		cookie_ok3 := (cookie != nil && cookie.Value == "COOKIE-CCC")
-		username, password, user_ok4 := r.BasicAuth()
-		if user_ok4 {
+		token1 := r.URL.Query().Get("aaa_token")
+		token2 := r.Header.Get("BBB-TOKEN")
+		cookie := ""
+		if c, err := r.Cookie("_gitlab_session"); err == nil {
+			cookie = c.Value
+		}
+		username, password, user_ok := r.BasicAuth()
+		if user_ok {
 			// user:password only accepted for `git fetch` requests
-			user_ok4 = (gitfetch && username == "user-ddd" && password == "password-eee")
+			user_ok = (gitfetch && username == "user-ddd" && password == "password-eee")
+		}
+
+		// simulate rails which gives "302 location: .../users/sign_in" when no access by token
+		if !gitfetch && (token1 == "" && token2 == "" && cookie == "") {
+			w.Header().Set("location", ".../users/sign_in")
+			w.WriteHeader(302)
+			return
 		}
-		if !(token_ok1 || token_ok2 || cookie_ok3 || user_ok4) {
+
+		if !(token1 == "TOKEN-4AAA" || token2 == "TOKEN-4BBB" || cookie == "COOKIE-CCC" || user_ok) {
 			w.WriteHeader(403)
 			fmt.Fprintf(w, "Access denied")
 			return
@@ -872,35 +892,41 @@ func TestPrivateBlobDownload(t *testing.T) {
 	defer ws.Close()
 	dl := NewDownloadContext(t, fmt.Sprintf("%s/%s/raw", ws.URL, testProject))
 
-	dl.ExpectCode("/5f923865/README.md", 403)
-	dl.ExpectCode("/5f923865/README.md?bbb_token=TOKEN-4BBB", 403)
+	dl.ExpectCode("/5f923865/README.md", 401)
+	dl.ExpectCode("/5f923865/README.md?bbb_token=TOKEN-4BBB", 302)
+	dl.ExpectCode("/5f923865/README.md?aaa_token=TOKEN-XXXX", 403)
 	dl.ExpectCode("/5f923865/README.md?aaa_token=TOKEN-4AAA", 200)
 	dl.ExpectSha1("/5f923865/README.md?aaa_token=TOKEN-4AAA", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
 
-	dl.Header.Add("AAA-TOKEN", "TOKEN-4AAA")
+	dl.Header.Set("AAA-TOKEN", "TOKEN-4AAA")
+	dl.ExpectCode("/5f923865/README.md", 302)
+	dl.Header.Set("BBB-TOKEN", "TOKEN-XXX")
 	dl.ExpectCode("/5f923865/README.md", 403)
-	dl.Header.Add("BBB-TOKEN", "TOKEN-4BBB")
+	dl.Header.Set("BBB-TOKEN", "TOKEN-4BBB")
 	dl.ExpectCode("/5f923865/README.md", 200)
 	dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
 
 	dl.Header = make(http.Header) // clear
-	dl.ExpectCode("/5f923865/README.md", 403)
+	dl.ExpectCode("/5f923865/README.md", 401)
 	dl.Header.Set("Cookie", "alpha=1")
-	dl.ExpectCode("/5f923865/README.md", 403)
+	dl.ExpectCode("/5f923865/README.md", 401)
 	dl.Header.Set("Cookie", "alpha=1; beta=2")
+	dl.ExpectCode("/5f923865/README.md", 401)
+	dl.Header.Set("Cookie", "alpha=1; _gitlab_session=COOKIE-XXX; beta=2")
 	dl.ExpectCode("/5f923865/README.md", 403)
 	dl.Header.Set("Cookie", "alpha=1; _gitlab_session=COOKIE-CCC; beta=2")
 	dl.ExpectCode("/5f923865/README.md", 200)
 	dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
 
 	dl.Header = make(http.Header) // clear
-	dl.ExpectCode("/5f923865/README.md", 403)
+	dl.ExpectCode("/5f923865/README.md", 401)
+	dl.ExpectHeader("/5f923865/README.md", "www-authenticate", "Basic realm=\"\"")
 	dl.username = "user-aaa"
 	dl.password = "password-bbb"
 	dl.ExpectCode("/5f923865/README.md", 403)
 	dl.username = "user-ddd"
 	dl.password = "password-eee"
-	dl.ExpectCode("/5f923865/README.md?qqq_token=1", 403)
+	dl.ExpectCode("/5f923865/README.md?qqq_token=1", 302)
 	dl.ExpectCode("/5f923865/README.md", 200)
 	dl.ExpectSha1("/5f923865/README.md", "5f7af35c185a9e5face2f4afb6d7c4f00328d04c")
 }