16212: Move user/pass authentication to its own endpoint.

Overloading the /login endpoint turns out to be too awkward: method,
CORS permissions, and response type are all different.

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

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 279b7a51d5d8d4e57f920721db10ace45268b1d2..3e37a5c618429833ea2874ac14cacc7d98b3efa9 100644 (file)
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -478,6 +478,10 @@ func (conn *Conn) UserBatchUpdate(ctx context.Context, options arvados.UserBatch
        return conn.local.UserBatchUpdate(ctx, options)
 }
 
+func (conn *Conn) UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
+       return conn.local.UserAuthenticate(ctx, options)
+}
+
 func (conn *Conn) APIClientAuthorizationCurrent(ctx context.Context, options arvados.GetOptions) (arvados.APIClientAuthorization, error) {
        return conn.chooseBackend(options.UUID).APIClientAuthorizationCurrent(ctx, options)
 }

diff --git a/lib/controller/federation/login_test.go b/lib/controller/federation/login_test.go
index 1d6e12e0159f5c73d45ebfda595a10587580a9b5..2de260fdc2493a30857894a85ebef22e7d898670 100644 (file)
--- a/lib/controller/federation/login_test.go
+++ b/lib/controller/federation/login_test.go
@@ -46,6 +46,9 @@ func (s *LoginSuite) TestLogout(c *check.C) {
        s.cluster.Login.GoogleClientID = "zzzzzzzzzzzzzz"
        s.addHTTPRemote(c, "zhome", &arvadostest.APIStub{})
        s.cluster.Login.LoginCluster = "zhome"
+       // s.fed is already set by SetUpTest, but we need to
+       // reinitialize with the above config changes.
+       s.fed = New(s.cluster)
 
        returnTo := "https://app.example.com/foo?bar"
        for _, trial := range []struct {

diff --git a/lib/controller/handler.go b/lib/controller/handler.go
index e3869261a160b4e42d147574410f15b9641b4e9d..d7bc9bd9a2468f4f54b205f9ff1f0618d4bdca8e 100644 (file)
--- a/lib/controller/handler.go
+++ b/lib/controller/handler.go
@@ -85,6 +85,7 @@ func (h *Handler) setup() {
                mux.Handle("/arvados/v1/collections/", rtr)
                mux.Handle("/arvados/v1/users", rtr)
                mux.Handle("/arvados/v1/users/", rtr)
+               mux.Handle("/"+arvados.EndpointUserAuthenticate.Path, rtr)
                mux.Handle("/login", rtr)
                mux.Handle("/logout", rtr)
        }

diff --git a/lib/controller/localdb/conn.go b/lib/controller/localdb/conn.go
index 909b6e1ff33a8489bae3a13a4839f902cb2d5a44..60263455bdb1d02c10a9164c7c235d22a0f90fb7 100644 (file)
--- a/lib/controller/localdb/conn.go
+++ b/lib/controller/localdb/conn.go
@@ -36,3 +36,7 @@ func (conn *Conn) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvad
 func (conn *Conn) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) {
        return conn.loginController.Login(ctx, opts)
 }
+
+func (conn *Conn) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
+       return conn.loginController.UserAuthenticate(ctx, opts)
+}

diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go
index af9a0348274c672bed8edcf673a13c5b9486b7a0..2d20531714b242a208909c6b3ffe4b40a30a646d 100644 (file)
--- a/lib/controller/localdb/login.go
+++ b/lib/controller/localdb/login.go
@@ -14,6 +14,7 @@ import (
 type loginController interface {
        Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error)
        Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error)
