16159: Expires tokens on logout on different login controllers.

Also, optimizes the DB query a bit.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas@di-pentima.com.ar>

diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go
index 4e76b176d99a9c7846a3c0cae55c37e13c031573..01fa84ea4fe885ba59e7f56099cdfb41d21e2a8c 100644 (file)
--- a/lib/controller/localdb/login.go
+++ b/lib/controller/localdb/login.go
@@ -124,25 +124,13 @@ type federatedLoginController struct {
 func (ctrl federatedLoginController) Login(context.Context, arvados.LoginOptions) (arvados.LoginResponse, error) {
        return arvados.LoginResponse{}, httpserver.ErrorWithStatus(errors.New("Should have been redirected to login cluster"), http.StatusBadRequest)
 }
-func (ctrl federatedLoginController) Logout(_ context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
-       return noopLogout(ctrl.Cluster, opts)
+func (ctrl federatedLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
+       return logout(ctx, ctrl.Cluster, opts)
 }
 func (ctrl federatedLoginController) UserAuthenticate(context.Context, arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) {
        return arvados.APIClientAuthorization{}, httpserver.ErrorWithStatus(errors.New("username/password authentication is not available"), http.StatusBadRequest)
 }
 
-func noopLogout(cluster *arvados.Cluster, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
-       target := opts.ReturnTo
-       if target == "" {
-               if cluster.Services.Workbench2.ExternalURL.Host != "" {
-                       target = cluster.Services.Workbench2.ExternalURL.String()
-               } else {
-                       target = cluster.Services.Workbench1.ExternalURL.String()
-               }
-       }
-       return arvados.LogoutResponse{RedirectLocation: target}, nil
-}
-
 func (conn *Conn) CreateAPIClientAuthorization(ctx context.Context, rootToken string, authinfo rpc.UserSessionAuthInfo) (resp arvados.APIClientAuthorization, err error) {
        if rootToken == "" {
                return arvados.APIClientAuthorization{}, errors.New("configuration error: empty SystemRootToken")

diff --git a/lib/controller/localdb/login_ldap.go b/lib/controller/localdb/login_ldap.go
index 49f557ae5b9ce50a8f7c3ceb59cc0fb31b50e187..3f13c7b27aafed09f1c6b201270e034ca9c011d9 100644 (file)
--- a/lib/controller/localdb/login_ldap.go
+++ b/lib/controller/localdb/login_ldap.go
@@ -26,7 +26,7 @@ type ldapLoginController struct {
 }
 
 func (ctrl *ldapLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
-       return noopLogout(ctrl.Cluster, opts)
+       return logout(ctx, ctrl.Cluster, opts)
 }
 
 func (ctrl *ldapLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) {

diff --git a/lib/controller/localdb/login_oidc.go b/lib/controller/localdb/login_oidc.go
index 73b557723752556d4c6071e2d186f4a2a495e65d..a435b014d967deafae3a72060809a3e843ecc975 100644 (file)
--- a/lib/controller/localdb/login_oidc.go
+++ b/lib/controller/localdb/login_oidc.go
@@ -98,7 +98,7 @@ func (ctrl *oidcLoginController) setup() error {
 }
 
 func (ctrl *oidcLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
-       return noopLogout(ctrl.Cluster, opts)
+       return logout(ctx, ctrl.Cluster, opts)
 }
 
 func (ctrl *oidcLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) {

diff --git a/lib/controller/localdb/login_pam.go b/lib/controller/localdb/login_pam.go
index 5d116a9e8f490be37225d0e3333ab6413c79e979..237f900a83458b61e695ffe7e6808820419c2cea 100644 (file)
--- a/lib/controller/localdb/login_pam.go
+++ b/lib/controller/localdb/login_pam.go
@@ -25,7 +25,7 @@ type pamLoginController struct {
 }
 
 func (ctrl *pamLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
-       return noopLogout(ctrl.Cluster, opts)
+       return logout(ctx, ctrl.Cluster, opts)
 }
 
 func (ctrl *pamLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) {

diff --git a/lib/controller/localdb/login_testuser.go b/lib/controller/localdb/login_testuser.go
index 7bb620baa9e5083adf882caae40678c16bb96c23..9988f6997a1b6f03ceb1d46505bbd4b84e7acee2 100644 (file)
--- a/lib/controller/localdb/login_testuser.go
+++ b/lib/controller/localdb/login_testuser.go
@@ -7,15 +7,12 @@ package localdb
 import (
        "bytes"
        "context"
-       "errors"
        "fmt"
        "html/template"
-       "net/http"
 
        "git.arvados.org/arvados.git/lib/controller/rpc"
        "git.arvados.org/arvados.git/sdk/go/arvados"
        "git.arvados.org/arvados.git/sdk/go/ctxlog"
-       "git.arvados.org/arvados.git/sdk/go/httpserver"
        "github.com/sirupsen/logrus"
 )
 
@@ -25,12 +22,7 @@ type testLoginController struct {
 }
 
 func (ctrl *testLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
-       err := ctrl.Parent.expireAPIClientAuthorization(ctx)
-       if err != nil {
-               ctxlog.FromContext(ctx).Errorf("attempting to expire token on logout: %q", err)
-               return arvados.LogoutResponse{}, httpserver.ErrorWithStatus(errors.New("could not expire token on logout"), http.StatusInternalServerError)
-       }
-       return noopLogout(ctrl.Cluster, opts)
+       return logout(ctx, ctrl.Cluster, opts)
 }
 
 func (ctrl *testLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) {

diff --git a/lib/controller/localdb/logout.go b/lib/controller/localdb/logout.go
index e6b6f6c585c008c49eedd97f8ec5603db6d632e7..e1603f14485eb0a4f664a2a00b080a1497c64d2a 100644 (file)
--- a/lib/controller/localdb/logout.go
+++ b/lib/controller/localdb/logout.go
@@ -9,17 +9,40 @@ import (
        "database/sql"
        "errors"
        "fmt"
+       "net/http"
        "strings"
 
        "git.arvados.org/arvados.git/lib/ctrlctx"
+       "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"
 )
 
-func (conn *Conn) expireAPIClientAuthorization(ctx context.Context) error {
+func logout(ctx context.Context, cluster *arvados.Cluster, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) {
+       err := expireAPIClientAuthorization(ctx)
+       if err != nil {
+               ctxlog.FromContext(ctx).Errorf("attempting to expire token on logout: %q", err)
+               return arvados.LogoutResponse{}, httpserver.ErrorWithStatus(errors.New("could not expire token on logout"), http.StatusInternalServerError)
+       }
+
+       target := opts.ReturnTo
+       if target == "" {
+               if cluster.Services.Workbench2.ExternalURL.Host != "" {
+                       target = cluster.Services.Workbench2.ExternalURL.String()
+               } else {
+                       target = cluster.Services.Workbench1.ExternalURL.String()
+               }
+       }
+       return arvados.LogoutResponse{RedirectLocation: target}, nil
+}
+
+func expireAPIClientAuthorization(ctx context.Context) error {
        creds, ok := auth.FromContext(ctx)
        if !ok {
-               return errors.New("credentials not found from context")
+               // Tests could be passing empty contexts
+               ctxlog.FromContext(ctx).Debugf("expireAPIClientAuthorization: credentials not found from context")
+               return nil
        }
 
        if len(creds.Tokens) == 0 {
@@ -52,13 +75,14 @@ func (conn *Conn) expireAPIClientAuthorization(ctx context.Context) error {
                ctxlog.FromContext(ctx).WithError(err).Debugf("expireAPIClientAuthorization(%s): database error", token)
                return err
        }
+
        if tokenUuid != "" && retrievedUuid != tokenUuid {
                // secret part matches, but UUID doesn't -- somewhat surprising
                ctxlog.FromContext(ctx).Debugf("expireAPIClientAuthorization(%s): secret part found, but with different UUID: %s", tokenSecret, retrievedUuid)
                return nil
        }
 
-       res, err := tx.ExecContext(ctx, "UPDATE api_client_authorizations SET expires_at=current_timestamp AT TIME ZONE 'UTC' WHERE api_token=$1 AND (expires_at IS NULL OR expires_at > current_timestamp AT TIME ZONE 'UTC')", tokenSecret)
+       res, err := tx.ExecContext(ctx, "UPDATE api_client_authorizations SET expires_at=current_timestamp AT TIME ZONE 'UTC' WHERE uuid=$1", retrievedUuid)
        if err != nil {
                return err
        }