Merge branch '16007-validate-group-class' refs #16007
authorPeter Amstutz <peter.amstutz@curii.com>
Fri, 19 Jun 2020 15:38:18 +0000 (11:38 -0400)
committerPeter Amstutz <peter.amstutz@curii.com>
Fri, 19 Jun 2020 15:38:18 +0000 (11:38 -0400)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

29 files changed:
apps/workbench/test/integration/projects_test.rb
doc/admin/upgrading.html.textile.liquid
doc/api/permission-model.html.textile.liquid
services/api/app/models/arvados_model.rb
services/api/app/models/group.rb
services/api/app/models/link.rb
services/api/db/migrate/20200602141328_fix_roles_projects.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/lib/current_api_client.rb
services/api/lib/fix_roles_projects.rb [new file with mode: 0644]
services/api/lib/update_permissions.rb
services/api/test/fixtures/collections.yml
services/api/test/fixtures/groups.yml
services/api/test/fixtures/links.yml
services/api/test/functional/application_controller_test.rb
services/api/test/functional/arvados/v1/filters_test.rb
services/api/test/functional/arvados/v1/groups_controller_test.rb
services/api/test/functional/arvados/v1/repositories_controller_test.rb
services/api/test/functional/arvados/v1/users_controller_test.rb
services/api/test/integration/groups_test.rb
services/api/test/integration/permissions_test.rb
services/api/test/performance/permission_test.rb
services/api/test/unit/arvados_model_test.rb
services/api/test/unit/group_test.rb
services/api/test/unit/owner_test.rb
services/api/test/unit/permission_test.rb
services/fuse/tests/test_mount.py
tools/sync-groups/sync-groups.go
tools/sync-groups/sync-groups_test.go

index 17ab5e4661db335f7f40129d2dac246f69990e67..7a5103007f80f2eba79275a67c602a8cb7d5e3c9 100644 (file)
@@ -132,7 +132,7 @@ class ProjectsTest < ActionDispatch::IntegrationTest
     show_object_using('active', 'groups', 'aproject', 'A Project')
     click_on "Sharing"
     click_on "Share with groups"
-    good_uuid = api_fixture("groups")["private"]["uuid"]
+    good_uuid = api_fixture("groups")["future_project_viewing_group"]["uuid"]
     assert(page.has_selector?(".selectable[data-object-uuid=\"#{good_uuid}\"]"),
            "'share with groups' listing missing owned user group")
     bad_uuid = api_fixture("groups")["asubproject"]["uuid"]
index edd92fa0ea1a117a91d60f3453dc6c3bd146ca3d..9cddce5fe647656a3ef0134aa1ab2642db1b5fcd 100644 (file)
@@ -34,7 +34,7 @@ TODO: extract this information based on git commit messages and generate changel
 <div class="releasenotes">
 </notextile>
 
