15720: Batch user update API.

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

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 47ae0ba9ec3ab4c59aa09191d884fdc973345a55..1d8fa7e462cf96480adba03d7d814b1c246911c6 100644 (file)
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -15,6 +15,7 @@ import (
        "net/url"
        "regexp"
        "strings"
+       "time"
 
        "git.curoverse.com/arvados.git/lib/config"
        "git.curoverse.com/arvados.git/lib/controller/localdb"
@@ -350,12 +351,25 @@ func (conn *Conn) UserList(ctx context.Context, options arvados.ListOptions) (ar
                if err != nil {
                        return resp, err
                }
-               ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{conn.cluster.SystemRootToken}})
+               batchOpts := arvados.UserBatchUpdateOptions{Updates: map[string]map[string]interface{}{}}
                for _, user := range resp.Items {
                        if !strings.HasPrefix(user.UUID, id) {
                                continue
                        }
-                       logger.Debug("cache user info for uuid %q", user.UUID)
+                       logger.Debugf("cache user info for uuid %q", user.UUID)
+
+                       // If the remote cluster has null timestamps
+                       // (e.g., test server with incomplete
+                       // fixtures) use dummy timestamps (instead of
+                       // the zero time, which causes a Rails API
+                       // error "year too big to marshal: 1 UTC").
+                       if user.ModifiedAt.IsZero() {
+                               user.ModifiedAt = time.Now()
+                       }
+                       if user.CreatedAt.IsZero() {
+                               user.CreatedAt = time.Now()
+                       }
+
                        var allFields map[string]interface{}
                        buf, err := json.Marshal(user)
                        if err != nil {
@@ -380,19 +394,13 @@ func (conn *Conn) UserList(ctx context.Context, options arvados.ListOptions) (ar
                                        }
                                }
                        }
-                       _, err = conn.local.UserUpdate(ctxRoot, arvados.UpdateOptions{
-                               UUID:  user.UUID,
-                               Attrs: updates,
-                       })
-                       if errStatus(err) == http.StatusNotFound {
-                               updates["uuid"] = user.UUID
-                               _, err = conn.local.UserCreate(ctxRoot, arvados.CreateOptions{
-                                       Attrs: updates,
-                               })
-                       }
+                       batchOpts.Updates[user.UUID] = updates
+               }
+               if len(batchOpts.Updates) > 0 {
+                       ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{conn.cluster.SystemRootToken}})
+                       _, err = conn.local.UserBatchUpdate(ctxRoot, batchOpts)
                        if err != nil {
-                               logger.WithError(err).WithField("UUID", user.UUID).Error("error updating local user record")
-                               return arvados.UserList{}, fmt.Errorf("error updating local user record: %s", err)
+                               return arvados.UserList{}, fmt.Errorf("error updating local user records: %s", err)
                        }
                }
                return resp, nil
@@ -445,6 +453,10 @@ func (conn *Conn) UserDelete(ctx context.Context, options arvados.DeleteOptions)
        return conn.chooseBackend(options.UUID).UserDelete(ctx, options)
 }
 