+       UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error)
 }
 
 func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) loginController {
@@ -42,6 +43,9 @@ func (ctrl errorLoginController) Login(context.Context, arvados.LoginOptions) (a
 func (ctrl errorLoginController) Logout(context.Context, arvados.LogoutOptions) (arvados.LogoutResponse, error) {
        return arvados.LogoutResponse{}, ctrl.error
 }
+func (ctrl errorLoginController) UserAuthenticate(context.Context, arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
+       return arvados.APIClientAuthorization{}, ctrl.error
+}
 
 func noopLogout(cluster *arvados.Cluster, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
        target := opts.ReturnTo

diff --git a/lib/controller/localdb/login_google.go b/lib/controller/localdb/login_google.go
index 61bbaf01b12f73949dd4965afddf20f83a4aaede..bf1754c158968e40c6cb7d32a31d55ab4c83fcde 100644 (file)
--- a/lib/controller/localdb/login_google.go
+++ b/lib/controller/localdb/login_google.go
@@ -12,6 +12,7 @@ import (
        "encoding/base64"
        "errors"
        "fmt"
+       "net/http"
        "net/url"
        "strings"
        "sync"
@@ -22,6 +23,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/coreos/go-oidc"
        "golang.org/x/oauth2"
        "google.golang.org/api/option"
@@ -129,6 +131,10 @@ func (ctrl *googleLoginController) Login(ctx context.Context, opts arvados.Login
        }
 }
 
+func (ctrl *googleLoginController) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
+       return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(errors.New("username/password authentication is not available"), http.StatusBadRequest)
+}
+
 // Use a person's token to get all of their email addresses, with the
 // primary address at index 0. The provided defaultAddr is always
 // included in the returned slice, and is used as the primary if the

diff --git a/lib/controller/localdb/login_pam.go b/lib/controller/localdb/login_pam.go
index 059e27fa26ef1b9a44d3ceb9cf687b9e4831aaf7..be90cb3d1463bd45db7463af057ab15babd2fa55 100644 (file)
--- a/lib/controller/localdb/login_pam.go
+++ b/lib/controller/localdb/login_pam.go
@@ -31,6 +31,10 @@ func (ctrl *pamLoginController) Logout(ctx context.Context, opts arvados.LogoutO
 }
 
 func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) {
+       return arvados.LoginResponse{}, errors.New("interactive login is not available")
+}
+
+func (ctrl *pamLoginController) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
        errorMessage := ""
        tx, err := pam.StartFunc(ctrl.Cluster.Login.PAMService, opts.Username, func(style pam.Style, message string) (string, error) {
                ctxlog.FromContext(ctx).Debugf("pam conversation: style=%v message=%q", style, message)
@@ -49,18 +53,18 @@ func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOpt
                }
        })
        if err != nil {
-               return arvados.LoginResponse{}, err
+               return arvados.APIClientAuthorization{}, err
        }
        err = tx.Authenticate(pam.DisallowNullAuthtok)
        if err != nil {
-               return arvados.LoginResponse{}, httpserver.ErrorWithStatus(err, http.StatusUnauthorized)
+               return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(err, http.StatusUnauthorized)
        }
        if errorMessage != "" {
-               return arvados.LoginResponse{}, httpserver.ErrorWithStatus(errors.New(errorMessage), http.StatusUnauthorized)
+               return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(errors.New(errorMessage), http.StatusUnauthorized)
        }
        user, err := tx.GetItem(pam.User)
        if err != nil {
-               return arvados.LoginResponse{}, err
+               return arvados.APIClientAuthorization{}, err
        }
        email := user
        if domain := ctrl.Cluster.Login.PAMDefaultEmailDomain; domain != "" && !strings.Contains(email, "@") {
@@ -72,20 +76,18 @@ func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOpt
                // Send a fake ReturnTo value instead of the caller's
                // opts.ReturnTo. We won't follow the resulting
                // redirect target anyway.
-               ReturnTo: opts.Remote + ",https://none.invalid",
+               ReturnTo: ",https://none.invalid",
                AuthInfo: rpc.UserSessionAuthInfo{
                        Username: user,
                        Email:    email,
                },
        })
        if err != nil {
-               return arvados.LoginResponse{}, err
+               return arvados.APIClientAuthorization{}, err
        }
        target, err := url.Parse(resp.RedirectLocation)
        if err != nil {
-               return arvados.LoginResponse{}, err
+               return arvados.APIClientAuthorization{}, err
        }
-       resp.Token = target.Query().Get("api_token")
-       resp.RedirectLocation = ""
-       return resp, err
+       return arvados.APIClientAuthorization{APIToken: target.Query().Get("api_token")}, err
 }

diff --git a/lib/controller/localdb/login_pam_docker_test.sh b/lib/controller/localdb/login_pam_docker_test.sh
index 1d37f5c1c9e7d4250642cfecff1b762ad353351a..c71de47a45f9ca15f15aa33f7b3d0644758c99fa 100755 (executable)
--- a/lib/controller/localdb/login_pam_docker_test.sh
+++ b/lib/controller/localdb/login_pam_docker_test.sh
@@ -170,13 +170,13 @@ check_contains() {
 }
 
 echo >&2 "Testing authentication failure"
-resp="$(curl -s --include -H "X-Http-Method-Override: GET" -d username=foo -d password=nosecret "http://${ctrlhostport}/login" | tee $debug)"
+resp="$(curl -s --include -d username=foo -d password=nosecret "http://${ctrlhostport}/arvados/v1/users/authenticate" | tee $debug)"
 check_contains "${resp}" "HTTP/1.1 401"
 check_contains "${resp}" '{"errors":["Authentication failure"]}'
 
 echo >&2 "Testing authentication success"
-resp="$(curl -s --include -H "X-Http-Method-Override: GET" -d username=foo -d password=secret "http://${ctrlhostport}/login" | tee $debug)"
+resp="$(curl -s --include -d username=foo -d password=secret "http://${ctrlhostport}/arvados/v1/users/authenticate" | tee $debug)"
 check_contains "${resp}" "HTTP/1.1 200"