-h2(#master). development master (as of 2020-02-07)
+h2(#master). development master (as of 2020-06-17)
 
 "Upgrading from 2.0.0":#v2_0_0
 
@@ -48,6 +48,41 @@ h3. S3 signatures
 
 Keepstore now uses "V4 signatures":https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-authenticating-requests.html by default for S3 requests. If you are using Amazon S3, no action is needed; all regions support V4 signatures. If you are using a different S3-compatible service that does not support V4 signatures, add @V2Signature: true@ to your volume driver parameters to preserve the old behavior. See "configuring S3 object storage":{{site.baseurl}}/install/configure-s3-object-storage.html.
 
+h3. New permission system constraints
+
+Some constraints on the permission system have been added, in particular @role@ and @project@ group types now have distinct behavior. These constraints were already de-facto imposed by the Workbench UI, so on most installations the only effect of this migration will be to reassign @role@ groups to the system user and create a @can_manage@ permission link for the previous owner.
+
+# The @group_class@ field must be either @role@ or @project@. Invalid group_class are migrated to @role@.
+# A @role@ cannot own things. Anything owned by a role is migrated to a @can_manage@ link and reassigned to the system user.
+# Only @role@ and @user@ can have outgoing permission links. Permission links originating from projects are deleted by the migration.
+# A @role@ is always owned by the system_user. When a group is created, it creates a @can_manage@ link for the object that would have been assigned to @owner_uuid@.  Migration adds @can_manage@ links and reassigns roles to the system user.  This also has the effect of requiring that all @role@ groups have unique names on the system.  If there is a name collision during migration, roles will be renamed to ensure they are unique.
+# A permission link can have the permission level (@name@) updated but not @head_uuid@, @tail_uuid@ or @link_class@.
+
+The @arvados-sync-groups@ tool has been updated to reflect these constraints, so it is important to use the version of @arvados-sync-groups@ that matches the API server version.
+
+Before upgrading, use the following commands to find out which groups and permissions in your database will be automatically modified or deleted during the upgrade.
+
+To determine which groups have invalid @group_class@ (these will be migrated to @role@ groups):
+
+<pre>
+arv group list --filters '[["group_class", "not in", ["project", "role"]]]'
+</pre>
+
+To list all @role@ groups, which will be reassigned to the system user (unless @owner_uuid@ is already the system user):
+
+<pre>
+arv group list --filters '[["group_class", "=", "role"]]'
+</pre>
+
+To list which @project@ groups have outgoing permission links (such links are now invalid and will be deleted by the migration):
+
+<pre>
+for uuid in $(arv link list --filters '[["link_class", "=", "permission"], ["tail_uuid", "like", "%-j7d0g-%"]]' |
+              jq -r .items[].tail_uuid | sort | uniq) ; do
+   arv group list --filters '[["group_class", "=", "project"], ["uuid", "=", "'$uuid'"]]' | jq .items
+done
+</pre>
+
 h2(#v2_0_0). v2.0.0 (2020-02-07)
 
 "Upgrading from 1.4":#v1_4_1
index 1f08ea419523a5178946bfcf43bd1ecd4e3a96a4..7f10521299742fc7e61e6d992d40c902b058a3ed 100644 (file)
@@ -10,59 +10,89 @@ Copyright (C) The Arvados Authors. All rights reserved.
 SPDX-License-Identifier: CC-BY-SA-3.0
 {% endcomment %}
 
-* There are four levels of permission: *none*, *can_read*, *can_write*, and *can_manage*.
-** *none* is the default state when there are no other permission grants.
-*** the object is not included in any list query response.
-*** direct queries of the object by uuid return 404 Not Found.
-*** Link objects require valid identifiers in @head_uuid@ and @tail_uuid@, so an attempt to create a Link that references an unreadable object will return an error indicating the object is not found.
-** *can_read* grants read-only access to the record.  Attempting to update or delete the record returns an error.  *can_read* does not allow a reader to see any permission grants on the object except the object's owner_uuid and the reader's own permissions.
-** *can_write* permits changes to the record (but not permission links).  *can_write* permits the user to delete the object.  *can_write* also implies *can_read*.
-** *can_manage* permits the user to read, create, update and delete permission links whose @head_uuid@ is this object's @uuid@.  *can_manage* also implies *can_write* and *can_read*.
+There are four levels of permission: *none*, *can_read*, *can_write*, and *can_manage*.
+
+* *none* is the default state when there are no other permission grants.
+** the object is not included in any list query response.
+** direct queries of the object by uuid return 404 Not Found.
+** Link objects require valid identifiers in @head_uuid@ and @tail_uuid@, so an attempt to create a Link that references an unreadable object will return an error indicating the object is not found.
+* *can_read* grants read-only access to the record.  Attempting to update or delete the record returns an error.
+** *can_read* does not allow a reader to see any permission grants on the object except the object's owner_uuid and the reader's own permissions.
+* *can_write* permits changes to the record, including changing ownership and deleting the object.
+** *can_write* cannot read, create, update or delete permission links associated with the object.
+** *can_write* also implies *can_read*.
+* *can_manage* permits the user to read, create, update and delete permission links whose @head_uuid@ is this object's @uuid@.
+** *can_manage* also implies *can_write* and *can_read*.
 
 h2. Ownership
 
-* All Arvados objects have an @owner_uuid@ field. Valid uuid types for @owner_uuid@ are "User" and "Group".
-* The User or Group specified by @owner_uuid@ has *can_manage* permission on the object.
-** This permission is one way: A User or Group's @owner_uuid@ being equal to @X@ does not imply any permission for that User/Group to read, write, or manage an object whose @uuid@ is equal to @X@.
-* Applications should represent each object as belonging to, or being "inside", the Group/User referenced by its @owner_uuid@.
-** A "project" is a subtype of Group that is treated as a "Project" in Workbench, and as a directory by @arv-mount@.
-** A "role" is a subtype of Group that is treated in Workbench as a group of users who have permissions in common (typically an organizational group).
-* To change the @owner_uuid@ field, it is necessary to have @can_write@ permission on both the current owner and the new owner.
+All Arvados objects have an @owner_uuid@ field. Valid uuid types for @owner_uuid@ are "User" and "Group".  For Group, the @group_class@ must be a "project".
+
+The User or Group specified by @owner_uuid@ has *can_manage* permission on the object.  This permission is one way: an object that is owned does not get any special permissions on the User or Group that owns it.
+
+To change the @owner_uuid@ field, it is necessary to have @can_write@ permission on both the current owner and the new owner.
 
 h2(#links). Permission links
 
-A link object with
+A permission link is a link object with:
 
 * @owner_uuid@ of the system user.
 * @link_class@ "permission"
 * @name@ one of *can_read*, *can_write* or *can_manage*
 * @head_uuid@ of some Arvados object
-* @tail_uuid@ of a User or Group
+* @tail_uuid@ of a User or Group.  For Group, the @group_class@ must be a "role".
 
-grants the @name@ permission for @tail_uuid@ accessing @head_uuid@
+This grants the permission in @name@ for @tail_uuid@ accessing @head_uuid@.
 
-* If a User has *can_manage* permission on some object, this grants permission to read, create, update and delete permission links where the @head_uuid@ is the object under management.
+If a User has *can_manage* permission on some object, the user has the ability to read, create, update and delete permission links with @head_uuid@ of the managed object.  In other words, the user has the ability to modify the permission grants on the object.
 
 h3. Transitive permissions
 
-Permissions can be obtained indirectly through Groups.
-* If a User X *can_read* Group A, and Group A *can_read* Object B, then User X *can_read* Object B.
+Permissions can be obtained indirectly through nested ownership (*can_manage*) or by following multiple permission links.
+
+* If a User X owns project A, and project A owns project B, then User X *can_manage* project B.
+* If a User X *can_read* role A, and role A *can_read* Object B, then User X *can_read* Object B.
 * Permissions are narrowed to the least powerful permission on the path.
-** If User X *can_write* Group A, and Group A *can_read* Object B, then User X *can_read* Object B.
-** If User X *can_read* Group A, and Group A *can_write* Object B, then User X *can_read* Object B.
+** If User X *can_write* role A, and role A *can_read* Object B, then User X *can_read* Object B.
+** If User X *can_read* role A, and role A *can_write* Object B, then User X *can_read* Object B.
+
+h2. Projects and Roles
+
+A "project" is a subtype of Group that is displayed as a "Project" in Workbench, and as a directory by @arv-mount@.
+* A project can own things (appear in @owner_uuid@)
+* A project can be owned by a user or another project.
+* The name of a project is unique only among projects with the same owner_uuid.
+* Projects can be targets (@head_uuid@) of permission links, but not origins (@tail_uuid@).  Putting a project in a @tail_uuid@ field is an error.
+
+A "role" is a subtype of Group that is treated in Workbench as a group of users who have permissions in common (typically an organizational group).
+* A role cannot own things (cannot appear in @owner_uuid@).  Putting a role in an @owner_uuid@ field is an error.
+* All roles are owned by the system user.
+* The name of a role is unique across a single Arvados cluster.
+* Roles can be both targets (@head_uuid@) and origins (@tail_uuid@) of permission links.
+
+h3. Access through Roles
 
-h2. Group Membership
+A "role" consists of a set of users or other roles that have that role, and a set of permissions (primarily read/write/manage access to projects) the role grants.
 
-Group membership is determined by whether the group has *can_read* permission on an object.  If a group G *can_read* an object A, then we say A is a member of G.
+If there is a permission link stating that user A *can_write* role R, then we say A has role R.  This means user A has up to *can_write* access to everything the role has access to.
 
-For some kinds of groups, like roles, it is natural for users who are members of a group to also have *can_manage* permission on the group, i.e., G *can_read* A  and A *can_manage* G ("A can do anything G can do"). However, this is not necessary: A can be a member of a group while being unable to even read it.
+Because permissions are one-way, the links A *can_write* R and B *can_write* R does not imply that user A and B will be able to see each other.  For users in a role to see each other, read permission should be added going in the opposite direction: R *can_read* A and R *can_read* B.
+
+If a user needs to be able to manipulate permissions of objects that are accessed through the role (for example, to share project P with a user outside the role), then role R must have *can_manage* permission on project P (R *can_manage* P) and the user must be granted *can_manage* permission on R (A *can_manage* R).
 
 h2. Special cases
 
-* Log table objects are additionally readable based on whether the User has *can_read* permission on @object_uuid@ (User can access log history about objects it can read).  To retain the integrity of the log, the log table should deny all update or delete operations.
-* Permission links where @tail_uuid@ is a User permit @can_read@ on the link by that user.  (User can discover her own permission grants.)
-* *can_read* on a Collection grants permission to read the blocks that make up the collection (API server returns signed blocks)
-* If User or Group X *can_FOO* Group A, and Group A *can_manage* User B, then X *can_FOO* _everything that User B can_FOO_.
+Log table objects are additionally readable based on whether the User has *can_read* permission on @object_uuid@ (User can access log history about objects it can read).  To retain the integrity of the log, the log table denies all update or delete operations.
+
+Permission links where @tail_uuid@ is a User allow *can_read* on the link record by that user (User can discover her own permission grants.)
+
+At least *can_read* on a Collection grants permission to read the blocks that make up the collection (API server returns signed blocks).
+
+A user can only read a container record if the user has read permission to a container_request with that container_uuid.
+
+*can_read* and *can_write* access on a user grants access to the user record, but not anything owned by the user.
+*can_manage* access to a user grants can_manage access to the user, _and everything owned by that user_ .
+If a user A *can_read* role R, and role R *can_manage* user B, then user A *can_read* user B _and everything owned by that user_ .
 
 h2(#system). System user and group
 
@@ -70,7 +100,9 @@ A privileged user account exists for the use by internal Arvados components.  Th
 
 h2. Anoymous user and group
 
-An Arvados site may be configured to allow users to browse resources without requiring a login.  In this case, permissions for non-logged-in users are associated with the "anonymous" user.  To make objects visible to the public, they can be shared with the "anonymous" group.  The anonymous user uuid is @{siteprefix}-tpzed-anonymouspublic@.  The anonymous group uuid is @{siteprefix}-j7d0g-anonymouspublic@.
+An Arvados site may be configured to allow users to browse resources without requiring a login.  In this case, permissions for non-logged-in users are associated with the "anonymous" user.  To make objects visible to anyone (both logged-in and non-logged-in users), they can be shared with the "anonymous" role.  Note that objects shared with the "anonymous" user will only be visible to non-logged-in users!
+
+The anonymous user uuid is @{siteprefix}-tpzed-anonymouspublic@.  The anonymous group uuid is @{siteprefix}-j7d0g-anonymouspublic@.
 
 h2. Example
 
index 8afebfb79eab56e24e83394f3923469d28faba94..01a31adb91967c0cb3648e364b3af7c891fd28f0 100644 (file)
@@ -574,6 +574,9 @@ class ArvadosModel < ApplicationRecord
           logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
           errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
           raise PermissionDeniedError
+        elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project"
+          errors.add :owner_uuid, "must be a project"
+          raise PermissionDeniedError
         end
       end
     else
@@ -582,7 +585,7 @@ class ArvadosModel < ApplicationRecord
       # itself.
       if !current_user.can?(write: self.uuid)
         logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission"
-        errors.add :uuid, "is not writable"
+        errors.add :uuid, " #{uuid} is not writable by #{current_user.uuid}"
         raise PermissionDeniedError
       end
     end
index 36814a3163e074433dfb3f5beeffb77510740475..02c6a242f911ddcaebd3a4ae68113c546d5487bd 100644 (file)
@@ -17,6 +17,7 @@ class Group < ArvadosModel
   attribute :properties, :jsonbHash, default: {}
 
   validate :ensure_filesystem_compatible_name
+  validate :check_group_class
   before_create :assign_name
   after_create :after_ownership_change
   after_create :update_trash
@@ -24,6 +25,8 @@ class Group < ArvadosModel
   before_update :before_ownership_change
   after_update :after_ownership_change
 
+  after_create :add_role_manage_link
+
   after_update :update_trash
   before_destroy :clear_permissions_and_trash
 
@@ -44,6 +47,15 @@ class Group < ArvadosModel
     super if group_class == 'project'
   end
 
+  def check_group_class
+    if group_class != 'project' && group_class != 'role'
+      errors.add :group_class, "value must be one of 'project' or 'role', was '#{group_class}'"
+    end
+    if group_class_changed? && !group_class_was.nil?
+      errors.add :group_class, "cannot be modified after record is created"
+    end
+  end
+
   def update_trash
     if trash_at_changed? or owner_uuid_changed?
       # The group was added or removed from the trash.
@@ -104,4 +116,33 @@ delete from trashed_groups where group_uuid=$1
     end
     true
   end
+
+  def ensure_owner_uuid_is_permitted
+    if group_class == "role"
+      @requested_manager_uuid = nil
+      if new_record?
+        @requested_manager_uuid = owner_uuid
+        self.owner_uuid = system_user_uuid
+        return true
+      end
+      if self.owner_uuid != system_user_uuid
+        raise "Owner uuid for role must be system user"
+      end
+      raise PermissionDeniedError unless current_user.can?(manage: uuid)
+      true
+    else
+      super
+    end
+  end
+
+  def add_role_manage_link
+    if group_class == "role" && @requested_manager_uuid
+      act_as_system_user do
+       Link.create!(tail_uuid: @requested_manager_uuid,
+                    head_uuid: self.uuid,
+                    link_class: "permission",
+                    name: "can_manage")
+      end
+    end
+  end
 end
index 21d89767c7139a0c8b7ae8eb67595d6ebe336110..e4ba7f3de1ef8f20833355efb0dae1a153b05113 100644 (file)
@@ -12,8 +12,8 @@ class Link < ArvadosModel
   attribute :properties, :jsonbHash, default: {}
 
   validate :name_links_are_obsolete
-  before_create :permission_to_attach_to_objects
-  before_update :permission_to_attach_to_objects
+  validate :permission_to_attach_to_objects
+  before_update :restrict_alter_permissions
   after_update :call_update_permissions
   after_create :call_update_permissions
   before_destroy :clear_permissions
@@ -50,13 +50,37 @@ class Link < ArvadosModel
     # All users can write links that don't affect permissions
     return true if self.link_class != 'permission'
 
+    if PERM_LEVEL[self.name].nil?
+      errors.add(:name, "is invalid permission, must be one of 'can_read', 'can_write', 'can_manage', 'can_login'")
+      return false
+    end
+
+    rsc_class = ArvadosModel::resource_class_for_uuid tail_uuid
+    if rsc_class == Group
+      tail_obj = Group.find_by_uuid(tail_uuid)
+      if tail_obj.nil?
+        errors.add(:tail_uuid, "does not exist")
+        return false
+      end
+      if tail_obj.group_class != "role"
+        errors.add(:tail_uuid, "must be a user or role, was group with group_class #{tail_obj.group_class}")
+        return false
+      end
+    elsif rsc_class != User
+      errors.add(:tail_uuid, "must be a user or role")
+      return false
+    end
+
     # Administrators can grant permissions
     return true if current_user.is_admin
 
     head_obj = ArvadosModel.find_by_uuid(head_uuid)
 
     # No permission links can be pointed to past collection versions
-    return false if head_obj.is_a?(Collection) && head_obj.current_version_uuid != head_uuid
+    if head_obj.is_a?(Collection) && head_obj.current_version_uuid != head_uuid
+      errors.add(:head_uuid, "cannot point to a past version of a collection")
+      return false
+    end
 
     # All users can grant permissions on objects they own or can manage
     return true if current_user.can?(manage: head_obj)
@@ -65,6 +89,16 @@ class Link < ArvadosModel
     false
   end
 
+  def restrict_alter_permissions
+    return true if self.link_class != 'permission' && self.link_class_was != 'permission'
+
+    return true if current_user.andand.uuid == system_user.uuid
+
+    if link_class_changed? || tail_uuid_changed? || head_uuid_changed?
+      raise "Can only alter permission link level"
+    end
+  end
+
   PERM_LEVEL = {
     'can_read' => 1,
     'can_login' => 1,
diff --git a/services/api/db/migrate/20200602141328_fix_roles_projects.rb b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
new file mode 100644 (file)
index 0000000..e17ef6d
--- /dev/null
@@ -0,0 +1,17 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'fix_roles_projects'
+
+class FixRolesProjects < ActiveRecord::Migration[5.0]
+  def up
+    # defined in a function for easy testing.
+    fix_roles_projects
+  end
+
+  def down
+    # This migration is not reversible.  However, the results are
+    # backwards compatible.
+  end
+end
index 469475ad9604232fbeafc76889c4665b6cd77b80..83987d051859843a8df981cf539f8514daecc15b 100644 (file)
@@ -3196,6 +3196,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20190808145904'),
 ('20190809135453'),
 ('20190905151603'),
-('20200501150153');
+('20200501150153'),
+('20200602141328');
 
 
index c7b48c0cdd6ff1ed0056a32e49f65c106afc100c..98112c858b98445c9bacb8c9c614343f8a7f5ef1 100644 (file)
@@ -90,7 +90,8 @@ module CurrentApiClient
         ActiveRecord::Base.transaction do
           Group.where(uuid: system_group_uuid).
             first_or_create!(name: "System group",
-                             description: "System group") do |g|
+                             description: "System group",
+                             group_class: "role") do |g|
             g.save!
             User.all.collect(&:uuid).each do |user_uuid|
               Link.create!(link_class: 'permission',
@@ -188,7 +189,7 @@ module CurrentApiClient
     end
   end
 
-  def empty_collection_uuid
+  def empty_collection_pdh
     'd41d8cd98f00b204e9800998ecf8427e+0'
   end
 
@@ -197,8 +198,16 @@ module CurrentApiClient
       act_as_system_user do
         ActiveRecord::Base.transaction do
           Collection.
-            where(portable_data_hash: empty_collection_uuid).
-            first_or_create!(manifest_text: '', owner_uuid: anonymous_group.uuid)
+            where(portable_data_hash: empty_collection_pdh).
+            first_or_create(manifest_text: '', owner_uuid: system_user.uuid, name: "empty collection") do |c|
+            c.save!
+            Link.where(tail_uuid: anonymous_group.uuid,
+                       head_uuid: c.uuid,
+                       link_class: 'permission',
+                       name: 'can_read').
+                  first_or_create!
+            c
+          end
         end
       end
     end
diff --git a/services/api/lib/fix_roles_projects.rb b/services/api/lib/fix_roles_projects.rb
new file mode 100644 (file)
index 0000000..5dd127b
--- /dev/null
@@ -0,0 +1,73 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+include CurrentApiClient
+
+def fix_roles_projects
+  batch_update_permissions do
+    # This migration is not reversible.  However, the behavior it
+    # enforces is backwards-compatible, and most of the time there
+    # shouldn't be anything to do at all.
+    act_as_system_user do
+      ActiveRecord::Base.transaction do
+        Group.where("group_class != 'project' or group_class is null").each do |g|
+          # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups)
+          old_owner = g.owner_uuid
+          g.owner_uuid = system_user_uuid
+          g.group_class = 'role'
+          g.save_with_unique_name!
+
+          if old_owner != system_user_uuid
+            # 2) Ownership of a role becomes a can_manage link
+            Link.create!(link_class: 'permission',
+                         name: 'can_manage',
+                         tail_uuid: old_owner,
+                         head_uuid: g.uuid)
+          end
+        end
+
+        ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |klass|
+          next if [ApiClientAuthorization,
+                   AuthorizedKey,
+                   Log,
+                   Group].include?(klass)
+          next if !klass.columns.collect(&:name).include?('owner_uuid')
+
+          # 3) If a role owns anything, give it to system user and it
+          # becomes a can_manage link
+          klass.joins("join groups on groups.uuid=#{klass.table_name}.owner_uuid and groups.group_class='role'").each do |owned|
+            Link.create!(link_class: 'permission',
+                         name: 'can_manage',
+                         tail_uuid: owned.owner_uuid,
+                         head_uuid: owned.uuid)
+            owned.owner_uuid = system_user_uuid
+            owned.save_with_unique_name!
+          end
+        end
+
+        Group.joins("join groups as g2 on g2.uuid=groups.owner_uuid and g2.group_class='role'").each do |owned|
+          Link.create!(link_class: 'permission',
+                       name: 'can_manage',
+                       tail_uuid: owned.owner_uuid,
+                       head_uuid: owned.uuid)
+          owned.owner_uuid = system_user_uuid
+          owned.save_with_unique_name!
+        end
+
+        # 4) Projects can't have outgoing permission links.  Just
+        # print a warning and delete them.
+        q = ActiveRecord::Base.connection.exec_query %{
+select links.uuid from links, groups where groups.uuid = links.tail_uuid and
+       links.link_class = 'permission' and groups.group_class = 'project'
+}
+        q.each do |lu|
+          ln = Link.find_by_uuid(lu['uuid'])
+          puts "WARNING: Projects cannot have outgoing permission links, removing '#{ln.name}' link #{ln.uuid} from #{ln.tail_uuid} to #{ln.head_uuid}"
+          Rails.logger.warn "Projects cannot have outgoing permission links, removing '#{ln.name}' link #{ln.uuid} from #{ln.tail_uuid} to #{ln.head_uuid}"
+          ln.destroy!
+        end
+      end
+    end
+  end
+end
index 4c2e72d9553c3b6e110c9c3c6a0360afebe64390..7b1b900cacbcae00a4d44ce5d8f72d02b213feb3 100644 (file)
@@ -8,6 +8,8 @@ REVOKE_PERM = 0
 CAN_MANAGE_PERM = 3
 
 def update_permissions perm_origin_uuid, starting_uuid, perm_level, edge_id=nil
+  return if Thread.current[:suppress_update_permissions]
+
   #
   # Update a subset of the permission table affected by adding or
   # removing a particular permission relationship (ownership or a
@@ -140,7 +142,7 @@ end
 
 def check_permissions_against_full_refresh
   # No-op except when running tests
-  return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh]
+  return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh] and !Thread.current[:suppress_update_permissions]
 
   # For checking correctness of the incremental permission updates.
   # Check contents of the current 'materialized_permission' table
@@ -191,6 +193,17 @@ def skip_check_permissions_against_full_refresh
   end
 end
 
+def batch_update_permissions
+  check_perm_was = Thread.current[:suppress_update_permissions]
+  Thread.current[:suppress_update_permissions] = true
+  begin
+    yield
+  ensure
+    Thread.current[:suppress_update_permissions] = check_perm_was
+    refresh_permissions
+  end
+end
+
 # Used to account for permissions that a user gains by having
 # can_manage on another user.
 #
index 1581039bb388638ee0fc56decc7ce0940ea49f4d..a16ee8763f3f32016e76af30f74da1fda86be186 100644 (file)
@@ -199,7 +199,6 @@ unlinked_docker_image:
 empty:
   uuid: zzzzz-4zz18-gs9ooj1h9sd5mde
   current_version_uuid: zzzzz-4zz18-gs9ooj1h9sd5mde
-  # Empty collection owned by anonymous_group is added with rake db:seed.
   portable_data_hash: d41d8cd98f00b204e9800998ecf8427e+0
   owner_uuid: zzzzz-tpzed-000000000000000
   created_at: 2014-06-11T17:22:54Z
@@ -208,7 +207,7 @@ empty:
   modified_at: 2014-06-11T17:22:54Z
   updated_at: 2014-06-11T17:22:54Z
   manifest_text: ""
-  name: empty_collection
+  name: "empty collection for python test"
 
 foo_collection_in_aproject:
   uuid: zzzzz-4zz18-fy296fx3hot09f7
index 22c999ecd752ecea18160b807f5950ee94a8a521..ee0d786bbe2f1537a9ef904df51eba7f221eea75 100644 (file)
@@ -6,19 +6,33 @@ public:
   uuid: zzzzz-j7d0g-it30l961gq3t0oi
   owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
   name: Public
-  description: Public Group
+  description: Public Project
+  group_class: project
+
+public_role:
+  uuid: zzzzz-j7d0g-jt30l961gq3t0oi
+  owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  name: Public Role
+  description: Public Role
   group_class: role
 
 private:
   uuid: zzzzz-j7d0g-rew6elm53kancon
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
   name: Private
-  description: Private Group
+  description: Private Project
+  group_class: project
+
+private_role:
+  uuid: zzzzz-j7d0g-pew6elm53kancon
+  owner_uuid: zzzzz-tpzed-000000000000000
+  name: Private Role
+  description: Private Role
   group_class: role
 
 private_and_can_read_foofile:
   uuid: zzzzz-j7d0g-22xp1wpjul508rk
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  owner_uuid: zzzzz-tpzed-000000000000000
   name: Private and Can Read Foofile
   description: Another Private Group
   group_class: role
@@ -88,7 +102,7 @@ asubproject:
 
 future_project_viewing_group:
   uuid: zzzzz-j7d0g-futrprojviewgrp
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  owner_uuid: zzzzz-tpzed-000000000000000
   created_at: 2014-04-21 15:37:48 -0400
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
@@ -107,6 +121,7 @@ bad_group_has_ownership_cycle_a:
   modified_at: 2014-05-03 18:50:08 -0400
   updated_at: 2014-05-03 18:50:08 -0400
   name: Owned by bad group b
+  group_class: project
 
 bad_group_has_ownership_cycle_b:
   uuid: zzzzz-j7d0g-0077nzts8c178lw
@@ -117,6 +132,7 @@ bad_group_has_ownership_cycle_b:
   modified_at: 2014-05-03 18:50:08 -0400
   updated_at: 2014-05-03 18:50:08 -0400
   name: Owned by bad group a
+  group_class: project
 
 anonymous_group:
   uuid: zzzzz-j7d0g-anonymouspublic
@@ -246,17 +262,6 @@ fuse_owned_project:
   description: Test project belonging to FUSE test user
   group_class: project
 
-group_with_no_class:
-  uuid: zzzzz-j7d0g-groupwithnoclas
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
-  created_at: 2014-04-21 15:37:48 -0400
-  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
-  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
-  modified_at: 2014-04-21 15:37:48 -0400
-  updated_at: 2014-04-21 15:37:48 -0400
-  name: group_with_no_class
-  description: This group has no class at all. So rude!
-
 # This wouldn't pass model validation, but it enables a workbench
 # infinite-loop test. See #4389
 project_owns_itself:
index 2f66433379ed82db8cc95fa1e395d4c020420cec..b30d1edc259833b0350bb949fa0c00249c8f051b 100644 (file)
@@ -198,20 +198,6 @@ foo_file_readable_by_active_redundant_permission_via_private_group:
   head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w
   properties: {}
 
-foo_file_readable_by_aproject:
-  uuid: zzzzz-o0j2j-fp1d8395ldqw22p
-  owner_uuid: zzzzz-tpzed-000000000000000
-  created_at: 2014-01-24 20:42:26 -0800
-  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
-  modified_by_user_uuid: zzzzz-tpzed-000000000000000
-  modified_at: 2014-01-24 20:42:26 -0800
-  updated_at: 2014-01-24 20:42:26 -0800
-  tail_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
-  link_class: permission
-  name: can_read
-  head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w
-  properties: {}
-
 bar_file_readable_by_active:
   uuid: zzzzz-o0j2j-8hppiuduf8eqdng
   owner_uuid: zzzzz-tpzed-000000000000000
@@ -1111,3 +1097,17 @@ tagged_collection_readable_by_spectator:
   name: can_read
   head_uuid: zzzzz-4zz18-taggedcolletion
   properties: {}
+
+active_manages_viewing_group:
+  uuid: zzzzz-o0j2j-activemanagesvi
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-000000000000000
+  modified_at: 2014-01-24 20:42:26 -0800
+  updated_at: 2014-01-24 20:42:26 -0800
+  tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  link_class: permission
+  name: can_manage
+  head_uuid: zzzzz-j7d0g-futrprojviewgrp
+  properties: {}
index 175a8f71ea0544e2253754f607a2217a441d63cc..2cfa054448c29fcbbe3beb0b80edc37af514eb2e 100644 (file)
@@ -100,7 +100,7 @@ class ApplicationControllerTest < ActionController::TestCase
         @controller = Arvados::V1::GroupsController.new
         authorize_with :active
         post :create, params: {
-          group: {},
+          group: {group_class: "project"},
           ensure_unique_name: boolparam
         }
         assert_response :success
@@ -113,7 +113,8 @@ class ApplicationControllerTest < ActionController::TestCase
         post :create, params: {
           group: {
             name: groups(:aproject).name,
-            owner_uuid: groups(:aproject).owner_uuid
+            owner_uuid: groups(:aproject).owner_uuid,
+            group_class: "project"
           },
           ensure_unique_name: boolparam
         }
index b30afd745345df01fdb986d8ead8b8c7ad2ab0c4..26270b1c3c9c9b4da0ec4c03f6a8d6fd861fbe70 100644 (file)
@@ -6,16 +6,16 @@ require 'test_helper'
 
 class Arvados::V1::FiltersTest < ActionController::TestCase
   test '"not in" filter passes null values' do
-    @controller = Arvados::V1::GroupsController.new
+    @controller = Arvados::V1::ContainerRequestsController.new
     authorize_with :admin
     get :index, params: {
-      filters: [ ['group_class', 'not in', ['project']] ],
-      controller: 'groups',
+      filters: [ ['container_uuid', 'not in', ['zzzzz-dz642-queuedcontainer', 'zzzzz-dz642-runningcontainr']] ],
+      controller: 'container_requests',
     }
     assert_response :success
     found = assigns(:objects)
-    assert_includes(found.collect(&:group_class), nil,
-                    "'group_class not in ['project']' filter should pass null")
+    assert_includes(found.collect(&:container_uuid), nil,
+                    "'container_uuid not in [zzzzz-dz642-queuedcontainer, zzzzz-dz642-runningcontainr]' filter should pass null")
   end
 
   test 'error message for non-array element in filters array' do
index 2b5e8d5a9d099947adbb21bec74c8508fd456d16..ff89cd2129b31ad5357d54066262a80cd702e41c 100644 (file)
@@ -29,8 +29,9 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     end
     assert_includes group_uuids, groups(:aproject).uuid
     assert_includes group_uuids, groups(:asubproject).uuid
+    assert_includes group_uuids, groups(:private).uuid
     assert_not_includes group_uuids, groups(:system_group).uuid
-    assert_not_includes group_uuids, groups(:private).uuid
+    assert_not_includes group_uuids, groups(:private_and_can_read_foofile).uuid
   end
 
   test "get list of groups that are not projects" do
@@ -44,8 +45,6 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     end
     assert_not_includes group_uuids, groups(:aproject).uuid
     assert_not_includes group_uuids, groups(:asubproject).uuid
-    assert_includes group_uuids, groups(:private).uuid
-    assert_includes group_uuids, groups(:group_with_no_class).uuid
   end
 
   test "get list of groups with bogus group_class" do
@@ -746,20 +745,23 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     assert_equal 0, json_response['included'].length
   end
 
-  test 'get shared, owned by non-project' do
+  test 'get shared, add permission link' do
     authorize_with :user_bar_in_sharing_group
 
     act_as_system_user do
-      Group.find_by_uuid(groups(:project_owned_by_foo).uuid).update!(owner_uuid: groups(:group_for_sharing_tests).uuid)
+      Link.create!(tail_uuid: groups(:group_for_sharing_tests).uuid,
+                   head_uuid: groups(:project_owned_by_foo).uuid,
+                   link_class: 'permission',
+                   name: 'can_manage')
     end
 
     get :shared, params: {:filters => [["group_class", "=", "project"]], :include => "owner_uuid"}
 
     assert_equal 1, json_response['items'].length
-    assert_equal json_response['items'][0]["uuid"], groups(:project_owned_by_foo).uuid
+    assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
 
     assert_equal 1, json_response['included'].length
-    assert_equal json_response['included'][0]["uuid"], groups(:group_for_sharing_tests).uuid
+    assert_equal users(:user_foo_in_sharing_group).uuid, json_response['included'][0]["uuid"]
   end
 
   ### contents with exclude_home_project
@@ -810,20 +812,23 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     assert_equal 0, json_response['included'].length
   end
 
-  test 'contents, exclude home, owned by non-project' do
+  test 'contents, exclude home, add permission link' do
     authorize_with :user_bar_in_sharing_group
 
     act_as_system_user do
-      Group.find_by_uuid(groups(:project_owned_by_foo).uuid).update!(owner_uuid: groups(:group_for_sharing_tests).uuid)
+      Link.create!(tail_uuid: groups(:group_for_sharing_tests).uuid,
+                   head_uuid: groups(:project_owned_by_foo).uuid,
+                   link_class: 'permission',
+                   name: 'can_manage')
     end
 
     get :contents, params: {:include => "owner_uuid", :exclude_home_project => true}
 
     assert_equal 1, json_response['items'].length
-    assert_equal json_response['items'][0]["uuid"], groups(:project_owned_by_foo).uuid
+    assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
 
     assert_equal 1, json_response['included'].length
-    assert_equal json_response['included'][0]["uuid"], groups(:group_for_sharing_tests).uuid
+    assert_equal users(:user_foo_in_sharing_group).uuid, json_response['included'][0]["uuid"]
   end
 
   test 'contents, exclude home, with parent specified' do
index cfcd917d6538ec2eacd584cc2ac18b5c1afee7e9..84bd846c912fa1c897d47ab62d4eaed1dab4d2dd 100644 (file)
@@ -150,7 +150,7 @@ class Arvados::V1::RepositoriesControllerTest < ActionController::TestCase
   test "get_all_permissions obeys group permissions" do
     act_as_user system_user do
       r = Repository.create!(name: 'admin/groupcanwrite', owner_uuid: users(:admin).uuid)
-      g = Group.create!(group_class: 'group', name: 'repo-writers')
+      g = Group.create!(group_class: 'role', name: 'repo-writers')
       u1 = users(:active)
       u2 = users(:spectator)
       Link.create!(tail_uuid: g.uuid, head_uuid: r.uuid, link_class: 'permission', name: 'can_manage')
@@ -158,7 +158,7 @@ class Arvados::V1::RepositoriesControllerTest < ActionController::TestCase
       Link.create!(tail_uuid: u2.uuid, head_uuid: g.uuid, link_class: 'permission', name: 'can_read')
 
       r = Repository.create!(name: 'admin/groupreadonly', owner_uuid: users(:admin).uuid)
-      g = Group.create!(group_class: 'group', name: 'repo-readers')
+      g = Group.create!(group_class: 'role', name: 'repo-readers')
       u1 = users(:active)
       u2 = users(:spectator)
       Link.create!(tail_uuid: g.uuid, head_uuid: r.uuid, link_class: 'permission', name: 'can_read')
index 817a1c9ef944eb38e2dc708d53a199a9e70e5e0f..0ce9f1137f3fad8592e16318720c3b13d00406d3 100644 (file)
@@ -660,7 +660,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
 
   test "non-admin user gets only safe attributes from users#show" do
     g = act_as_system_user do
-      create :group
+      create :group, group_class: "role"
     end
     users = create_list :active_user, 2, join_groups: [g]
     token = create :token, user: users[0]
@@ -672,7 +672,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
   [2, 4].each do |limit|
     test "non-admin user can limit index to #{limit}" do
       g = act_as_system_user do
-        create :group
+        create :group, group_class: "role"
       end
       users = create_list :active_user, 4, join_groups: [g]
       token = create :token, user: users[0]
index 445670a3d51572827289bb0dc37430168cd4eb9f..7021761278d72143c277b06622504eae3c593334 100644 (file)
@@ -206,7 +206,8 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
     post "/arvados/v1/groups",
       params: {
         group: {
-          name: name
+          name: name,
+          group_class: "project"
         },
         async: true
       },
index eec41aa0857fd481d600c69fa51b9514241057cf..66a62543bb4364590019d42697bae5220e75fc93 100644 (file)
@@ -6,7 +6,6 @@ require 'test_helper'
 
 class PermissionsTest < ActionDispatch::IntegrationTest
   include DbCurrentTime
-  include CurrentApiClient  # for empty_collection
   fixtures :users, :groups, :api_client_authorizations, :collections
 
   test "adding and removing direct can_read links" do
@@ -88,7 +87,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
           tail_uuid: users(:spectator).uuid,
           link_class: 'permission',
           name: 'can_read',
-          head_uuid: groups(:private).uuid,
+          head_uuid: groups(:private_role).uuid,
           properties: {}
         }
       },
@@ -106,7 +105,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       params: {
         :format => :json,
         :link => {
-          tail_uuid: groups(:private).uuid,
+          tail_uuid: groups(:private_role).uuid,
           link_class: 'permission',
           name: 'can_read',
           head_uuid: collections(:foo_file).uuid,
@@ -150,7 +149,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       params: {
         :format => :json,
         :link => {
-          tail_uuid: groups(:private).uuid,
+          tail_uuid: groups(:private_role).uuid,
           link_class: 'permission',
           name: 'can_read',
           head_uuid: collections(:foo_file).uuid,
@@ -174,7 +173,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
           tail_uuid: users(:spectator).uuid,
           link_class: 'permission',
           name: 'can_read',
-          head_uuid: groups(:private).uuid,
+          head_uuid: groups(:private_role).uuid,
           properties: {}
         }
       },
@@ -217,7 +216,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
           tail_uuid: users(:spectator).uuid,
           link_class: 'permission',
           name: 'can_read',
-          head_uuid: groups(:private).uuid,
+          head_uuid: groups(:private_role).uuid,
           properties: {}
         }
       },
@@ -229,7 +228,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       params: {
         :format => :json,
         :link => {
-          tail_uuid: groups(:private).uuid,
+          tail_uuid: groups(:private_role).uuid,
           link_class: 'permission',
           name: 'can_read',
           head_uuid: groups(:empty_lonely_group).uuid,
@@ -441,7 +440,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
   test "active user can read the empty collection" do
     # The active user should be able to read the empty collection.
 
-    get("/arvados/v1/collections/#{empty_collection_uuid}",
+    get("/arvados/v1/collections/#{empty_collection_pdh}",
       params: {:format => :json},
       headers: auth(:active))
     assert_response :success
index d0e6413b16ab195b0e8fe03ba865abba0a2b3541..e5d62cf4cf147eaad10cf6ffe913a037f7c6adac 100644 (file)
@@ -24,7 +24,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest
     act_as_system_user do
       puts("Time spent creating records:", Benchmark.measure do
              ActiveRecord::Base.transaction do
-               root = Group.create!(owner_uuid: users(:permission_perftest).uuid)
+               root = Group.create!(owner_uuid: users(:permission_perftest).uuid, group_class: "project")
                n += 1
                a = create_eight root.uuid
                n += 8
index d447c76c6d0053d1c52bd186e4498e80123eeac2..c1db8c8b5db1aa48fe4a843fb2a573f0b0966a3f 100644 (file)
@@ -97,7 +97,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
     while longstring.length < 2**16
       longstring = longstring + longstring
     end
-    g = Group.create! name: 'Has a long description', description: longstring
+    g = Group.create! name: 'Has a long description', description: longstring, group_class: "project"
     g = Group.find_by_uuid g.uuid
     assert_equal g.description, longstring
   end
@@ -248,7 +248,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
 
   test 'create and retrieve using created_at time' do
     set_user_from_auth :active
-    group = Group.create! name: 'test create and retrieve group'
+    group = Group.create! name: 'test create and retrieve group', group_class: "project"
     assert group.valid?, "group is not valid"
 
     results = Group.where(created_at: group.created_at)
@@ -258,7 +258,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
 
   test 'create and update twice and expect different update times' do
     set_user_from_auth :active
-    group = Group.create! name: 'test create and retrieve group'
+    group = Group.create! name: 'test create and retrieve group', group_class: "project"
     assert group.valid?, "group is not valid"
 
     # update 1
index 24d7333ab515cbdf127f1c5759db36006a473db6..3d1fda927f0554ce7955a4e73d7e0e8921f7d8a5 100644 (file)
@@ -3,6 +3,7 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 require 'test_helper'
+require 'fix_roles_projects'
 
 class GroupTest < ActiveSupport::TestCase
 
@@ -31,8 +32,8 @@ class GroupTest < ActiveSupport::TestCase
   test "cannot create a new ownership cycle" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar")
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", group_class: "project")
 
     g_foo.owner_uuid = g_bar.uuid
     assert g_foo.save, lambda { g_foo.errors.messages }
@@ -45,7 +46,7 @@ class GroupTest < ActiveSupport::TestCase
   test "cannot create a single-object ownership cycle" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
+    g_foo = Group.create!(name: "foo", group_class: "project")
     assert g_foo.save
 
     # Ensure I have permission to manage this group even when its owner changes
@@ -60,10 +61,47 @@ class GroupTest < ActiveSupport::TestCase
     assert g_foo.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
   end
 
+  test "cannot create a group that is not a 'role' or 'project'" do
+    set_user_from_auth :active_trustedclient
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      Group.create!(name: "foo")
+    end
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      Group.create!(name: "foo", group_class: "")
+    end
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      Group.create!(name: "foo", group_class: "bogus")
+    end
+  end
+
+  test "cannot change group_class on an already created group" do
+    set_user_from_auth :active_trustedclient
+    g = Group.create!(name: "foo", group_class: "role")
+    assert_raises(ActiveRecord::RecordInvalid) do
+      g.update_attributes!(group_class: "project")
+    end
+  end
+
+  test "role cannot own things" do
+    set_user_from_auth :active_trustedclient
+    role = Group.create!(name: "foo", group_class: "role")
+    assert_raises(ArvadosModel::PermissionDeniedError) do
+      Collection.create!(name: "bzzz123", owner_uuid: role.uuid)
+    end
+
+    c = Collection.create!(name: "bzzz124")
+    assert_raises(ArvadosModel::PermissionDeniedError) do
+      c.update_attributes!(owner_uuid: role.uuid)
+    end
+  end
+
   test "trash group hides contents" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
+    g_foo = Group.create!(name: "foo", group_class: "project")
     col = Collection.create!(owner_uuid: g_foo.uuid)
 
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
@@ -77,9 +115,9 @@ class GroupTest < ActiveSupport::TestCase
   test "trash group" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
-    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid, group_class: "project")
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid, group_class: "project")
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
@@ -98,9 +136,9 @@ class GroupTest < ActiveSupport::TestCase
   test "trash subgroup" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
-    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid, group_class: "project")
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid, group_class: "project")
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
@@ -118,9 +156,9 @@ class GroupTest < ActiveSupport::TestCase
   test "trash subsubgroup" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
-    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid, group_class: "project")
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid, group_class: "project")
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
@@ -168,7 +206,7 @@ class GroupTest < ActiveSupport::TestCase
   test "trashed does not propagate across permission links" do
     set_user_from_auth :admin
 
