From: Tom Clegg Date: Tue, 19 Nov 2019 14:49:15 +0000 (-0500) Subject: 15720: Batch user update API. X-Git-Tag: 2.0.0~107^2~3 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/233a2b6bd23a3e2054cfc0690f2bc06c0f9f7323?hp=-c 15720: Batch user update API. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- 233a2b6bd23a3e2054cfc0690f2bc06c0f9f7323 diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go index 47ae0ba9ec..1d8fa7e462 100644 --- 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 993be9b0b8..8202a668ff 100644 --- 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 250f3cb45b..47082197a0 100644 --- 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 25efcfd439..66523e5ac3 100644 --- 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 d86df9ef33..7d6ddb3176 100644 --- 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 f18af8caaa..91e3ee8ba6 100644 --- 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 2889eacee6..ddf74cec67 100644 --- 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 b54c3c5bf1..8afd22192a 100644 --- 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 d5db103964..74f017a678 100644 --- 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