Merge branch '19146-can-write-manage'

refs #19146

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index 2a762d92480955c0a55ed3e2e4fe11d7139b6664..db0aac3c7a3570fd0b3b5e9aa2c74dbe9874c196 100644 (file)
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -30,7 +30,9 @@ table(table table-bordered table-condensed).
 @"role"@|
 |description|text|||
 |properties|hash|User-defined metadata, may be used in queries using "subproperty filters":{{site.baseurl}}/api/methods.html#subpropertyfilters ||
-|writable_by|array|List of UUID strings identifying Users and other Groups that have write permission for this Group.  Only users who are allowed to administer the Group will receive a full list.  Other users will receive a partial list that includes the Group's owner_uuid and (if applicable) their own user UUID.||
+|writable_by|array|(Deprecated) List of UUID strings identifying Users and other Groups that have write permission for this Group.  Users who are allowed to administer the Group will receive a list of user/group UUIDs that have permission via explicit permission links; permissions via parent/ancestor groups are not taken into account.  Other users will receive a partial list including only the Group's owner_uuid and (if applicable) their own user UUID.||
+|can_write|boolean|True if the current user has write permission on this group.||
+|can_manage|boolean|True if the current user has manage permission on this group.||
 |trash_at|datetime|If @trash_at@ is non-null and in the past, this group and all objects directly or indirectly owned by the group will be hidden from API calls.  May be untrashed.||
 |delete_at|datetime|If @delete_at@ is non-null and in the past, the group and all objects directly or indirectly owned by the group may be permanently deleted.||
 |is_trashed|datetime|True if @trash_at@ is in the past, false if not.||