-    g_foo = Group.create!(name: "foo")
+    g_foo = Group.create!(name: "foo", group_class: "role")
     u_bar = User.create!(first_name: "bar")
 
     assert Group.readable_by(users(:admin)).where(uuid: g_foo.uuid).any?
@@ -237,7 +275,8 @@ class GroupTest < ActiveSupport::TestCase
     set_user_from_auth :active
     ["", "{SOLIDUS}"].each do |subst|
       Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
-      g = Group.create
+      proj = Group.create group_class: "project"
+      role = Group.create group_class: "role"
       [[nil, true],
        ["", true],
        [".", false],
@@ -248,12 +287,60 @@ class GroupTest < ActiveSupport::TestCase
        ["../..", subst != ""],
        ["/", subst != ""],
       ].each do |name, valid|
-        g.name = name
-        g.group_class = "role"
-        assert_equal true, g.valid?
-        g.group_class = "project"
-        assert_equal valid, g.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
+        role.name = name
+        assert_equal true, role.valid?
+        proj.name = name
+        assert_equal valid, proj.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
       end
     end
   end
+
+  def insert_group uuid, owner_uuid, name, group_class
+    q = ActiveRecord::Base.connection.exec_query %{
+insert into groups (uuid, owner_uuid, name, group_class, created_at, updated_at)
+       values ('#{uuid}', '#{owner_uuid}',
+               '#{name}', #{if group_class then "'"+group_class+"'" else 'NULL' end},
+               statement_timestamp(), statement_timestamp())
+}
+    uuid
+  end
+
+  test "migration to fix roles and projects" do
+    g1 = insert_group Group.generate_uuid, system_user_uuid, 'group with no class', nil
+    g2 = insert_group Group.generate_uuid, users(:active).uuid, 'role owned by a user', 'role'
+
+    g3 = insert_group Group.generate_uuid, system_user_uuid, 'role that owns a project', 'role'
+    g4 = insert_group Group.generate_uuid, g3, 'the project', 'project'
+
+    g5 = insert_group Group.generate_uuid, users(:active).uuid, 'a project with an outgoing permission link', 'project'
+
+    g6 = insert_group Group.generate_uuid, system_user_uuid, 'name collision', 'role'
+    g7 = insert_group Group.generate_uuid, users(:active).uuid, 'name collision', 'role'
+
+    refresh_permissions
+
+    act_as_system_user do
+      l1 = Link.create!(link_class: 'permission', name: 'can_manage', tail_uuid: g3, head_uuid: g4)
+      q = ActiveRecord::Base.connection.exec_query %{
+update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
+}
+    refresh_permissions
+    end
+
+    assert_equal nil, Group.find_by_uuid(g1).group_class
+    assert_equal users(:active).uuid, Group.find_by_uuid(g2).owner_uuid
+    assert_equal g3, Group.find_by_uuid(g4).owner_uuid
+    assert !Link.where(tail_uuid: users(:active).uuid, head_uuid: g2, link_class: "permission", name: "can_manage").any?
+    assert !Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any?
+    assert Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any?
+
+    fix_roles_projects
+
+    assert_equal 'role', Group.find_by_uuid(g1).group_class
+    assert_equal system_user_uuid, Group.find_by_uuid(g2).owner_uuid
+    assert_equal system_user_uuid, Group.find_by_uuid(g4).owner_uuid
+    assert Link.where(tail_uuid: users(:active).uuid, head_uuid: g2, link_class: "permission", name: "can_manage").any?
+    assert Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any?
+    assert !Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any?
+  end
 end
