15934: Fix "bad token" error message.
authorTom Clegg <tom@tomclegg.ca>
Thu, 2 Jan 2020 21:18:47 +0000 (16:18 -0500)
committerTom Clegg <tom@tomclegg.ca>
Thu, 2 Jan 2020 21:18:47 +0000 (16:18 -0500)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

lib/controller/fed_containers.go
lib/controller/federation.go
lib/controller/federation_test.go
lib/controller/handler_test.go

index 8bb68d171fc672684be2704373db9030c933559c..a923f757f2eb61afc29d27ee18bfbd42a27a6c1c 100644 (file)
@@ -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
        }
 
index fcff05c003f4723626a736f37f6af2ee6b98321b..674183dcc1d8b9f2d96e5f9b5bded909dad23f26 100644 (file)
@@ -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)
index d0bff84eaf5eb3387450f6e12de28ca6167eeffd..2b0cb22b04fbed0fedcb282c4269dbb008bff1a5 100644 (file)
@@ -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
index 201c5ec02d305cf513b0e734d51f57f98730fef7..c20d0e77ec79db5ce823175137703525fdc30b04 100644 (file)
@@ -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"})