From: Lucas Di Pentima Date: Tue, 6 Apr 2021 20:46:00 +0000 (-0300) Subject: 16159: Expires tokens on logout on different login controllers. X-Git-Tag: 2.2.0~73^2~3 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/c7c08265ddec9f990c9dce9befa21d555983e43e 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 --- diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go index 4e76b176d9..01fa84ea4f 100644 --- 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 49f557ae5b..3f13c7b27a 100644 --- 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 73b5577237..a435b014d9 100644 --- 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 5d116a9e8f..237f900a83 100644 --- 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 7bb620baa9..9988f6997a 100644 --- 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 e6b6f6c585..e1603f1448 100644 --- 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 }