+func (conn *Conn) UserBatchUpdate(ctx context.Context, options arvados.UserBatchUpdateOptions) (arvados.UserList, error) {
+       return conn.local.UserBatchUpdate(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/user_test.go b/lib/controller/federation/user_test.go
index 993be9b0b836707ecc2361c4e2f6bad3e7372d37..8202a668ff0e8600d9ccdaaa9e3b53ec9492fe92 100644 (file)
--- a/lib/controller/federation/user_test.go
+++ b/lib/controller/federation/user_test.go
@@ -53,7 +53,7 @@ func (s *UserSuite) TestLoginClusterUserList(c *check.C) {
                                c.Check(stub.Calls(nil), check.HasLen, 0)
                        } else if updateFail {
                                c.Logf("... err %#v", err)
-                               calls := stub.Calls(stub.UserUpdate)
+                               calls := stub.Calls(stub.UserBatchUpdate)
                                if c.Check(calls, check.HasLen, 1) {
                                        c.Logf("... stub.UserUpdate called with options: %#v", calls[0].Options)
                                        shouldUpdate := map[string]bool{
@@ -84,8 +84,11 @@ func (s *UserSuite) TestLoginClusterUserList(c *check.C) {
                                                        }
                                                }
                                        }
+                                       var uuid string
+                                       for uuid = range calls[0].Options.(arvados.UserBatchUpdateOptions).Updates {
+                                       }
                                        for k, shouldFind := range shouldUpdate {
-                                               _, found := calls[0].Options.(arvados.UpdateOptions).Attrs[k]
+                                               _, found := calls[0].Options.(arvados.UserBatchUpdateOptions).Updates[uuid][k]
                                                c.Check(found, check.Equals, shouldFind, check.Commentf("offending attr: %s", k))
                                        }
                                }
@@ -93,13 +96,13 @@ func (s *UserSuite) TestLoginClusterUserList(c *check.C) {
                                updates := 0
                                for _, d := range spy.RequestDumps {
                                        d := string(d)
-                                       if strings.Contains(d, "PATCH /arvados/v1/users/zzzzz-tpzed-") {
+                                       if strings.Contains(d, "PATCH /arvados/v1/users/batch") {
                                                c.Check(d, check.Matches, `(?ms).*Authorization: Bearer `+arvadostest.SystemRootToken+`.*`)
                                                updates++
                                        }
                                }
                                c.Check(err, check.IsNil)
-                               c.Check(updates, check.Equals, len(userlist.Items))
+                               c.Check(updates, check.Equals, 1)
                                c.Logf("... response items %#v", userlist.Items)
                        }
                }

diff --git a/lib/controller/router/router.go b/lib/controller/router/router.go
index 250f3cb45bce88137050fe73ca3fff52e17e40f7..47082197a01316f4db874b0989c836e7d4a0850f 100644 (file)
--- a/lib/controller/router/router.go
+++ b/lib/controller/router/router.go
@@ -282,6 +282,13 @@ func (rtr *router) addRoutes() {
                                return rtr.fed.UserList(ctx, *opts.(*arvados.ListOptions))
                        },
                },
+               {
+                       arvados.EndpointUserBatchUpdate,
+                       func() interface{} { return &arvados.UserBatchUpdateOptions{} },
+                       func(ctx context.Context, opts interface{}) (interface{}, error) {
+                               return rtr.fed.UserBatchUpdate(ctx, *opts.(*arvados.UserBatchUpdateOptions))
+                       },
+               },
                {
                        arvados.EndpointUserDelete,
                        func() interface{} { return &arvados.DeleteOptions{} },

diff --git a/lib/controller/rpc/conn.go b/lib/controller/rpc/conn.go
index 25efcfd43948ad6331d9b71c2001950241c35902..66523e5ac3203215de84795f76db632083451372 100644 (file)
--- a/lib/controller/rpc/conn.go
+++ b/lib/controller/rpc/conn.go
@@ -406,3 +406,10 @@ func (conn *Conn) UserSessionCreate(ctx context.Context, options UserSessionCrea
        err := conn.requestAndDecode(ctx, &resp, ep, nil, options)
        return resp, err
 }
+
+func (conn *Conn) UserBatchUpdate(ctx context.Context, options arvados.UserBatchUpdateOptions) (arvados.UserList, error) {
+       ep := arvados.APIEndpoint{Method: "PATCH", Path: "arvados/v1/users/batch_update"}
+       var resp arvados.UserList
+       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 d86df9ef33272a89dae345dd8a14c12c78676397..7d6ddb317621fde9800c385cc024218d16eb4630 100644 (file)
--- a/sdk/go/arvados/api.go
+++ b/sdk/go/arvados/api.go
@@ -54,6 +54,7 @@ var (
        EndpointUserUnsetup                   = APIEndpoint{"POST", "arvados/v1/users/{uuid}/unsetup", ""}
        EndpointUserUpdate                    = APIEndpoint{"PATCH", "arvados/v1/users/{uuid}", "user"}
        EndpointUserUpdateUUID                = APIEndpoint{"POST", "arvados/v1/users/{uuid}/update_uuid", ""}
+       EndpointUserBatchUpdate               = APIEndpoint{"PATCH", "arvados/v1/users/batch", ""}
        EndpointAPIClientAuthorizationCurrent = APIEndpoint{"GET", "arvados/v1/api_client_authorizations/current", ""}
 )
 
@@ -119,6 +120,12 @@ type UserMergeOptions struct {
        NewUserToken string `json:"new_user_token,omitempty"`
 }
 
+type UserBatchUpdateOptions struct {
+       Updates map[string]map[string]interface{} `json:"updates"`
+}
+
+type UserBatchUpdateResponse struct{}
+
 type DeleteOptions struct {
        UUID string `json:"uuid"`
 }
@@ -166,5 +173,6 @@ type API interface {
        UserGetSystem(ctx context.Context, options GetOptions) (User, error)
        UserList(ctx context.Context, options ListOptions) (UserList, error)
        UserDelete(ctx context.Context, options DeleteOptions) (User, error)
+       UserBatchUpdate(context.Context, UserBatchUpdateOptions) (UserList, error)
        APIClientAuthorizationCurrent(ctx context.Context, options GetOptions) (APIClientAuthorization, error)
 }

diff --git a/sdk/go/arvadostest/api.go b/sdk/go/arvadostest/api.go
index f18af8caaa65806699559b003c8beb886f7eaff9..91e3ee8ba66dc3df73c462ac3adaee2300c2ba23 100644 (file)
--- a/sdk/go/arvadostest/api.go
+++ b/sdk/go/arvadostest/api.go
@@ -169,6 +169,10 @@ func (as *APIStub) UserMerge(ctx context.Context, options arvados.UserMergeOptio
        as.appendCall(as.UserMerge, ctx, options)
        return arvados.User{}, as.Error
 }
+func (as *APIStub) UserBatchUpdate(ctx context.Context, options arvados.UserBatchUpdateOptions) (arvados.UserList, error) {
+       as.appendCall(as.UserBatchUpdate, ctx, options)
+       return arvados.UserList{}, 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

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 2889eacee644ba080439faa6a0e17ad629c8171c..ddf74cec67306605c47469af52ca3644f0e812ad 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -4,12 +4,31 @@
 
 class Arvados::V1::UsersController < ApplicationController
   accept_attribute_as_json :prefs, Hash
+  accept_param_as_json :updates
 
   skip_before_action :find_object_by_uuid, only:
-    [:activate, :current, :system, :setup, :merge]
+    [:activate, :current, :system, :setup, :merge, :batch_update]
   skip_before_action :render_404_if_no_object, only:
-    [:activate, :current, :system, :setup, :merge]
-  before_action :admin_required, only: [:setup, :unsetup, :update_uuid]
+    [:activate, :current, :system, :setup, :merge, :batch_update]
+  before_action :admin_required, only: [:setup, :unsetup, :update_uuid, :batch_update]
+
+  # Internal API used by controller to update local cache of user
+  # records from LoginCluster.
+  def batch_update
+    @objects = []
+    params[:updates].andand.each do |uuid, attrs|
+      begin
+        u = User.find_or_create_by(uuid: uuid)
+      rescue ActiveRecord::RecordNotUnique
+        retry
+      end
+      u.update_attributes!(attrs)
+      @objects << u
+    end
+    @offset = 0
+    @limit = -1
+    render_list
+  end
 
   def current
     if current_user

diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb
index b54c3c5bf170cc431140a0925b9846e7f172b397..8afd22192a62f56c002b363bf63625e07009fcec 100644 (file)
--- a/services/api/config/routes.rb
+++ b/services/api/config/routes.rb
@@ -83,6 +83,7 @@ Server::Application.routes.draw do
         post 'unsetup', on: :member
         post 'update_uuid', on: :member
         post 'merge', on: :collection
+        patch 'batch_update', on: :collection
       end
       resources :virtual_machines do
         get 'logins', on: :member

diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index d5db1039645cbadffc45d93317cc87664b889b38..74f017a678f76643884ff09261f17188483328eb 100644 (file)
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -1043,6 +1043,47 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
     assert_nil(users(:project_viewer).redirect_to_user_uuid)
   end
 
+  test "batch update fails for non-admin" do
+    authorize_with(:active)
+    patch(:batch_update, params: {updates: {}})
+    assert_response(403)
+  end
+
+  test "batch update" do
+    existinguuid = 'remot-tpzed-foobarbazwazqux'
+    newuuid = 'remot-tpzed-newnarnazwazqux'
+    act_as_system_user do
+      User.create!(uuid: existinguuid, email: 'root@existing.example.com')
+    end
+
+    authorize_with(:admin)
+    patch(:batch_update,
+          params: {
+            updates: {
+              existinguuid => {
+                'first_name' => 'root',
+                'email' => 'root@remot.example.com',
+                'is_active' => true,
+                'is_admin' => true,
+                'prefs' => {'foo' => 'bar'},
+              },
+              newuuid => {
+                'first_name' => 'noot',
+                'email' => 'root@remot.example.com',
+              },
+            }})
+    assert_response(:success)
+
+    assert_equal('root', User.find_by_uuid(existinguuid).first_name)
+    assert_equal('root@remot.example.com', User.find_by_uuid(existinguuid).email)
+    assert_equal(true, User.find_by_uuid(existinguuid).is_active)
+    assert_equal(true, User.find_by_uuid(existinguuid).is_admin)
+    assert_equal({'foo' => 'bar'}, User.find_by_uuid(existinguuid).prefs)
+
+    assert_equal('noot', User.find_by_uuid(newuuid).first_name)
+    assert_equal('root@remot.example.com', User.find_by_uuid(newuuid).email)
+  end
+
   NON_ADMIN_USER_DATA = ["uuid", "kind", "is_active", "email", "first_name",
                          "last_name", "username"].sort