index ca02e2db5e3a08a8fb3ecdd79222e2835f8a5e2d..e356f4d9fa19806e9fc48f3f41f16bb90e200608 100644 (file)
@@ -21,7 +21,11 @@ class OwnerTest < ActiveSupport::TestCase
   Group.all
   [User, Group].each do |o_class|
     test "create object with legit #{o_class} owner" do
-      o = o_class.create!
+      if o_class == Group
+        o = o_class.create! group_class: "project"
+      else
+        o = o_class.create!
+      end
       i = Specimen.create(owner_uuid: o.uuid)
       assert i.valid?, "new item should pass validation"
       assert i.uuid, "new item should have an ID"
@@ -44,9 +48,19 @@ class OwnerTest < ActiveSupport::TestCase
 
     [User, Group].each do |new_o_class|
       test "change owner from legit #{o_class} to legit #{new_o_class} owner" do
-        o = o_class.create!
+        o = if o_class == Group
+              o_class.create! group_class: "project"
+            else
+              o_class.create!
+            end
         i = Specimen.create!(owner_uuid: o.uuid)
-        new_o = new_o_class.create!
+
+        new_o = if new_o_class == Group
+              new_o_class.create! group_class: "project"
+            else
+              new_o_class.create!
+            end
+
         assert(Specimen.where(uuid: i.uuid).any?,
                "new item should really be in DB")
         assert(i.update_attributes(owner_uuid: new_o.uuid),
@@ -55,7 +69,11 @@ class OwnerTest < ActiveSupport::TestCase
     end
 
     test "delete #{o_class} that owns nothing" do
-      o = o_class.create!
+      if o_class == Group
+        o = o_class.create! group_class: "project"
+      else
+        o = o_class.create!
+      end
       assert(o_class.where(uuid: o.uuid).any?,
              "new #{o_class} should really be in DB")
       assert(o.destroy, "should delete #{o_class} that owns nothing")
@@ -65,7 +83,11 @@ class OwnerTest < ActiveSupport::TestCase
 
     test "change uuid of #{o_class} that owns nothing" do
       # (we're relying on our admin credentials here)
-      o = o_class.create!
+      if o_class == Group
+        o = o_class.create! group_class: "project"
+      else
+        o = o_class.create!
+      end
       assert(o_class.where(uuid: o.uuid).any?,
              "new #{o_class} should really be in DB")
       old_uuid = o.uuid
index cb5ae7ba2f2367462229927e364e640913727141..10664474c68bf219a4cfb521a0431e97a21c5fdc 100644 (file)
@@ -46,7 +46,7 @@ class PermissionTest < ActiveSupport::TestCase
   end
 
   test "readable_by" do
-    set_user_from_auth :active_trustedclient
+    set_user_from_auth :admin
 
     ob = Collection.create!
     Link.create!(tail_uuid: users(:active).uuid,
@@ -57,7 +57,7 @@ class PermissionTest < ActiveSupport::TestCase
   end
 
   test "writable_by" do
-    set_user_from_auth :active_trustedclient
+    set_user_from_auth :admin
 
     ob = Collection.create!
     Link.create!(tail_uuid: users(:active).uuid,
@@ -153,6 +153,7 @@ class PermissionTest < ActiveSupport::TestCase
     set_user_from_auth :admin
 
     owner_grp = Group.create!(owner_uuid: users(:active).uuid, group_class: "role")
+
     sp_grp = Group.create!(group_class: "project")
 
     Link.create!(link_class: 'permission',
@@ -227,7 +228,7 @@ class PermissionTest < ActiveSupport::TestCase
     # anyone any additional permissions.)
     g = nil
     act_as_user manager do
-      g = create :group, name: "NoBigSecret Lab"
+      g = create :group, name: "NoBigSecret Lab", group_class: "role"
       assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
                    "saw a user I shouldn't see")
       assert_raises(ArvadosModel::PermissionDeniedError,
@@ -323,7 +324,7 @@ class PermissionTest < ActiveSupport::TestCase
                      "#{a.first_name} should not be able to see 'b' in the user list")
 
     act_as_system_user do
-      g = create :group
+      g = create :group, group_class: "role"
       [a,b].each do |u|
         create(:permission_link,
                name: 'can_read', tail_uuid: u.uuid, head_uuid: g.uuid)
@@ -423,7 +424,13 @@ class PermissionTest < ActiveSupport::TestCase
   test "add user to group, then remove them" do
     set_user_from_auth :admin
     grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
-    col = Collection.create!(owner_uuid: grp.uuid)
+    col = Collection.create!(owner_uuid: system_user_uuid)
+
+    l0 = Link.create!(tail_uuid: grp.uuid,
+                 head_uuid: col.uuid,
+                 link_class: 'permission',
+                 name: 'can_read')
+
     assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
     assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
 
@@ -459,7 +466,7 @@ class PermissionTest < ActiveSupport::TestCase
 
   test "add user to group, then change permission level" do
     set_user_from_auth :admin
-    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "project")
     col = Collection.create!(owner_uuid: grp.uuid)
     assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
     assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
@@ -468,10 +475,6 @@ class PermissionTest < ActiveSupport::TestCase
                  head_uuid: grp.uuid,
                  link_class: 'permission',
                  name: 'can_manage')
-    l2 = Link.create!(tail_uuid: grp.uuid,
-                 head_uuid: users(:active).uuid,
-                 link_class: 'permission',
-                 name: 'can_read')
 
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).first
     assert users(:active).can?(read: col.uuid)
