13493: Remove old api_token query param when mangling tokens.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/lib/controller/federation.go b/lib/controller/federation.go
index 3571de62454c0de2fd7eddc0a6aeafe09c0b5816..c1d34be0d6aa13e4508007fe41f00f902a670709 100644 (file)
--- a/lib/controller/federation.go
+++ b/lib/controller/federation.go
@@ -33,6 +33,11 @@ func (h *Handler) proxyRemoteCluster(w http.ResponseWriter, req *http.Request, n
        if scheme == "" {
                scheme = "https"
        }
+       err := h.saltAuthToken(req, remoteID)
+       if err != nil {
+               httpserver.Error(w, err.Error(), http.StatusBadRequest)
+               return
+       }
        urlOut := &url.URL{
                Scheme:   scheme,
                Host:     remote.Host,
@@ -40,11 +45,6 @@ func (h *Handler) proxyRemoteCluster(w http.ResponseWriter, req *http.Request, n
                RawPath:  req.URL.RawPath,
                RawQuery: req.URL.RawQuery,
        }
-       err := h.saltAuthToken(req, remoteID)
-       if err != nil {
-               httpserver.Error(w, err.Error(), http.StatusBadRequest)
-               return
-       }
        client := h.secureClient
        if remote.Insecure {
                client = h.insecureClient
@@ -66,7 +66,7 @@ func (h *Handler) saltAuthToken(req *http.Request, remote string) error {
                }
                // Replace req.Body with a buffer that re-encodes the
                // form without api_token, in case we end up
-               // forwarding the request to RailsAPI.
+               // forwarding the request.
                if req.PostForm != nil {
                        req.PostForm.Del("api_token")
                }
@@ -86,5 +86,14 @@ func (h *Handler) saltAuthToken(req *http.Request, remote string) error {
                return err
        }
        req.Header.Set("Authorization", "Bearer "+token)
+
+       // Remove api_token=... from the the query string, in case we
+       // end up forwarding the request.
+       if values, err := url.ParseQuery(req.URL.RawQuery); err != nil {
+               return err
+       } else if _, ok := values["api_token"]; ok {
+               delete(values, "api_token")
+               req.URL.RawQuery = values.Encode()
+       }
        return nil
 }

diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index 156b8d5f26009f3e35a0968a3aa3e068693b37f5..8dd8e806de575a16dcd1cf27c35d16d8df5cd55e 100644 (file)
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -23,10 +23,12 @@ import (
 var _ = check.Suite(&FederationSuite{})
 
 type FederationSuite struct {
-       log          *logrus.Logger
-       localServer  *httpserver.Server
-       remoteServer *httpserver.Server
-       handler      *Handler
+       log                *logrus.Logger
+       localServer        *httpserver.Server
+       remoteServer       *httpserver.Server
+       remoteMock         *httpserver.Server
+       remoteMockRequests []http.Request
+       handler            *Handler
 }
 
 func (s *FederationSuite) SetUpTest(c *check.C) {
@@ -37,6 +39,10 @@ func (s *FederationSuite) SetUpTest(c *check.C) {
        s.remoteServer = newServerFromIntegrationTestEnv(c)
        c.Assert(s.remoteServer.Start(), check.IsNil)
 
+       s.remoteMock = newServerFromIntegrationTestEnv(c)
+       s.remoteMock.Server.Handler = http.HandlerFunc(s.remoteMockHandler)
+       c.Assert(s.remoteMock.Start(), check.IsNil)
+
        nodeProfile := arvados.NodeProfile{
                Controller: arvados.SystemServiceInstance{Listen: ":"},
                RailsAPI:   arvados.SystemServiceInstance{Listen: ":1"}, // local reqs will error "connection refused"
@@ -49,16 +55,27 @@ func (s *FederationSuite) SetUpTest(c *check.C) {
        }, NodeProfile: &nodeProfile}
        s.localServer = newServerFromIntegrationTestEnv(c)
        s.localServer.Server.Handler = httpserver.AddRequestIDs(httpserver.LogRequests(s.log, s.handler))
+
        s.handler.Cluster.RemoteClusters = map[string]arvados.RemoteCluster{
                "zzzzz": {
                        Host:   s.remoteServer.Addr,
                        Proxy:  true,
                        Scheme: "http",
                },
+               "zmock": {
+                       Host:   s.remoteMock.Addr,
+                       Proxy:  true,
+                       Scheme: "http",
+               },
        }
+
        c.Assert(s.localServer.Start(), check.IsNil)
 }
 
+func (s *FederationSuite) remoteMockHandler(w http.ResponseWriter, req *http.Request) {
+       s.remoteMockRequests = append(s.remoteMockRequests, *req)
+}
+
 func (s *FederationSuite) TearDownTest(c *check.C) {
        if s.remoteServer != nil {
                s.remoteServer.Close()
@@ -144,6 +161,13 @@ func (s *FederationSuite) TestGetRemoteWorkflow(c *check.C) {
        c.Check(wf.OwnerUUID, check.Equals, arvadostest.ActiveUserUUID)
 }
 
+func (s *FederationSuite) TestRemoteWithTokenInQuery(c *check.C) {
+       req := httptest.NewRequest("GET", "/arvados/v1/workflows/"+strings.Replace(arvadostest.WorkflowWithDefinitionYAMLUUID, "zzzzz-", "zmock-", 1)+"?api_token="+arvadostest.ActiveToken, nil)
+       s.handler.ServeHTTP(httptest.NewRecorder(), req)
+       c.Assert(len(s.remoteMockRequests), check.Equals, 1)
+       c.Check(s.remoteMockRequests[0].URL.String(), check.Not(check.Matches), `.*api_token=.*`)
+}
+
 func (s *FederationSuite) TestUpdateRemoteWorkflow(c *check.C) {
        updateDescription := func(descr string) *httptest.ResponseRecorder {
                req := httptest.NewRequest("PATCH", "/arvados/v1/workflows/"+arvadostest.WorkflowWithDefinitionYAMLUUID, strings.NewReader(url.Values{