16212: Return 401 or 500 instead of 200 on authentication failure.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

diff --git a/lib/controller/localdb/login_pam.go b/lib/controller/localdb/login_pam.go
index 7955aef1187309576988661f55dc33633d20517b..059e27fa26ef1b9a44d3ceb9cf687b9e4831aaf7 100644 (file)
--- a/lib/controller/localdb/login_pam.go
+++ b/lib/controller/localdb/login_pam.go
@@ -6,7 +6,9 @@ package localdb
 
 import (
        "context"
+       "errors"
        "fmt"
+       "net/http"
        "net/url"
        "strings"
 
@@ -14,6 +16,7 @@ import (
        "git.arvados.org/arvados.git/sdk/go/arvados"
        "git.arvados.org/arvados.git/sdk/go/auth"
        "git.arvados.org/arvados.git/sdk/go/ctxlog"
+       "git.arvados.org/arvados.git/sdk/go/httpserver"
        "github.com/msteinert/pam"
        "github.com/sirupsen/logrus"
 )
@@ -46,18 +49,18 @@ func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOpt
                }
        })
        if err != nil {
-               return arvados.LoginResponse{Message: err.Error()}, nil
+               return arvados.LoginResponse{}, err
        }
        err = tx.Authenticate(pam.DisallowNullAuthtok)
        if err != nil {
-               return arvados.LoginResponse{Message: err.Error()}, nil
+               return arvados.LoginResponse{}, httpserver.ErrorWithStatus(err, http.StatusUnauthorized)
        }
        if errorMessage != "" {
-               return arvados.LoginResponse{Message: errorMessage}, nil
+               return arvados.LoginResponse{}, httpserver.ErrorWithStatus(errors.New(errorMessage), http.StatusUnauthorized)
        }
        user, err := tx.GetItem(pam.User)
        if err != nil {
-               return arvados.LoginResponse{Message: err.Error()}, nil
+               return arvados.LoginResponse{}, err
        }
        email := user
        if domain := ctrl.Cluster.Login.PAMDefaultEmailDomain; domain != "" && !strings.Contains(email, "@") {
@@ -76,11 +79,11 @@ func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOpt
                },
        })
        if err != nil {
-               return arvados.LoginResponse{Message: err.Error()}, nil
+               return arvados.LoginResponse{}, err
        }
        target, err := url.Parse(resp.RedirectLocation)
        if err != nil {
-               return arvados.LoginResponse{Message: err.Error()}, nil
+               return arvados.LoginResponse{}, err
        }
        resp.Token = target.Query().Get("api_token")
        resp.RedirectLocation = ""

diff --git a/lib/controller/localdb/login_pam_docker_test.sh b/lib/controller/localdb/login_pam_docker_test.sh
index f85d6bb5fa426e75f1d33f78d0314f014216f2ad..1d37f5c1c9e7d4250642cfecff1b762ad353351a 100755 (executable)
--- a/lib/controller/localdb/login_pam_docker_test.sh
+++ b/lib/controller/localdb/login_pam_docker_test.sh
@@ -159,9 +159,24 @@ done
 echo >&2
 echo >&2 "Arvados controller is up at http://${ctrlhostport}"
 
+check_contains() {
+    resp="${1}"
+    str="${2}"
+    if ! echo "${resp}" | fgrep -q "${str}"; then
+        echo >&2 "${resp}"
+        echo >&2 "FAIL: expected in response, but not found: ${str@Q}"
+        return 1
+    fi
+}
+
 echo >&2 "Testing authentication failure"
-curl -s -H "X-Http-Method-Override: GET" -d username=foo -d password=nosecret "http://${ctrlhostport}/login" | tee $debug | grep "Authentication failure"
+resp="$(curl -s --include -H "X-Http-Method-Override: GET" -d username=foo -d password=nosecret "http://${ctrlhostport}/login" | tee $debug)"
+check_contains "${resp}" "HTTP/1.1 401"
+check_contains "${resp}" '{"errors":["Authentication failure"]}'
+
 echo >&2 "Testing authentication success"
-curl -s -H "X-Http-Method-Override: GET" -d username=foo -d password=secret "http://${ctrlhostport}/login" | tee $debug | fgrep '{"token":"v2/zzzzz-gj3su-'
+resp="$(curl -s --include -H "X-Http-Method-Override: GET" -d username=foo -d password=secret "http://${ctrlhostport}/login" | tee $debug)"
+check_contains "${resp}" "HTTP/1.1 200"
+check_contains "${resp}" '{"token":"v2/zzzzz-gj3su-'
 
 cleanup

diff --git a/lib/controller/localdb/login_pam_test.go b/lib/controller/localdb/login_pam_test.go
index 885aa86b88b3c0fb0159a8b2599472b833245283..4af40f8d38fb4ebe8cec8b98eb6d457ca193fb63 100644 (file)
--- a/lib/controller/localdb/login_pam_test.go
+++ b/lib/controller/localdb/login_pam_test.go
@@ -7,6 +7,7 @@ package localdb
 import (
        "context"
        "io/ioutil"
+       "net/http"
        "os"
        "strings"
 
@@ -46,10 +47,14 @@ func (s *PamSuite) TestLoginFailure(c *check.C) {
                Password: "boguspassword",
                ReturnTo: "https://example.com/foo",
        })
-       c.Check(err, check.IsNil)
+       c.Check(err, check.ErrorMatches, "Authentication failure")
+       hs, ok := err.(interface{ HTTPStatus() int })
+       if c.Check(ok, check.Equals, true) {
+               c.Check(hs.HTTPStatus(), check.Equals, http.StatusUnauthorized)
+       }
        c.Check(resp.RedirectLocation, check.Equals, "")
        c.Check(resp.Token, check.Equals, "")
-       c.Check(resp.Message, check.Equals, "Authentication failure")
+       c.Check(resp.Message, check.Equals, "")
        c.Check(resp.HTML.String(), check.Equals, "")
 }
 

diff --git a/sdk/go/arvados/login.go b/sdk/go/arvados/login.go
index 579c1083799888db480f0d3cd5cd1522a7f8a084..26c04b2c13603a2a7c9aa721b38a1fd5343c5228 100644 (file)
--- a/sdk/go/arvados/login.go
+++ b/sdk/go/arvados/login.go
@@ -24,6 +24,9 @@ func (resp LoginResponse) ServeHTTP(w http.ResponseWriter, req *http.Request) {
                w.WriteHeader(http.StatusFound)
        } else if resp.Token != "" || resp.Message != "" {
                w.Header().Set("Content-Type", "application/json")
+               if resp.Token == "" {
+                       w.WriteHeader(http.StatusUnauthorized)
+               }
                json.NewEncoder(w).Encode(resp)
        } else {
                w.Header().Set("Content-Type", "text/html")