@@ -498,7 +501,7 @@ class PermissionTest < ActiveSupport::TestCase
 
   test "add user to group, then add overlapping permission link to group" do
     set_user_from_auth :admin
-    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "project")
     col = Collection.create!(owner_uuid: grp.uuid)
     assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
     assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
@@ -507,10 +510,6 @@ class PermissionTest < ActiveSupport::TestCase
                  head_uuid: grp.uuid,
                  link_class: 'permission',
                  name: 'can_manage')
-    l2 = Link.create!(tail_uuid: grp.uuid,
-                 head_uuid: users(:active).uuid,
-                 link_class: 'permission',
-                 name: 'can_read')
 
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).first
     assert users(:active).can?(read: col.uuid)
@@ -538,8 +537,14 @@ class PermissionTest < ActiveSupport::TestCase
 
   test "add user to group, then add overlapping permission link to subproject" do
     set_user_from_auth :admin
-    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "project")
-    prj = Group.create!(owner_uuid: grp.uuid, group_class: "project")
+    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+    prj = Group.create!(owner_uuid: system_user_uuid, group_class: "project")
+
+    l0 = Link.create!(tail_uuid: grp.uuid,
+                 head_uuid: prj.uuid,
+                 link_class: 'permission',
+                 name: 'can_manage')
+
     assert_empty Group.readable_by(users(:active)).where(uuid: prj.uuid)
     assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
 
