[ARVADOS] created: 1.3.0-3241-g1a343a507

Git user git at public.arvados.org
Wed Sep 30 21:16:15 UTC 2020


        at  1a343a507d7e4bdf56bc5e6108996f0894b11a9c (commit)


commit 1a343a507d7e4bdf56bc5e6108996f0894b11a9c
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Wed Sep 30 17:13:59 2020 -0400

    16914: UserGet() passes "select" option along to batch update
    
    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 at curii.com>

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 43fe1cc01..f07c3b631 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 09aa5086d..2812c1f41 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 76b722694..518fe5693 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 778ad7d0b..8ec90f7e5 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 71f90d493..9ad93c2ee 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 at 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 at example.com', json_response['email']
+    assert_equal 'barney', json_response['username']
+
+    @stub_content = {
+      uuid: 'zbbbb-tpzed-000000000000001',
+      email: 'foo at 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 at 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 at example.com', json_response['email']
+    assert_equal 'barney', json_response['username']
+
+  end
+
   test 'pre-activate remote user' do
     @stub_content = {
       uuid: 'zbbbb-tpzed-000000000001234',

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list