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 <peter.amstutz@curii.com>

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 43fe1cc0140b34d24444dbb883eeb29da6d4055d..f07c3b63167d577f722b085ff56de252a73f251f 100644 (file)
--- 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 09aa5086decd3eeb62c20874b64ad1308674668c..2812c1f41d5c6cd7aa3f02da5b6a06f051c6283a 100644 (file)
--- 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 76b722694aa763baeb79a2ce621c620383c139a2..518fe569377ab1b2c2b21880e4f3411aa340844a 100644 (file)
--- 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 778ad7d0bb1728c22ad45dcfecdc5264f1c65312..8ec90f7e53a38805eff5b9ebac846eb88a4d7117 100644 (file)
--- 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 71f90d493846d637f2d296ef9583befcf1e3ba22..9ad93c2ee6af4f3cb6a6a42f10e85468b90e8946 100644 (file)
--- 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',