-check_contains "${resp}" '{"token":"v2/zzzzz-gj3su-'
+check_contains "${resp}" '{"api_token":"v2/zzzzz-gj3su-'
 
 cleanup

diff --git a/lib/controller/localdb/login_pam_test.go b/lib/controller/localdb/login_pam_test.go
index 4af40f8d38fb4ebe8cec8b98eb6d457ca193fb63..4ecead8b15d816098303bb5e80965ef243f23a82 100644 (file)
--- a/lib/controller/localdb/login_pam_test.go
+++ b/lib/controller/localdb/login_pam_test.go
@@ -42,20 +42,16 @@ func (s *PamSuite) SetUpSuite(c *check.C) {
 }
 
 func (s *PamSuite) TestLoginFailure(c *check.C) {
-       resp, err := s.ctrl.Login(context.Background(), arvados.LoginOptions{
+       resp, err := s.ctrl.UserAuthenticate(context.Background(), arvados.UserAuthenticateOptions{
                Username: "bogususername",
                Password: "boguspassword",
-               ReturnTo: "https://example.com/foo",
        })
        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, "")
-       c.Check(resp.HTML.String(), check.Equals, "")
+       c.Check(resp.APIToken, check.Equals, "")
 }
 
 // This test only runs if the ARVADOS_TEST_PAM_CREDENTIALS_FILE env
@@ -73,15 +69,12 @@ func (s *PamSuite) TestLoginSuccess(c *check.C) {
        c.Assert(len(lines), check.Equals, 2, check.Commentf("credentials file %s should contain \"username\\npassword\"", testCredsFile))
        u, p := lines[0], lines[1]
 
-       resp, err := s.ctrl.Login(context.Background(), arvados.LoginOptions{
+       resp, err := s.ctrl.UserAuthenticate(context.Background(), arvados.UserAuthenticateOptions{
                Username: u,
                Password: p,
-               ReturnTo: "https://example.com/foo",
        })
        c.Check(err, check.IsNil)
-       c.Check(resp.RedirectLocation, check.Equals, "")
-       c.Check(resp.Token, check.Matches, `v2/zzzzz-gj3su-.*/.*`)
-       c.Check(resp.HTML.String(), check.Equals, "")
+       c.Check(resp.APIToken, check.Matches, `v2/zzzzz-gj3su-.*/.*`)
 
        authinfo := getCallbackAuthInfo(c, s.railsSpy)
        c.Check(authinfo.Email, check.Equals, u+"@"+s.cluster.Login.PAMDefaultEmailDomain)

diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go
index bb6d9055852a4b94347ed7c5b89b364d3823f6be..59205788dddd94f0ca6a25086c41bb5f973d5f7e 100644 (file)
--- a/lib/controller/router/router.go
+++ b/lib/controller/router/router.go
@@ -303,6 +303,13 @@ func (rtr *router) addRoutes() {
                                return rtr.fed.UserDelete(ctx, *opts.(*arvados.DeleteOptions))
                        },
                },
+               {
+                       arvados.EndpointUserAuthenticate,
+                       func() interface{} { return &arvados.UserAuthenticateOptions{} },
+                       func(ctx context.Context, opts interface{}) (interface{}, error) {
+                               return rtr.fed.UserAuthenticate(ctx, *opts.(*arvados.UserAuthenticateOptions))
+                       },
+               },
        } {
                rtr.addRoute(route.endpoint, route.defaultOpts, route.exec)
        }

diff --git a/lib/controller/rpc/conn.go b/lib/controller/rpc/conn.go
index 4b143b770bbbe093bbec7c668f58e2f0100f81a6..c3c66d00a346d9cea1fc07c4add68766481986a1 100644 (file)
--- a/lib/controller/rpc/conn.go
+++ b/lib/controller/rpc/conn.go
@@ -417,8 +417,15 @@ func (conn *Conn) UserSessionCreate(ctx context.Context, options UserSessionCrea
 }
 
 func (conn *Conn) UserBatchUpdate(ctx context.Context, options arvados.UserBatchUpdateOptions) (arvados.UserList, error) {
-       ep := arvados.APIEndpoint{Method: "PATCH", Path: "arvados/v1/users/batch_update"}
+       ep := arvados.EndpointUserBatchUpdate
        var resp arvados.UserList
        err := conn.requestAndDecode(ctx, &resp, ep, nil, options)
        return resp, err
 }
+
+func (conn *Conn) UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
+       ep := arvados.EndpointUserAuthenticate
+       var resp arvados.APIClientAuthorization
+       err := conn.requestAndDecode(ctx, &resp, ep, nil, options)
+       return resp, err
+}

diff --git a/sdk/go/arvados/api.go b/sdk/go/arvados/api.go
index ff0dcf75a7ee8dfb37b38222e53fae1a59f2ef77..cec6e26b6dc8b4f1b9996cc23f30632f12ca850b 100644 (file)
--- a/sdk/go/arvados/api.go
+++ b/sdk/go/arvados/api.go
@@ -56,6 +56,7 @@ var (
        EndpointUserUpdate                    = APIEndpoint{"PATCH", "arvados/v1/users/{uuid}", "user"}
        EndpointUserUpdateUUID                = APIEndpoint{"POST", "arvados/v1/users/{uuid}/update_uuid", ""}
        EndpointUserBatchUpdate               = APIEndpoint{"PATCH", "arvados/v1/users/batch", ""}
+       EndpointUserAuthenticate              = APIEndpoint{"POST", "arvados/v1/users/authenticate", ""}
        EndpointAPIClientAuthorizationCurrent = APIEndpoint{"GET", "arvados/v1/api_client_authorizations/current", ""}
 )
 
@@ -136,10 +137,13 @@ type DeleteOptions struct {
 }
 
 type LoginOptions struct {
-       ReturnTo string `json:"return_to"`          // On success, redirect to this target with api_token=xxx query param
-       Remote   string `json:"remote,omitempty"`   // Salt token for remote Cluster ID
-       Code     string `json:"code,omitempty"`     // OAuth2 callback code
-       State    string `json:"state,omitempty"`    // OAuth2 callback state
+       ReturnTo string `json:"return_to"`        // On success, redirect to this target with api_token=xxx query param
+       Remote   string `json:"remote,omitempty"` // Salt token for remote Cluster ID
+       Code     string `json:"code,omitempty"`   // OAuth2 callback code
+       State    string `json:"state,omitempty"`  // OAuth2 callback state
+}
+
+type UserAuthenticateOptions struct {
        Username string `json:"username,omitempty"` // PAM username
        Password string `json:"password,omitempty"` // PAM password
 }
@@ -186,5 +190,6 @@ type API interface {
        UserList(ctx context.Context, options ListOptions) (UserList, error)
        UserDelete(ctx context.Context, options DeleteOptions) (User, error)
        UserBatchUpdate(context.Context, UserBatchUpdateOptions) (UserList, error)
+       UserAuthenticate(ctx context.Context, options UserAuthenticateOptions) (APIClientAuthorization, error)
        APIClientAuthorizationCurrent(ctx context.Context, options GetOptions) (APIClientAuthorization, error)
 }

diff --git a/sdk/go/arvados/login.go b/sdk/go/arvados/login.go
index 26c04b2c13603a2a7c9aa721b38a1fd5343c5228..9178cc6a272dd9d2f7cc56b0ea293f13fa9f050a 100644 (file)
--- a/sdk/go/arvados/login.go
+++ b/sdk/go/arvados/login.go
@@ -6,14 +6,11 @@ package arvados
 
 import (
        "bytes"
-       "encoding/json"
        "net/http"
 )
 
 type LoginResponse struct {
        RedirectLocation string       `json:"redirect_location,omitempty"`
-       Token            string       `json:"token,omitempty"`
-       Message          string       `json:"message,omitempty"`
        HTML             bytes.Buffer `json:"-"`
 }
 
@@ -22,12 +19,6 @@ func (resp LoginResponse) ServeHTTP(w http.ResponseWriter, req *http.Request) {
        if resp.RedirectLocation != "" {
                w.Header().Set("Location", resp.RedirectLocation)
                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")
                w.Write(resp.HTML.Bytes())

diff --git a/sdk/go/arvadostest/api.go b/sdk/go/arvadostest/api.go
index 9019d33cfb8bc167c45cf5e4c1b2fa4107d0c9c3..fa5f53936028504b9dd8f4bcc41f1304dd36656e 100644 (file)
--- a/sdk/go/arvadostest/api.go
+++ b/sdk/go/arvadostest/api.go
@@ -177,6 +177,10 @@ func (as *APIStub) UserBatchUpdate(ctx context.Context, options arvados.UserBatc
        as.appendCall(as.UserBatchUpdate, ctx, options)
        return arvados.UserList{}, as.Error
 }
+func (as *APIStub) UserAuthenticate(ctx context.Context, options arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
+       as.appendCall(as.UserAuthenticate, ctx, options)
+       return arvados.APIClientAuthorization{}, as.Error
+}
 func (as *APIStub) APIClientAuthorizationCurrent(ctx context.Context, options arvados.GetOptions) (arvados.APIClientAuthorization, error) {
        as.appendCall(as.APIClientAuthorizationCurrent, ctx, options)
        return arvados.APIClientAuthorization{}, as.Error