[ARVADOS] created: 1.3.0-2049-gceabb4293
Git user
git at public.arvados.org
Thu Jan 2 21:19:03 UTC 2020
at ceabb42934ec7c462f9ae03531080a24819dee1a (commit)
commit ceabb42934ec7c462f9ae03531080a24819dee1a
Author: Tom Clegg <tom at tomclegg.ca>
Date: Thu Jan 2 16:18:47 2020 -0500
15934: Fix "bad token" error message.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at tomclegg.ca>
diff --git a/lib/controller/fed_containers.go b/lib/controller/fed_containers.go
index 8bb68d171..a923f757f 100644
--- a/lib/controller/fed_containers.go
+++ b/lib/controller/fed_containers.go
@@ -33,9 +33,12 @@ func remoteContainerRequestCreate(
creds := auth.NewCredentials()
creds.LoadTokensFromHTTPRequest(req)
- currentUser, err := h.handler.validateAPItoken(req, creds.Tokens[0])
+ currentUser, ok, err := h.handler.validateAPItoken(req, creds.Tokens[0])
if err != nil {
- httpserver.Error(w, err.Error(), http.StatusForbidden)
+ httpserver.Error(w, err.Error(), http.StatusInternalServerError)
+ return true
+ } else if !ok {
+ httpserver.Error(w, "invalid API token", http.StatusForbidden)
return true
}
diff --git a/lib/controller/federation.go b/lib/controller/federation.go
index fcff05c00..674183dcc 100644
--- a/lib/controller/federation.go
+++ b/lib/controller/federation.go
@@ -141,11 +141,19 @@ type CurrentUser struct {
// checks it again api_client_authorizations table in the database,
// and fills in the token scope and user UUID. Does not handle remote
// tokens unless they are already in the database and not expired.
-func (h *Handler) validateAPItoken(req *http.Request, token string) (*CurrentUser, error) {
+//
+// Return values are:
+//
+// nil, false, non-nil -- if there was an internal error
+//
+// nil, false, nil -- if the token is invalid
+//
+// non-nil, true, nil -- if the token is valid
+func (h *Handler) validateAPItoken(req *http.Request, token string) (*CurrentUser, bool, error) {
user := CurrentUser{Authorization: arvados.APIClientAuthorization{APIToken: token}}
db, err := h.db(req)
if err != nil {
- return nil, err
+ return nil, false, err
}
var uuid string
@@ -157,17 +165,20 @@ func (h *Handler) validateAPItoken(req *http.Request, token string) (*CurrentUse
user.Authorization.APIToken = token
var scopes string
err = db.QueryRowContext(req.Context(), `SELECT api_client_authorizations.uuid, api_client_authorizations.scopes, users.uuid FROM api_client_authorizations JOIN users on api_client_authorizations.user_id=users.id WHERE api_token=$1 AND (expires_at IS NULL OR expires_at > current_timestamp) LIMIT 1`, token).Scan(&user.Authorization.UUID, &scopes, &user.UUID)
- if err != nil {
- return nil, err
+ if err == sql.ErrNoRows {
+ return nil, false, nil
+ } else if err != nil {
+ return nil, false, err
}
if uuid != "" && user.Authorization.UUID != uuid {
- return nil, fmt.Errorf("UUID embedded in v2 token did not match record")
+ // secret part matches, but UUID doesn't -- somewhat surprising
+ return nil, false, nil
}
err = json.Unmarshal([]byte(scopes), &user.Authorization.Scopes)
if err != nil {
- return nil, err
+ return nil, false, err
}
- return &user, nil
+ return &user, true, nil
}
func (h *Handler) createAPItoken(req *http.Request, userUUID string, scopes []string) (*arvados.APIClientAuthorization, error) {
@@ -251,12 +262,12 @@ func (h *Handler) saltAuthToken(req *http.Request, remote string) (updatedReq *h
// If the token exists in our own database, salt it
// for the remote. Otherwise, assume it was issued by
// the remote, and pass it through unmodified.
- currentUser, err := h.validateAPItoken(req, creds.Tokens[0])
- if err == sql.ErrNoRows {
+ currentUser, ok, err := h.validateAPItoken(req, creds.Tokens[0])
+ if err != nil {
+ return nil, err
+ } else if !ok {
// Not ours; pass through unmodified.
token = creds.Tokens[0]
- } else if err != nil {
- return nil, err
} else {
// Found; make V2 version and salt it.
token, err = auth.SaltToken(currentUser.Authorization.TokenV2(), remote)
diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index d0bff84ea..2b0cb22b0 100644
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -586,6 +586,21 @@ func (s *FederationSuite) TestUpdateRemoteContainerRequest(c *check.C) {
setPri(1) // Reset fixture so side effect doesn't break other tests.
}
+func (s *FederationSuite) TestCreateContainerRequestBadToken(c *check.C) {
+ defer s.localServiceReturns404(c).Close()
+ // pass cluster_id via query parameter, this allows arvados-controller
+ // to avoid parsing the body
+ req := httptest.NewRequest("POST", "/arvados/v1/container_requests?cluster_id=zzzzz",
+ strings.NewReader(`{"container_request":{}}`))
+ req.Header.Set("Authorization", "Bearer abcdefg")
+ req.Header.Set("Content-type", "application/json")
+ resp := s.testRequest(req).Result()
+ c.Check(resp.StatusCode, check.Equals, http.StatusForbidden)
+ var e map[string][]string
+ c.Check(json.NewDecoder(resp.Body).Decode(&e), check.IsNil)
+ c.Check(e["errors"], check.DeepEquals, []string{"invalid API token"})
+}
+
func (s *FederationSuite) TestCreateRemoteContainerRequest(c *check.C) {
defer s.localServiceReturns404(c).Close()
// pass cluster_id via query parameter, this allows arvados-controller
diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go
index 201c5ec02..c20d0e77e 100644
--- a/lib/controller/handler_test.go
+++ b/lib/controller/handler_test.go
@@ -180,8 +180,9 @@ func (s *HandlerSuite) TestProxyRedirect(c *check.C) {
func (s *HandlerSuite) TestValidateV1APIToken(c *check.C) {
req := httptest.NewRequest("GET", "/arvados/v1/users/current", nil)
- user, err := s.handler.(*Handler).validateAPItoken(req, arvadostest.ActiveToken)
+ user, ok, err := s.handler.(*Handler).validateAPItoken(req, arvadostest.ActiveToken)
c.Assert(err, check.IsNil)
+ c.Check(ok, check.Equals, true)
c.Check(user.Authorization.UUID, check.Equals, arvadostest.ActiveTokenUUID)
c.Check(user.Authorization.APIToken, check.Equals, arvadostest.ActiveToken)
c.Check(user.Authorization.Scopes, check.DeepEquals, []string{"all"})
@@ -190,8 +191,9 @@ func (s *HandlerSuite) TestValidateV1APIToken(c *check.C) {
func (s *HandlerSuite) TestValidateV2APIToken(c *check.C) {
req := httptest.NewRequest("GET", "/arvados/v1/users/current", nil)
- user, err := s.handler.(*Handler).validateAPItoken(req, arvadostest.ActiveTokenV2)
+ user, ok, err := s.handler.(*Handler).validateAPItoken(req, arvadostest.ActiveTokenV2)
c.Assert(err, check.IsNil)
+ c.Check(ok, check.Equals, true)
c.Check(user.Authorization.UUID, check.Equals, arvadostest.ActiveTokenUUID)
c.Check(user.Authorization.APIToken, check.Equals, arvadostest.ActiveToken)
c.Check(user.Authorization.Scopes, check.DeepEquals, []string{"all"})
@@ -205,8 +207,9 @@ func (s *HandlerSuite) TestCreateAPIToken(c *check.C) {
c.Assert(err, check.IsNil)
c.Check(auth.Scopes, check.DeepEquals, []string{"all"})
- user, err := s.handler.(*Handler).validateAPItoken(req, auth.TokenV2())
+ user, ok, err := s.handler.(*Handler).validateAPItoken(req, auth.TokenV2())
c.Assert(err, check.IsNil)
+ c.Check(ok, check.Equals, true)
c.Check(user.Authorization.UUID, check.Equals, auth.UUID)
c.Check(user.Authorization.APIToken, check.Equals, auth.APIToken)
c.Check(user.Authorization.Scopes, check.DeepEquals, []string{"all"})
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list