diff --git a/lib/controller/federation/conn.go b/lib/controller/federation/conn.go
index 1b8ec9e64a6e01138a1bfc58a599a78eb2f0e44b..d9f587852d149da46cd49ddbfc9dd46095fe180e 100644 (file)
--- a/lib/controller/federation/conn.go
+++ b/lib/controller/federation/conn.go
@@ -553,6 +553,8 @@ var userAttrsCachedFromLoginCluster = map[string]bool{
        "owner_uuid":              false,
        "uuid":                    false,
        "writable_by":             false,
+       "can_write":               false,
+       "can_manage":              false,
 }
 
 func (conn *Conn) batchUpdateUsers(ctx context.Context,

diff --git a/lib/controller/localdb/group_test.go b/lib/controller/localdb/group_test.go
index 2d55def9f6cbba8c68d2520b6d629845204bb26f..78150c95527dc0f66ed6187bd64e93c7aef428c8 100644 (file)
--- a/lib/controller/localdb/group_test.go
+++ b/lib/controller/localdb/group_test.go
@@ -24,14 +24,7 @@ type GroupSuite struct {
        railsSpy *arvadostest.Proxy
 }
 
-func (s *GroupSuite) TearDownSuite(c *check.C) {
-       // Undo any changes/additions to the user database so they
-       // don't affect subsequent tests.
-       arvadostest.ResetEnv()
-       c.Check(arvados.NewClientFromEnv().RequestAndDecode(nil, "POST", "database/reset", nil, nil), check.IsNil)
-}
-
-func (s *GroupSuite) SetUpTest(c *check.C) {
+func (s *GroupSuite) SetUpSuite(c *check.C) {
        cfg, err := config.NewLoader(nil, ctxlog.TestLogger(c)).Load()
        c.Assert(err, check.IsNil)
        s.cluster, err = cfg.GetCluster("")
@@ -41,8 +34,12 @@ func (s *GroupSuite) SetUpTest(c *check.C) {
        *s.localdb.railsProxy = *rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider)
 }
 
-func (s *GroupSuite) TearDownTest(c *check.C) {
+func (s *GroupSuite) TearDownSuite(c *check.C) {
        s.railsSpy.Close()
+       // Undo any changes/additions to the user database so they
+       // don't affect subsequent tests.
+       arvadostest.ResetEnv()
+       c.Check(arvados.NewClientFromEnv().RequestAndDecode(nil, "POST", "database/reset", nil, nil), check.IsNil)
 }
 
 func (s *GroupSuite) setUpVocabulary(c *check.C, testVocabulary string) {
@@ -136,3 +133,136 @@ func (s *GroupSuite) TestGroupUpdateWithProperties(c *check.C) {
                }
        }
 }
+
+func (s *GroupSuite) TestCanWriteCanManageResponses(c *check.C) {
+       ctxUser1 := auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{arvadostest.ActiveTokenV2}})
+       ctxUser2 := auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{arvadostest.SpectatorToken}})
+       ctxAdmin := auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{arvadostest.AdminToken}})
+       project, err := s.localdb.GroupCreate(ctxUser1, arvados.CreateOptions{
+               Attrs: map[string]interface{}{
+                       "group_class": "project",
+               },
+       })
+       c.Assert(err, check.IsNil)
+       c.Check(project.CanWrite, check.Equals, true)
+       c.Check(project.CanManage, check.Equals, true)
+
+       subproject, err := s.localdb.GroupCreate(ctxUser1, arvados.CreateOptions{
+               Attrs: map[string]interface{}{
+                       "owner_uuid":  project.UUID,
+                       "group_class": "project",
+               },
+       })
+       c.Assert(err, check.IsNil)
+       c.Check(subproject.CanWrite, check.Equals, true)
+       c.Check(subproject.CanManage, check.Equals, true)
+
+       projlist, err := s.localdb.GroupList(ctxUser1, arvados.ListOptions{
+               Limit:   -1,
+               Filters: []arvados.Filter{{"uuid", "in", []string{project.UUID, subproject.UUID}}},
+       })
+       c.Assert(err, check.IsNil)
+       c.Assert(projlist.Items, check.HasLen, 2)
+       for _, p := range projlist.Items {
+               c.Check(p.CanWrite, check.Equals, true)
+               c.Check(p.CanManage, check.Equals, true)
+       }
+
+       // Give 2nd user permission to read
+       permlink, err := s.localdb.LinkCreate(ctxAdmin, arvados.CreateOptions{
+               Attrs: map[string]interface{}{
+                       "link_class": "permission",
+                       "name":       "can_read",
+                       "tail_uuid":  arvadostest.SpectatorUserUUID,
+                       "head_uuid":  project.UUID,
+               },
+       })
+       c.Assert(err, check.IsNil)
+
+       // As 2nd user: can read, cannot manage, cannot write
+       project2, err := s.localdb.GroupGet(ctxUser2, arvados.GetOptions{UUID: project.UUID})
+       c.Assert(err, check.IsNil)
+       c.Check(project2.CanWrite, check.Equals, false)
+       c.Check(project2.CanManage, check.Equals, false)
+
+       _, err = s.localdb.LinkUpdate(ctxAdmin, arvados.UpdateOptions{
+               UUID: permlink.UUID,
+               Attrs: map[string]interface{}{
+                       "name": "can_write",
+               },
+       })
+       c.Assert(err, check.IsNil)
+
+       // As 2nd user: cannot manage, can write
+       project2, err = s.localdb.GroupGet(ctxUser2, arvados.GetOptions{UUID: project.UUID})
+       c.Assert(err, check.IsNil)
+       c.Check(project2.CanWrite, check.Equals, true)
+       c.Check(project2.CanManage, check.Equals, false)
+
+       // As owner: after freezing, can manage (owner), cannot write (frozen)
+       project, err = s.localdb.GroupUpdate(ctxUser1, arvados.UpdateOptions{
+               UUID: project.UUID,
+               Attrs: map[string]interface{}{
+                       "frozen_by_uuid": arvadostest.ActiveUserUUID,
+               }})
+       c.Assert(err, check.IsNil)
+       c.Check(project.CanWrite, check.Equals, false)
+       c.Check(project.CanManage, check.Equals, true)
+
+       // As admin: can manage (admin), cannot write (frozen)
+       project, err = s.localdb.GroupGet(ctxAdmin, arvados.GetOptions{UUID: project.UUID})
+       c.Assert(err, check.IsNil)
+       c.Check(project.CanWrite, check.Equals, false)
+       c.Check(project.CanManage, check.Equals, true)
+
+       // As 2nd user: cannot manage (perm), cannot write (frozen)
+       project2, err = s.localdb.GroupGet(ctxUser2, arvados.GetOptions{UUID: project.UUID})
+       c.Assert(err, check.IsNil)
+       c.Check(project2.CanWrite, check.Equals, false)
+       c.Check(project2.CanManage, check.Equals, false)
+
+       // After upgrading perm to "manage", as 2nd user: can manage (perm), cannot write (frozen)
+       _, err = s.localdb.LinkUpdate(ctxAdmin, arvados.UpdateOptions{
+               UUID: permlink.UUID,
+               Attrs: map[string]interface{}{
+                       "name": "can_manage",
+               },
+       })
+       c.Assert(err, check.IsNil)
+       project2, err = s.localdb.GroupGet(ctxUser2, arvados.GetOptions{UUID: project.UUID})
+       c.Assert(err, check.IsNil)
+       c.Check(project2.CanWrite, check.Equals, false)
+       c.Check(project2.CanManage, check.Equals, true)
+
+       // 2nd user can also manage (but not write) the subject inside the frozen project
+       subproject2, err := s.localdb.GroupGet(ctxUser2, arvados.GetOptions{UUID: subproject.UUID})
+       c.Assert(err, check.IsNil)
+       c.Check(subproject2.CanWrite, check.Equals, false)
+       c.Check(subproject2.CanManage, check.Equals, true)
+
+       u, err := s.localdb.UserGet(ctxUser1, arvados.GetOptions{
+               UUID: arvadostest.ActiveUserUUID,
+       })
+       c.Assert(err, check.IsNil)
+       c.Check(u.CanWrite, check.Equals, true)
+       c.Check(u.CanManage, check.Equals, true)
+
+       for _, selectParam := range [][]string{
+               nil,
+               {"can_write", "can_manage"},
+       } {
+               c.Logf("selectParam: %+v", selectParam)
+               ulist, err := s.localdb.UserList(ctxUser1, arvados.ListOptions{
+                       Limit:   -1,
+                       Filters: []arvados.Filter{{"uuid", "=", arvadostest.ActiveUserUUID}},
+                       Select:  selectParam,
+               })
+               c.Assert(err, check.IsNil)
+               c.Assert(ulist.Items, check.HasLen, 1)
+               c.Logf("%+v", ulist.Items)
+               for _, u := range ulist.Items {
+                       c.Check(u.CanWrite, check.Equals, true)
+                       c.Check(u.CanManage, check.Equals, true)
+               }
+       }
+}