index 593d945cff0be54e46cb360712efd20b28e1658d..b2816ac16f4c1893d279c72c86e61f05f1cc1740 100644 (file)
@@ -128,7 +128,7 @@ class FuseMagicTest(MountTestBase):
         super(FuseMagicTest, self).setUp(api=api)
 
         self.test_project = run_test_server.fixture('groups')['aproject']['uuid']
-        self.non_project_group = run_test_server.fixture('groups')['public']['uuid']
+        self.non_project_group = run_test_server.fixture('groups')['public_role']['uuid']
         self.collection_in_test_project = run_test_server.fixture('collections')['foo_collection_in_aproject']['name']
 
         cw = arvados.CollectionWriter()
index 9e2307b7a6751c66b6b990634bece50af0a6d366..4d03ba89e327aa7db1bd9f08808e15d3f0487c9f 100644 (file)
@@ -226,8 +226,9 @@ func SetParentGroup(cfg *ConfigParams) error {
                                log.Println("Default parent group not found, creating...")
                        }
                        groupData := map[string]string{
-                               "name":       cfg.ParentGroupName,
-                               "owner_uuid": cfg.SysUserUUID,
+                               "name":        cfg.ParentGroupName,
+                               "owner_uuid":  cfg.SysUserUUID,
+                               "group_class": "role",
                        }
                        if err := CreateGroup(cfg, &parentGroup, groupData); err != nil {
                                return fmt.Errorf("error creating system user owned group named %q: %s", groupData["name"], err)
@@ -528,17 +529,21 @@ func GetRemoteGroups(cfg *ConfigParams, allUsers map[string]arvados.User) (remot
 
        params := arvados.ResourceListParams{
                Filters: []arvados.Filter{{
-                       Attr:     "owner_uuid",
+                       Attr:     "tail_uuid",
                        Operator: "=",
                        Operand:  cfg.ParentGroupUUID,
                }},
        }
-       results, err := GetAll(cfg.Client, "groups", params, &GroupList{})
+       results, err := GetAll(cfg.Client, "links", params, &LinkList{})
        if err != nil {
                return remoteGroups, groupNameToUUID, fmt.Errorf("error getting remote groups: %s", err)
        }
        for _, item := range results {
-               group := item.(arvados.Group)
+               var group arvados.Group
+               err = GetGroup(cfg, &group, item.(arvados.Link).HeadUUID)
+               if err != nil {
+                       return remoteGroups, groupNameToUUID, fmt.Errorf("error getting remote group: %s", err)
+               }
                // Group -> User filter
                g2uFilter := arvados.ResourceListParams{
                        Filters: []arvados.Filter{{
index 9eec6b6d97aa3d1305f55f8d02e8d551e37ddc07..2da8c1cdde4bb2cf131e9afcd520eec7f4e5ed47 100644 (file)
@@ -170,7 +170,7 @@ func RemoteGroupExists(cfg *ConfigParams, groupName string) (uuid string, err er
                }, {
                        Attr:     "owner_uuid",
                        Operator: "=",
-                       Operand:  cfg.ParentGroupUUID,
+                       Operand:  cfg.SysUserUUID,
                }, {
                        Attr:     "group_class",
                        Operator: "=",