From: Peter Amstutz Date: Wed, 30 Sep 2020 21:13:59 +0000 (-0400) Subject: UserGet() passes "select" option along to batch update refs #16914 X-Git-Tag: 2.1.0~27 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/2a1a143938bfbfa9713c8f368898a8dda1ac685f UserGet() passes "select" option along to batch update refs #16914 Fixes issue of user being deactivated because something called "get user" and selected fields didn't include is_active. Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go index 43fe1cc014..f07c3b6316 100644 --- a/lib/controller/federation/conn.go +++ b/lib/controller/federation/conn.go @@ -508,7 +508,7 @@ 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{}, []arvados.User{resp}) + err = conn.batchUpdateUsers(ctx, arvados.ListOptions{Select: options.Select}, []arvados.User{resp}) if err != nil { return arvados.User{}, err } diff --git a/lib/controller/federation/user_test.go b/lib/controller/federation/user_test.go index 09aa5086de..2812c1f41d 100644 --- a/lib/controller/federation/user_test.go +++ b/lib/controller/federation/user_test.go @@ -117,6 +117,60 @@ func (s *UserSuite) TestLoginClusterUserList(c *check.C) { } } +func (s *UserSuite) TestLoginClusterUserGet(c *check.C) { + s.cluster.ClusterID = "local" + s.cluster.Login.LoginCluster = "zzzzz" + s.fed = New(s.cluster) + s.addDirectRemote(c, "zzzzz", rpc.NewConn("zzzzz", &url.URL{Scheme: "https", Host: os.Getenv("ARVADOS_API_HOST")}, true, rpc.PassthroughTokenProvider)) + + opts := arvados.GetOptions{UUID: "zzzzz-tpzed-xurymjxw79nv3jz", Select: []string{"uuid", "email"}} + + stub := &arvadostest.APIStub{Error: errors.New("local cluster failure")} + s.fed.local = stub + s.fed.UserGet(s.ctx, opts) + + 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{ + "uuid": false, + "email": true, + "first_name": true, + "last_name": true, + "is_admin": true, + "is_active": true, + "prefs": true, + // can't safely update locally + "owner_uuid": false, + "identity_url": false, + // virtual attrs + "full_name": false, + "is_invited": false, + } + if opts.Select != nil { + // Only the selected + // fields (minus uuid) + // should be updated. + for k := range shouldUpdate { + shouldUpdate[k] = false + } + for _, k := range opts.Select { + if k != "uuid" { + shouldUpdate[k] = true + } + } + } + var uuid string + for uuid = range calls[0].Options.(arvados.UserBatchUpdateOptions).Updates { + } + for k, shouldFind := range shouldUpdate { + _, found := calls[0].Options.(arvados.UserBatchUpdateOptions).Updates[uuid][k] + c.Check(found, check.Equals, shouldFind, check.Commentf("offending attr: %s", k)) + } + } + +} + func (s *UserSuite) TestLoginClusterUserListBypassFederation(c *check.C) { s.cluster.ClusterID = "local" s.cluster.Login.LoginCluster = "zzzzz" diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb index 76b722694a..518fe56937 100644 --- a/services/api/app/models/api_client_authorization.rb +++ b/services/api/app/models/api_client_authorization.rb @@ -268,14 +268,12 @@ class ApiClientAuthorization < ArvadosModel end act_as_system_user do - user.save! - if (user.is_active && !remote_user['is_active']) or (user.is_invited && !remote_user['is_invited']) - # If the user is newly created and AutoSetupNewUsers is - # true, they will auto-setup in an after_create hook. - # Synchronize the user's "active/invited" state state after the record - # has been saved. + # Synchronize the user's "active/invited" state state. This + # also saves the record. user.unsetup + else + user.save! end # We will accept this token (and avoid reloading the user diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index 778ad7d0bb..8ec90f7e53 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -38,7 +38,8 @@ class User < ArvadosModel after_create :auto_setup_new_user, :if => Proc.new { Rails.configuration.Users.AutoSetupNewUsers and (uuid != system_user_uuid) and - (uuid != anonymous_user_uuid) + (uuid != anonymous_user_uuid) and + (uuid[0..4] == Rails.configuration.ClusterID) } after_create :send_admin_notifications diff --git a/services/api/test/integration/remote_user_test.rb b/services/api/test/integration/remote_user_test.rb index 71f90d4938..9ad93c2ee6 100644 --- a/services/api/test/integration/remote_user_test.rb +++ b/services/api/test/integration/remote_user_test.rb @@ -348,6 +348,68 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest assert_equal 'barney', json_response['username'] end + test 'get active user from Login cluster when AutoSetupNewUsers is set' do + Rails.configuration.Login.LoginCluster = 'zbbbb' + Rails.configuration.Users.AutoSetupNewUsers = true + @stub_content = { + uuid: 'zbbbb-tpzed-000000000000001', + email: 'foo@example.com', + username: 'barney', + is_admin: false, + is_active: true, + is_invited: true, + } + get '/arvados/v1/users/current', + params: {format: 'json'}, + headers: auth(remote: 'zbbbb') + assert_response :success + assert_equal 'zbbbb-tpzed-000000000000001', json_response['uuid'] + assert_equal false, json_response['is_admin'] + assert_equal true, json_response['is_active'] + assert_equal true, json_response['is_invited'] + assert_equal 'foo@example.com', json_response['email'] + assert_equal 'barney', json_response['username'] + + @stub_content = { + uuid: 'zbbbb-tpzed-000000000000001', + email: 'foo@example.com', + username: 'barney', + is_admin: false, + is_active: false, + is_invited: false, + } + + # Use cached value. User will still be active because we haven't + # re-queried the upstream cluster. + get '/arvados/v1/users/current', + params: {format: 'json'}, + headers: auth(remote: 'zbbbb') + assert_response :success + assert_equal 'zbbbb-tpzed-000000000000001', json_response['uuid'] + assert_equal false, json_response['is_admin'] + assert_equal true, json_response['is_active'] + assert_equal true, json_response['is_invited'] + assert_equal 'foo@example.com', json_response['email'] + assert_equal 'barney', json_response['username'] + + # Delete cached value. User should be inactive now. + act_as_system_user do + ApiClientAuthorization.delete_all + end + + get '/arvados/v1/users/current', + params: {format: 'json'}, + headers: auth(remote: 'zbbbb') + assert_response :success + assert_equal 'zbbbb-tpzed-000000000000001', json_response['uuid'] + assert_equal false, json_response['is_admin'] + assert_equal false, json_response['is_active'] + assert_equal false, json_response['is_invited'] + assert_equal 'foo@example.com', json_response['email'] + assert_equal 'barney', json_response['username'] + + end + test 'pre-activate remote user' do @stub_content = { uuid: 'zbbbb-tpzed-000000000001234',