diff --git a/sdk/go/arvados/group.go b/sdk/go/arvados/group.go
index ad7ac1ee2b74a0d972b2adb4570a6ba417d455b6..0782bd43d154f1ada231307f6a85970c61a28b08 100644 (file)
--- a/sdk/go/arvados/group.go
+++ b/sdk/go/arvados/group.go
@@ -27,6 +27,8 @@ type Group struct {
        WritableBy           []string               `json:"writable_by,omitempty"`
        Description          string                 `json:"description"`
        FrozenByUUID         string                 `json:"frozen_by_uuid"`
+       CanWrite             bool                   `json:"can_write"`
+       CanManage            bool                   `json:"can_manage"`
 }
 
 // GroupList is an arvados#groupList resource.

diff --git a/sdk/go/arvados/user.go b/sdk/go/arvados/user.go
index 68960144a8a3dae092c604bfa4a256efcc8a669b..2fb061e7fb818840fc8e1c42ae10e771c45e1ee5 100644 (file)
--- a/sdk/go/arvados/user.go
+++ b/sdk/go/arvados/user.go
@@ -26,6 +26,8 @@ type User struct {
        ModifiedByClientUUID string                 `json:"modified_by_client_uuid"`
        Prefs                map[string]interface{} `json:"prefs"`
        WritableBy           []string               `json:"writable_by,omitempty"`
+       CanWrite             bool                   `json:"can_write"`
+       CanManage            bool                   `json:"can_manage"`
 }
 
 // UserList is an arvados#userList resource.

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 54db521768592742dd9c4406ff987f0d7c7ee496..507cb4ac339fe5fd63fbbf7fb3013411fc44b5e9 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -274,7 +274,7 @@ class Arvados::V1::UsersController < ApplicationController
     return super if @read_users.any?(&:is_admin)
     if params[:uuid] != current_user.andand.uuid
       # Non-admin index/show returns very basic information about readable users.
