20831: Revert making IsAdmin and IsInvited a pointer

Now uses API server revision and the requesting user to decide whether
IsAdmin and IsInvited have valid values or not.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 24a7b6f8833a2131cf44eebfb1042b34648cae64..bd5a6d812ea893307fac43cb89a2410adae77e7d 100644 (file)
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -652,7 +652,8 @@ var userAttrsCachedFromLoginCluster = map[string]bool{
 
 func (conn *Conn) batchUpdateUsers(ctx context.Context,
        options arvados.ListOptions,
-       items []arvados.User) (err error) {
+       items []arvados.User,
+       includeAdminAndInvited bool) (err error) {
 
        id := conn.cluster.Login.LoginCluster
        logger := ctxlog.FromContext(ctx)
@@ -699,6 +700,12 @@ func (conn *Conn) batchUpdateUsers(ctx context.Context,
                                }
                        }
                }
+               if !includeAdminAndInvited {
+                       // make sure we don't send these fields.
+                       delete(updates, "is_admin")
+                       delete(updates, "is_invited")
+               }
+               fmt.Printf("updates %v", updates)
                batchOpts.Updates[user.UUID] = updates
        }
        if len(batchOpts.Updates) > 0 {
@@ -711,13 +718,43 @@ func (conn *Conn) batchUpdateUsers(ctx context.Context,
        return nil
 }
 
+func (conn *Conn) includeAdminAndInvitedInBatchUpdate(ctx context.Context, be backend, updateUserUUID string) (bool, error) {
+       // API versions prior to 20231117 would only include the
+       // is_invited and is_admin fields if the current user is an
+       // admin, or is requesting their own user record.  If those
+       // fields aren't actually valid then we don't want to
+       // send them in the batch update.
+       dd, err := be.DiscoveryDocument(ctx)
+       if dd.Revision >= "20231117" {
+               // newer version, fields are valid.
+               return true, nil
+       }
+       selfuser, err := be.UserGetCurrent(ctx, arvados.GetOptions{})
+       if err != nil {
+               // couldn't get our user record
+               return false, err
+       }
+       if selfuser.IsAdmin || selfuser.UUID == updateUserUUID {
+               // we are an admin, or the current user is the same as
+               // the user that we are updating.
+               return true, nil
+       }
+       // Better safe than sorry.
+       return false, nil
+}
+
 func (conn *Conn) UserList(ctx context.Context, options arvados.ListOptions) (arvados.UserList, error) {
        if id := conn.cluster.Login.LoginCluster; id != "" && id != conn.cluster.ClusterID && !options.BypassFederation {
-               resp, err := conn.chooseBackend(id).UserList(ctx, options)
+               be := conn.chooseBackend(id)
+               resp, err := be.UserList(ctx, options)
                if err != nil {
                        return resp, err
                }
-               err = conn.batchUpdateUsers(ctx, options, resp.Items)
+               includeAdminAndInvited, err := conn.includeAdminAndInvitedInBatchUpdate(ctx, be, "")
+               if err != nil {
+                       return arvados.UserList{}, err
+               }
+               err = conn.batchUpdateUsers(ctx, options, resp.Items, includeAdminAndInvited)
                if err != nil {
                        return arvados.UserList{}, err
                }
@@ -734,13 +771,18 @@ func (conn *Conn) UserUpdate(ctx context.Context, options arvados.UpdateOptions)
        if options.BypassFederation {
                return conn.local.UserUpdate(ctx, options)
        }
-       resp, err := conn.chooseBackend(options.UUID).UserUpdate(ctx, options)
+       be := conn.chooseBackend(options.UUID)
+       resp, err := be.UserUpdate(ctx, options)
        if err != nil {
                return resp, err
        }
        if !strings.HasPrefix(options.UUID, conn.cluster.ClusterID) {
+               includeAdminAndInvited, err := conn.includeAdminAndInvitedInBatchUpdate(ctx, be, options.UUID)
+               if err != nil {
+                       return arvados.User{}, err
+               }
                // Copy the updated user record to the local cluster
-               err = conn.batchUpdateUsers(ctx, arvados.ListOptions{}, []arvados.User{resp})
+               err = conn.batchUpdateUsers(ctx, arvados.ListOptions{}, []arvados.User{resp}, includeAdminAndInvited)
                if err != nil {
                        return arvados.User{}, err
                }
@@ -787,7 +829,8 @@ func (conn *Conn) UserUnsetup(ctx context.Context, options arvados.GetOptions) (
 }
 
 func (conn *Conn) UserGet(ctx context.Context, options arvados.GetOptions) (arvados.User, error) {
-       resp, err := conn.chooseBackend(options.UUID).UserGet(ctx, options)
+       be := conn.chooseBackend(options.UUID)
+       resp, err := be.UserGet(ctx, options)
        if err != nil {
                return resp, err
        }
@@ -795,7 +838,11 @@ func (conn *Conn) UserGet(ctx context.Context, options arvados.GetOptions) (arva
                return arvados.User{}, httpErrorf(http.StatusBadGateway, "Had requested %v but response was for %v", options.UUID, resp.UUID)
        }
        if options.UUID[:5] != conn.cluster.ClusterID {
-               err = conn.batchUpdateUsers(ctx, arvados.ListOptions{Select: options.Select}, []arvados.User{resp})
+               includeAdminAndInvited, err := conn.includeAdminAndInvitedInBatchUpdate(ctx, be, options.UUID)
+               if err != nil {
+                       return arvados.User{}, err
+               }
+               err = conn.batchUpdateUsers(ctx, arvados.ListOptions{Select: options.Select}, []arvados.User{resp}, includeAdminAndInvited)
                if err != nil {
                        return arvados.User{}, err
                }

diff --git a/lib/controller/localdb/container_gateway.go b/lib/controller/localdb/container_gateway.go
index 376f55b7b3f65d0be7121d07e0a5e985a33a7c07..0b6a630faea3707a2fdf59164cf2f96e04750644 100644 (file)
--- a/lib/controller/localdb/container_gateway.go
+++ b/lib/controller/localdb/container_gateway.go
@@ -349,7 +349,7 @@ func (conn *Conn) ContainerSSH(ctx context.Context, opts arvados.ContainerSSHOpt
                return sshconn, err
        }
        ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{conn.cluster.SystemRootToken}})
-       if !*user.IsAdmin || !conn.cluster.Containers.ShellAccess.Admin {
+       if !user.IsAdmin || !conn.cluster.Containers.ShellAccess.Admin {
                if !conn.cluster.Containers.ShellAccess.User {
                        return sshconn, httpserver.ErrorWithStatus(errors.New("shell access is disabled in config"), http.StatusServiceUnavailable)
                }

diff --git a/sdk/go/arvados/user.go b/sdk/go/arvados/user.go
index 8dd71f2b72b1fe6226b6af31f9b9c960ac3dd858..2fb061e7fb818840fc8e1c42ae10e771c45e1ee5 100644 (file)
--- a/sdk/go/arvados/user.go
+++ b/sdk/go/arvados/user.go
@@ -11,14 +11,14 @@ type User struct {
        UUID                 string                 `json:"uuid"`
        Etag                 string                 `json:"etag"`
        IsActive             bool                   `json:"is_active"`
-       IsAdmin              *bool                  `json:"is_admin,omitempty"`
+       IsAdmin              bool                   `json:"is_admin"`
        Username             string                 `json:"username"`
        Email                string                 `json:"email"`
        FullName             string                 `json:"full_name"`
        FirstName            string                 `json:"first_name"`
        LastName             string                 `json:"last_name"`
        IdentityURL          string                 `json:"identity_url"`
-       IsInvited            *bool                  `json:"is_invited,omitempty"`
+       IsInvited            bool                   `json:"is_invited"`
        OwnerUUID            string                 `json:"owner_uuid"`
        CreatedAt            time.Time              `json:"created_at"`
        ModifiedAt           time.Time              `json:"modified_at"`

diff --git a/services/keep-balance/balance.go b/services/keep-balance/balance.go
index 0b865d74757aa9a1627bafd75e337ef193a6f07e..e44dfeda8748aec51e18cd55118c15d509d8fc12 100644 (file)
--- a/services/keep-balance/balance.go
+++ b/services/keep-balance/balance.go
@@ -267,7 +267,7 @@ func (bal *Balancer) CheckSanityEarly(c *arvados.Client) error {
        if err != nil {
                return fmt.Errorf("CurrentUser(): %v", err)
        }
-       if !u.IsActive || !*u.IsAdmin {
+       if !u.IsActive || !u.IsAdmin {
                return fmt.Errorf("current user (%s) is not an active admin user", u.UUID)
        }
        for _, srv := range bal.KeepServices {

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index b5e0e7e896d6ff34edee3c6e9c7d0b653499d947..123c4fe34da3d1dba8ae504b9867a410cd5022b5 100644 (file)
--- a/services/keep-web/handler.go
+++ b/services/keep-web/handler.go
@@ -851,7 +851,7 @@ func (h *handler) seeOtherWithCookie(w http.ResponseWriter, r *http.Request, loc
 func (h *handler) userPermittedToUploadOrDownload(method string, tokenUser *arvados.User) bool {
        var permitDownload bool
        var permitUpload bool
-       if tokenUser != nil && tokenUser.IsAdmin != nil && *tokenUser.IsAdmin {
+       if tokenUser != nil && tokenUser.IsAdmin {
                permitUpload = h.Cluster.Collections.WebDAVPermission.Admin.Upload
                permitDownload = h.Cluster.Collections.WebDAVPermission.Admin.Download
        } else {

diff --git a/services/keepproxy/keepproxy.go b/services/keepproxy/keepproxy.go
index 964eaa7bdf442f5d08f1cb681930c2394e2cafaa..2090c506869d69419ffff32d14abf0aa7bfd58fd 100644 (file)
--- a/services/keepproxy/keepproxy.go
+++ b/services/keepproxy/keepproxy.go
@@ -151,7 +151,7 @@ func (h *proxyHandler) checkAuthorizationHeader(req *http.Request) (pass bool, t
                return false, "", nil
        }
 
-       if userCurrentError == nil && *user.IsAdmin {
+       if userCurrentError == nil && user.IsAdmin {
                // checking userCurrentError is probably redundant,
                // IsAdmin would be false anyway. But can't hurt.
                if op == "read" && !h.cluster.Collections.KeepproxyPermission.Admin.Download {