[ARVADOS] updated: 1.3.0-2348-g23d90de73
Git user
git at public.arvados.org
Thu Mar 26 18:33:19 UTC 2020
Summary of changes:
lib/controller/localdb/login_pam.go | 15 +++++++++------
lib/controller/localdb/login_pam_docker_test.sh | 24 +++++++++++++++++++++---
lib/controller/localdb/login_pam_test.go | 9 +++++++--
sdk/go/arvados/login.go | 3 +++
4 files changed, 40 insertions(+), 11 deletions(-)
via 23d90de7344ae39d1d0082a369b0dc5e9086531d (commit)
via c0ac5a7421a59e562ea796daa926bbf1d0cc0c3e (commit)
from 4e0eb166fd808b32c10cccc2b4014a02edcf29a6 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 23d90de7344ae39d1d0082a369b0dc5e9086531d
Author: Tom Clegg <tom at tomclegg.ca>
Date: Thu Mar 26 14:31:18 2020 -0400
16212: Return 401 or 500 instead of 200 on authentication failure.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>
diff --git a/lib/controller/localdb/login_pam.go b/lib/controller/localdb/login_pam.go
index 7955aef11..059e27fa2 100644
--- 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 f85d6bb5f..1d37f5c1c 100755
--- 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 at 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 885aa86b8..4af40f8d3 100644
--- 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 579c10837..26c04b2c1 100644
--- 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")
commit c0ac5a7421a59e562ea796daa926bbf1d0cc0c3e
Author: Tom Clegg <tom at tomclegg.ca>
Date: Thu Mar 26 12:25:37 2020 -0400
16212: Build arvados-server binary for test.
On a dev system, the one in $GOPATH/bin might be a different version.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>
diff --git a/lib/controller/localdb/login_pam_docker_test.sh b/lib/controller/localdb/login_pam_docker_test.sh
index 7fb0a303a..f85d6bb5f 100755
--- a/lib/controller/localdb/login_pam_docker_test.sh
+++ b/lib/controller/localdb/login_pam_docker_test.sh
@@ -131,12 +131,15 @@ docker run --rm --entrypoint= \
osixia/openldap:1.3.0 \
bash -c "for f in \$(seq 1 5); do if ldapadd -H '${ldapurl}' -D 'cn=${adminuser},dc=example,dc=org' -w '${adminpassword}' -f /add_example_user.ldif; then exit 0; else sleep 2; fi; done; echo 'failed to add user entry'; exit 1"
+echo >&2 "Building arvados controller binary to run in container"
+go build -o "${tmpdir}" ../../../cmd/arvados-server
+
ctrlctr=ctrl-${RANDOM}
echo >&2 "Starting arvados controller in docker container ${ctrlctr}"
docker run --detach --rm --name=${ctrlctr} \
-p 9999 \
-v "${tmpdir}/pam_ldap.conf":/etc/pam_ldap.conf:ro \
- -v "${GOPATH:-${HOME}/go}/bin/arvados-server":/bin/arvados-server:ro \
+ -v "${tmpdir}/arvados-server":/bin/arvados-server:ro \
-v "${tmpdir}/zzzzz.yml":/etc/arvados/config.yml:ro \
-v $(realpath "${PWD}/../../.."):/arvados:ro \
debian:10 \
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list