-      safe_attrs = ["uuid", "is_active", "email", "first_name", "last_name", "username"]
+      safe_attrs = ["uuid", "is_active", "email", "first_name", "last_name", "username", "can_write", "can_manage"]
       if @select
         @select = @select & safe_attrs
       else

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 07a31d81a8a129dc67acb4aa1a7fa8f39e253ea1..c2725506c02ef75a85dee2a7c3a11fbd8db7e119 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -273,6 +273,26 @@ class ArvadosModel < ApplicationRecord
     end.compact.uniq
   end
 
+  def can_write
+    if respond_to?(:frozen_by_uuid) && frozen_by_uuid
+      # This special case is needed to return the correct value from a
+      # "freeze project" API, during which writable status changes
+      # from true to false.
+      #
+      # current_user.can?(write: self) returns true (which is correct
+      # in the context of permission-checking hooks) but the can_write
+      # value we're returning to the caller here represents the state
+      # _after_ the update, i.e., false.
+      return false
+    else
+      return current_user.can?(write: self)
+    end
+  end
+
+  def can_manage
+    return current_user.can?(manage: self)
+  end
+
   # Return a query with read permissions restricted to the union of the
   # permissions of the members of users_list, i.e. if something is readable by
   # any user in users_list, it will be readable in the query returned by this

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index b1b2e942c60c4bc79c13dbe731ad9bc1112684d2..0c36a048dcfd9f31679abf642c085e14e97dc1e4 100644 (file)
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -44,6 +44,17 @@ class Group < ArvadosModel
     t.add :is_trashed
     t.add :properties
     t.add :frozen_by_uuid
+    t.add :can_write
+    t.add :can_manage
+  end
+
+  protected
+
+  def self.attributes_required_columns
+    super.merge(
+                'can_write' => ['owner_uuid', 'uuid'],
+                'can_manage' => ['owner_uuid', 'uuid'],
+                )
   end
 
   def ensure_filesystem_compatible_name

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 1d1d83662c17f68b6611486624714555c6b854e2..52d36ac57735f0c16d4b0ed6271a50681e08e05b 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -72,6 +72,8 @@ class User < ArvadosModel
     t.add :is_invited
     t.add :prefs
     t.add :writable_by
+    t.add :can_write
+    t.add :can_manage
   end
 
   ALL_PERMISSIONS = {read: true, write: true, manage: true}
@@ -121,6 +123,14 @@ class User < ArvadosModel
       end
       next if target_uuid == self.uuid
 
+      if action == :write && target && !target.new_record? &&
+         target.respond_to?(:frozen_by_uuid) &&
+         target.frozen_by_uuid_was
+        # Just an optimization to skip the PERMISSION_VIEW and
+        # FrozenGroup queries below
+        return false
+      end
+
       target_owner_uuid = target.owner_uuid if target.respond_to? :owner_uuid
 
       user_uuids_subquery = USER_UUIDS_SUBQUERY_TEMPLATE % {user: "$1", perm_level: "$3"}
@@ -581,6 +591,13 @@ SELECT target_uuid, perm_level
 
   protected
 
+  def self.attributes_required_columns
+    super.merge(
+                'can_write' => ['owner_uuid', 'uuid'],
+                'can_manage' => ['owner_uuid', 'uuid'],
+                )
+  end
+
   def change_all_uuid_refs(old_uuid:, new_uuid:)
     ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |klass|
       klass.columns.each do |col|

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 ae7b21dec83e556f8321ee7290e5680824be5881..6a7b00a00573f9413a61856d66fd6e6eb3900c65 100644 (file)
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -674,6 +674,12 @@ The Arvados team.
     get(:index)
     check_non_admin_index
     check_readable_users_index [:spectator], [:inactive, :active]
+    json_response["items"].each do |u|
+      if u["uuid"] == users(:spectator).uuid
+        assert_equal true, u["can_write"]
+        assert_equal true, u["can_manage"]
+      end
+    end
   end
 
   test "non-admin user gets only safe attributes from users#show" do
@@ -1079,7 +1085,7 @@ The Arvados team.
   end
 
   NON_ADMIN_USER_DATA = ["uuid", "kind", "is_active", "email", "first_name",
-                         "last_name", "username"].sort
+                         "last_name", "username", "can_write", "can_manage"].sort
 
   def check_non_admin_index
     assert_response :success