Activate new users created on the login cluster by federation-migrate

Users are activated if they were active on their original cluster.

Update check script part of federation-migrate test to check
is_active.

Also add assertions to permission test "users with bidirectional read
permission in group can see each other" to explictly check that the
other user appears in user's user listing.

no issue #

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

diff --git a/sdk/python/arvados/commands/federation_migrate.py b/sdk/python/arvados/commands/federation_migrate.py
index 885d6fda03560fccb735216ff10b6c730dd1c1c2..e74d6215c79cb76af6fdab7170f3cd989a77143e 100755 (executable)
--- a/sdk/python/arvados/commands/federation_migrate.py
+++ b/sdk/python/arvados/commands/federation_migrate.py
@@ -197,14 +197,17 @@ def choose_new_user(args, by_email, email, userhome, username, old_user_uuid, cl
             return None
         print("(%s) No user listed with same email to migrate %s to %s, will create new user with username '%s'" % (email, old_user_uuid, userhome, username))
         if not args.dry_run:
+            oldhomecluster = old_user_uuid[0:5]
+            oldhomearv = clusters[oldhomecluster]
             newhomecluster = userhome[0:5]
             homearv = clusters[userhome]
             user = None
             try:
+                olduser = oldhomearv.users().get(uuid=old_user_uuid).execute()
                 conflicts = homearv.users().list(filters=[["username", "=", username]]).execute()
                 if conflicts["items"]:
                     homearv.users().update(uuid=conflicts["items"][0]["uuid"], body={"user": {"username": username+"migrate"}}).execute()
-                user = homearv.users().create(body={"user": {"email": email, "username": username}}).execute()
+                user = homearv.users().create(body={"user": {"email": email, "username": username, "is_active": olduser["is_active"]}}).execute()
             except arvados.errors.ApiError as e:
                 print("(%s) Could not create user: %s" % (email, str(e)))
                 return None

diff --git a/sdk/python/tests/fed-migrate/check.py b/sdk/python/tests/fed-migrate/check.py
index 8f494be2fb4448ed3135c05328b12f3d398f55a1..85d2d31f2309fe3182fc1d606982d1bea34e41c1 100644 (file)
--- a/sdk/python/tests/fed-migrate/check.py
+++ b/sdk/python/tests/fed-migrate/check.py
@@ -8,6 +8,10 @@ apiA = arvados.api(host=j["arvados_api_hosts"][0], token=j["superuser_tokens"][0
 apiB = arvados.api(host=j["arvados_api_hosts"][1], token=j["superuser_tokens"][1], insecure=True)
 apiC = arvados.api(host=j["arvados_api_hosts"][2], token=j["superuser_tokens"][2], insecure=True)
 
+###
+### Check users on API server "A" (the LoginCluster) ###
+###
+
 users = apiA.users().list().execute()
 
 assert len(users["items"]) == 11
@@ -22,6 +26,15 @@ for i in range(1, 10):
             by_username[u["username"]] = u["uuid"]
     assert found
 
+# Should be active
+for i in (1, 2, 3, 4, 5, 6, 7, 8):
+    found = False
+    for u in users["items"]:
+        if u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and u["is_active"] is True:
+            found = True
+    assert found, "Not found case%i" % i
+
+# case9 should not be active
 found = False
 for u in users["items"]:
     if (u["username"] == "case9" and u["email"] == "case9@test" and
@@ -29,23 +42,40 @@ for u in users["items"]:
         found = True
 assert found
 
+
+###
+### Check users on API server "B" (federation member) ###
+###
 users = apiB.users().list().execute()
 assert len(users["items"]) == 11
 
-for i in range(2, 10):
+for i in range(2, 9):
     found = False
     for u in users["items"]:
-        if u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and u["uuid"] == by_username[u["username"]]:
+        if (u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and
+            u["uuid"] == by_username[u["username"]] and u["is_active"] is True):
             found = True
-    assert found
+    assert found, "Not found case%i" % i
+
+found = False
+for u in users["items"]:
+    if (u["username"] == "case9" and u["email"] == "case9@test" and
+        u["uuid"] == by_username[u["username"]] and u["is_active"] is False):
+        found = True
+assert found
+
 
+###
+### Check users on API server "C" (federation member) ###
+###
 users = apiC.users().list().execute()
 assert len(users["items"]) == 8
 
 for i in (2, 4, 6, 7, 8):
     found = False
     for u in users["items"]:
-        if u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and u["uuid"] == by_username[u["username"]]:
+        if (u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and
+            u["uuid"] == by_username[u["username"]] and u["is_active"] is True):
             found = True
     assert found
 
@@ -54,7 +84,8 @@ for i in (2, 4, 6, 7, 8):
 for i in (3, 5, 9):
     found = False
     for u in users["items"]:
-        if u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and u["uuid"] == by_username[u["username"]]:
+        if (u["username"] == ("case%d" % i) and u["email"] == ("case%d@test" % i) and
+            u["uuid"] == by_username[u["username"]] and u["is_active"] is True):
             found = True
     assert not found
 

diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 275d2a651b8d2edc0dabbb16c55e55cccef15bc2..18d2fbbcb5f7cef9d87cc71df1077b5a4c8cf1d6 100644 (file)
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -287,6 +287,12 @@ class PermissionTest < ActiveSupport::TestCase
     a = create :active_user, first_name: "A"
     b = create :active_user, first_name: "B"
     other = create :active_user, first_name: "OTHER"
+
+    assert_empty(User.readable_by(b).where(uuid: a.uuid),
+                     "#{b.first_name} should not be able to see 'a' in the user list")
+    assert_empty(User.readable_by(a).where(uuid: b.uuid),
+                     "#{a.first_name} should not be able to see 'b' in the user list")
+
     act_as_system_user do
       g = create :group
       [a,b].each do |u|
@@ -296,6 +302,12 @@ class PermissionTest < ActiveSupport::TestCase
                name: 'can_read', head_uuid: u.uuid, tail_uuid: g.uuid)
       end
     end
+
+    assert_not_empty(User.readable_by(b).where(uuid: a.uuid),
+                     "#{b.first_name} should be able to see 'a' in the user list")
+    assert_not_empty(User.readable_by(a).where(uuid: b.uuid),
+                     "#{a.first_name} should be able to see 'b' in the user list")
+
     a_specimen = act_as_user a do
       Specimen.create!
     end