Arvados

18691-freeze-project @ 3d5ac704cca086a5ce66e6724f7087ff487abe3c --

• Consider moving configs from "API" section to "Collections" section ("API" section is mostly about HTTP behavior, and "Collections" has other project things like forward slash behavior)
• Cannot trash a project that contains frozen objects (attempt to set “trash_at” will return an error)
• Trashed items in a frozen project are not returned with "include_trash" (should be explicitly filtered out from the call), from the user's perspective, trashed items in a frozen project are immediately gone, unless the project is un-frozen.

18691-freeze-project @ 67a86e26e3cc33af9ffe65d486137077b99a6944 --

The dry-run / no-op feature does not exist yet. I'm thinking this is already enough of a branch to review & merge, then tackle the dry-run flag.

Still want to consider moving configs from "API" section to "Collections" section ("API" section is mostly about HTTP behavior, and "Collections" has other project things like forward slash behavior)

18691-freeze-project @ 67a86e26e3cc33af9ffe65d486137077b99a6944 --

A user with manage permission can set the frozen_by_uuid attribute of a project group to their own user UUID. Once this is done, no further changes can be made to the project or its contents, including subprojects.

Probably worth elaborating a bit more, e.g. collections cannot be changed, objects cannot be trashed, things cannot be moved in or out, not even the admin can change things, etc.

     return [] if respond_to?(:frozen_by_uuid) && frozen_by_uuid &&
                 !(Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
                     current_user.andand.is_admin :
                     current_user.can?(manage: uuid))

Really small point but I found this logic hard to read, distributing the negation would make the intention much clearer:

                 (Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
                     !current_user.andand.is_admin :
                     !current_user.can?(manage: uuid))

-      trashed_check = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
-                      "where trash_at <= statement_timestamp()) #{exclude_trashed_records}" 

+      excluded_trash = "(#{sql_table}.owner_uuid IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
+                       "WHERE trash_at <= statement_timestamp()))" 
+      sql_conds = "NOT #{excluded_trash}" 

In theory, these are the same, in practice, this could blow up the query plan. (The readable_by query has been carefully tuned with workarounds to accommodate Postgres 9 query planner behavior).

We should have a plan to proactively compare "explain analyze" for the old and new versions of the query.

   def permission_to_create
-    current_user.andand.is_active
+    if !current_user.andand.is_active
+      return false
+    end
+    if self.respond_to?(:owner_uuid) && FrozenGroup.where(uuid: owner_uuid).any?
+      errors.add :owner_uuid, "#{owner_uuid} is frozen" 
+      return false
+    end
+    return true
   end

Why is this here instead of ensure_owner_uuid_is_permitted?

There should be a frozen project check in User#can? (which is used by ensure_owner_uuid_is_permitted among other places). This may require a special case in ensure_owner_uuid_is_permitted but seems cleaner overall (makes the permission check more centralized).

I believe the validation hooks run before the before_save hooks, so the better error messages offered by "check_frozen_state_change_allowed" will still show up.

It's getting late so I'm going to cut the review off here, I haven't quite gone through everything, I'll take another look next week.

FWIW: ran 'explain analyze' on the old & new readable_by queries on ce8i5 (postgresql 10).

With include_trash=false, results are similar:

# main
# explain analyze SELECT "collections".* FROM "collections" WHERE ((collections.owner_uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1 AND traverse_owned)   OR collections.uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1) ) AND collections.owner_uuid NOT IN (SELECT group_uuid FROM trashed_groups where trash_at <= statement_timestamp()) AND (collections.trash_at is NULL or collections.trash_at > statement_timestamp()) AND collections.uuid = collections.current_version_uuid)

 Seq Scan on collections  (cost=16.52..115184.67 rows=1558 width=809) (actual time=522.848..522.851 rows=0 loops=1)
   Filter: ((NOT (hashed SubPlan 3)) AND ((uuid)::text = (current_version_uuid)::text) AND ((hashed SubPlan 1) OR (hashed SubPlan 2)) AND ((trash_at IS NULL) OR (trash_at > statement_timestamp())))
   Rows Removed by Filter: 847539
   SubPlan 3
     ->  Seq Scan on trashed_groups  (cost=0.00..1.10 rows=7 width=28) (actual time=0.007..0.008 rows=0 loops=1)
           Filter: (trash_at <= statement_timestamp())
   SubPlan 1
     ->  Index Scan using permission_user_target on materialized_permissions  (cost=0.42..7.69 rows=1 width=28) (actual time=0.020..0.021 rows=0 loops=1)
           Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
           Filter: (traverse_owned AND (perm_level >= 1))
   SubPlan 2
     ->  Index Scan using permission_user_target on materialized_permissions materialized_permissions_1  (cost=0.42..7.69 rows=1 width=28) (actual time=0.008..0.008 rows=0 loops=1)
           Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
           Filter: (perm_level >= 1)
 Planning time: 0.335 ms
 Execution time: 522.915 ms

# 67a86e26e3cc33af9ffe65d486137077b99a6944
# explain analyze SELECT "collections".* FROM "collections" WHERE ((collections.owner_uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1 AND traverse_owned)   OR collections.uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1) ) AND NOT ((collections.owner_uuid IN (SELECT group_uuid FROM trashed_groups WHERE trash_at <= statement_timestamp())) OR collections.trash_at <= statement_timestamp() IS TRUE) AND collections.uuid = collections.current_version_uuid)

 Seq Scan on collections  (cost=16.52..115184.67 rows=1558 width=809) (actual time=493.009..493.011 rows=0 loops=1)
   Filter: ((NOT (hashed SubPlan 3)) AND ((uuid)::text = (current_version_uuid)::text) AND ((hashed SubPlan 1) OR (hashed SubPlan 2)) AND ((trash_at <= statement_timestamp()) IS NOT TRUE))
   Rows Removed by Filter: 847539
   SubPlan 3
     ->  Seq Scan on trashed_groups  (cost=0.00..1.10 rows=7 width=28) (actual time=0.006..0.006 rows=0 loops=1)
           Filter: (trash_at <= statement_timestamp())
   SubPlan 1
     ->  Index Scan using permission_user_target on materialized_permissions  (cost=0.42..7.69 rows=1 width=28) (actual time=0.021..0.021 rows=0 loops=1)
           Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
           Filter: (traverse_owned AND (perm_level >= 1))
   SubPlan 2
     ->  Index Scan using permission_user_target on materialized_permissions materialized_permissions_1  (cost=0.42..7.69 rows=1 width=28) (actual time=0.009..0.010 rows=0 loops=1)
           Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
           Filter: (perm_level >= 1)
 Planning time: 0.336 ms
 Execution time: 493.091 ms

With include_trash=true, the old version uses a different plan because it doesn't need to check the trashed_groups table, but the new version still needs to exclude trash in frozen projects:

# main
# explain analyze SELECT "collections".* FROM "collections" WHERE ((collections.owner_uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1 AND traverse_owned)   OR collections.uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1) )   AND collections.uuid = collections.current_version_uuid)

 Gather  (cost=1015.39..101241.08 rows=3153 width=809) (actual time=511.073..515.882 rows=0 loops=1)
   Workers Planned: 2
   Workers Launched: 2
   ->  Parallel Seq Scan on collections  (cost=15.39..99925.78 rows=1314 width=809) (actual time=500.745..500.746 rows=0 loops=3)
         Filter: (((uuid)::text = (current_version_uuid)::text) AND ((hashed SubPlan 1) OR (hashed SubPlan 2)))
         Rows Removed by Filter: 280341
         SubPlan 1
           ->  Index Scan using permission_user_target on materialized_permissions  (cost=0.42..7.69 rows=1 width=28) (actual time=0.046..0.046 rows=0 loops=3)
                 Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
                 Filter: (traverse_owned AND (perm_level >= 1))
         SubPlan 2
           ->  Index Scan using permission_user_target on materialized_permissions materialized_permissions_1  (cost=0.42..7.69 rows=1 width=28) (actual time=0.016..0.016 rows=0 loops=3)
                 Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
                 Filter: (perm_level >= 1)
 Planning time: 0.312 ms
 Execution time: 515.941 ms

# 67a86e26e3cc33af9ffe65d486137077b99a6944
# explain analyze SELECT "collections".* FROM "collections" WHERE ((collections.owner_uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1 AND traverse_owned)   OR collections.uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1) ) AND NOT (((collections.owner_uuid IN (SELECT group_uuid FROM trashed_groups WHERE trash_at <= statement_timestamp())) OR collections.trash_at <= statement_timestamp() IS TRUE) AND collections.owner_uuid IN (SELECT uuid FROM frozen_groups)) AND collections.uuid = collections.current_version_uuid)

 Seq Scan on collections  (cost=28.39..116931.48 rows=2353 width=809) (actual time=454.499..454.501 rows=0 loops=1)
   Filter: (((uuid)::text = (current_version_uuid)::text) AND ((hashed SubPlan 1) OR (hashed SubPlan 2)) AND (((NOT (hashed SubPlan 3)) AND ((trash_at <= statement_timestamp()) IS NOT TRUE)) OR (NOT (hashed SubPlan 4))))
   Rows Removed by Filter: 841024
   SubPlan 1
     ->  Index Scan using permission_user_target on materialized_permissions  (cost=0.42..7.69 rows=1 width=28) (actual time=0.032..0.032 rows=0 loops=1)
           Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
           Filter: (traverse_owned AND (perm_level >= 1))
   SubPlan 2
     ->  Index Scan using permission_user_target on materialized_permissions materialized_permissions_1  (cost=0.42..7.69 rows=1 width=28) (actual time=0.006..0.007 rows=0 loops=1)
           Index Cond: ((user_uuid)::text = 'zzzzz-tpzed-xurymjxw79nv3jz'::text)
           Filter: (perm_level >= 1)
   SubPlan 3
     ->  Seq Scan on trashed_groups  (cost=0.00..1.10 rows=7 width=28) (never executed)
           Filter: (trash_at <= statement_timestamp())
   SubPlan 4
     ->  Seq Scan on frozen_groups  (cost=0.00..11.50 rows=150 width=516) (never executed)
 Planning time: 0.504 ms
 Execution time: 454.589 ms

With include_trash=false, results are similar:

67a86e26e3cc33af9ffe65d486137077b99a6944

1. explain analyze SELECT "collections".* FROM "collections" WHERE ((collections.owner_uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1 AND traverse_owned) OR collections.uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1) ) AND NOT (((collections.owner_uuid IN (SELECT group_uuid FROM trashed_groups WHERE trash_at <= statement_timestamp())) OR collections.trash_at <= statement_timestamp() IS TRUE) AND collections.owner_uuid IN (SELECT uuid FROM frozen_groups)) AND collections.uuid = collections.current_version_uuid)

I think you must have made a copy and paste error, this query doesn't contain "frozen_groups" anywhere, so why would the analysis include a subquery on frozen_groups?

sql_conds = "NOT #{excluded_trash}"

Small point but this feels very mistake-prone, can we put extra parenthesis around the substitution?

sql_conds = "NOT (#{excluded_trash})"

Also I'd appreciate a comment like

1. "excluded_trash" is the set of things which are trashed, that should be excluded from the result

and/or rename "excluded_trash" to "trash_to_exclude"

    if (admin && include_trash) || sql_table == "api_client_authorizations" 
      excluded_trash = "false" 
    else
      excluded_trash = "(#{sql_table}.owner_uuid IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
                       "WHERE trash_at <= statement_timestamp()))" 
      if sql_table == "groups" || sql_table == "collections" 
        excluded_trash = "(#{excluded_trash} OR #{sql_table}.trash_at <= statement_timestamp() IS TRUE)" 
      end

      if include_trash
        # Exclude trash inside frozen projects
        excluded_trash = "(#{excluded_trash} AND #{sql_table}.owner_uuid IN (SELECT uuid FROM #{FROZEN_GROUPS}))" 
      end
    end

I'm having some trouble with this logic.

admin && include_trash -- user is admin, and wants to list everything. In this case, we should still not list trash owned by frozen groups, but this will. Is this wrong?

!admin && include trash -- regular user, wants to list everything. In this case, we list trash unless it has the additional constraint that it is owned by a frozen group. I think this is correct.

(admin || !admin) && !include_trash -- any kind of user, do not list trash. In this case, we do not list trash following the regular rules. frozen table is irrelevant.

For the first (admin && include_trash) case, perhaps this is needed to have keep-balance behave correctly? In that case I suppose it does make sense, but there should be a comment to the effect that admins will have a different view of trashed items in a frozen project than regular users, and that's intentional.

(There's a reason most of that method has more lines of comments explaining the SQL than actual SQL)

So I would expect

admin && include_trash -- doesn't check anything

!admin && include_trash -- checks both the trash and frozen_project tables

(admin || !admin) && !include_trash -- checks trash, but not frozen_project table

On the topic of checking for frozen projects in "User#can?" -- that method isn't used in a huge number of places but for example the without additional tweaks the "UsersController#merge" method would incorrectly allow you to move objects into a frozen project.

if trash_at || delete_at || !new_record? && TrashedGroup.where(group_uuid: uuid).any?

I assume this means:

if trash_at || delete_at || (!new_record? && TrashedGroup.where(group_uuid: uuid).any?)

Presumably because "uuid" is null if new_record.

Creating a project which is immediately frozen seems weird, we should certainly make sure it doesn't crash, but I'd also be fine with disallowing that case entirely (but maybe there's no reason not to).

    frozen_descendants = ActiveRecord::Base.connection.exec_query(
      "select uuid from frozen_groups, #{temptable} where uuid = target_uuid",
      "Group.update_trash.check_frozen")
    if frozen_descendants.any?
      raise ArgumentError.new("cannot trash project containing frozen project #{frozen_descendants[0]["uuid"]}")
    end

update_trash is an after_update hook, should double check if the transaction has been committed at that point? perhaps this check needs to happen earlier?

Peter Amstutz wrote:

A user with manage permission can set the frozen_by_uuid attribute of a project group to their own user UUID. Once this is done, no further changes can be made to the project or its contents, including subprojects.

Probably worth elaborating a bit more, e.g. collections cannot be changed, objects cannot be trashed, things cannot be moved in or out, not even the admin can change things, etc.

Added "...even by admins" and more examples of what "no further changes" means.

Really small point but I found this logic hard to read, distributing the negation would make the intention much clearer:

Done

Why is this here instead of ensure_owner_uuid_is_permitted?

There should be a frozen project check in User#can? (which is used by ensure_owner_uuid_is_permitted among other places). This may require a special case in ensure_owner_uuid_is_permitted but seems cleaner overall (makes the permission check more centralized).

Moved the frozen check to User#can?, which means ensure_owner_uuid_is_permitted does check it now.

1. explain analyze SELECT "collections".* FROM "collections" WHERE ((collections.owner_uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1 AND traverse_owned) OR collections.uuid IN (SELECT target_uuid FROM materialized_permissions WHERE user_uuid IN ('zzzzz-tpzed-xurymjxw79nv3jz') AND perm_level >= 1) ) AND NOT (((collections.owner_uuid IN (SELECT group_uuid FROM trashed_groups WHERE trash_at <= statement_timestamp())) OR collections.trash_at <= statement_timestamp() IS TRUE) AND collections.owner_uuid IN (SELECT uuid FROM frozen_groups)) AND collections.uuid = collections.current_version_uuid)

I think you must have made a copy and paste error, this query doesn't contain "frozen_groups" anywhere, so why would the analysis include a subquery on frozen_groups?

Yes, that was the wrong query. Fixed / edited note above. Similarity is more evident now.

sql_conds = "NOT #{excluded_trash}"

Small point but this feels very mistake-prone, can we put extra parenthesis around the substitution?

sql_conds = "NOT (#{excluded_trash})"

Done. (I had already ensured excluded_trash always had parentheses unless it was just "false", but extra ones don't hurt.)

Also I'd appreciate a comment like

1. "excluded_trash" is the set of things which are trashed, that should be excluded from the result

and/or rename "excluded_trash" to "trash_to_exclude"

Added comment with pseudocode.

if trash_at || delete_at || !new_record? && TrashedGroup.where(group_uuid: uuid).any?

I assume this means:

if trash_at || delete_at || (!new_record? && TrashedGroup.where(group_uuid: uuid).any?)

Yes.

Presumably because "uuid" is null if new_record.

Yes.

Creating a project which is immediately frozen seems weird, we should certainly make sure it doesn't crash, but I'd also be fine with disallowing that case entirely (but maybe there's no reason not to).

Not sure what anyone is going to use it for, but don't see any reason to disallow it either.

update_trash is an after_update hook, should double check if the transaction has been committed at that point? perhaps this check needs to happen earlier?

Transaction is not committed at that point. The outside-transaction callbacks are after_commit/after_rollback. https://guides.rubyonrails.org/active_record_callbacks.html

18691-freeze-project @ c3d04aeb81a04b5dc527af8f9297e9fefb5f4851 -- developer-run-tests: #2964

edge case: should not be able to freeze a project which has non-final container requests.

If keep-balance is reading from Postgres directly, do we actually need the API to return trashed items inside frozen projects for admins?

unless ((respond_to?(:frozen_by_uuid) && frozen_by_uuid_in_database && !frozen_by_uuid) ?

I've never seen this "_in_database" field before, we ususally use _was (i.e. frozen_by_uuid_was) to get the starting value, is there a difference?

VAL_FOR_PERM = {:read => 1,
:write => 2,
:unfreeze => 2,
:manage => 3}

Shouldn't unfreeze require manage level?

• Add test checking that user with only can_write can't unfreeze
• Add test that a project with a non-final container request, cannot be frozen

if trash_at || delete_at || !new_record? && TrashedGroup.where(group_uuid: uuid).any?

Parens around the last clause would be clearer

if trash_at || delete_at || (!new_record? && TrashedGroup.where(group_uuid: uuid).any?)

      # itself. (If we're in the act of unfreezing, we only need
      # :unfreeze permission, which means "what write permission would
      # be if target weren't frozen")

I'm a little confused by this comment. If it requires manage permission to freeze, it should definitely require manage permission to unfreeze?

I can't remember if this was stated explicitly in the design doc, but it seems that frozen projects also cannot be moved from their parent projects (but the parent project itself can be moved). That seems fine but should probably be stated somewhere.

Peter Amstutz wrote:

edge case: should not be able to freeze a project which has non-final container requests.

Added. I don't think "Uncommitted" state causes problems so we still allow that too, just not Committed -- wdyt?

If keep-balance is reading from Postgres directly, do we actually need the API to return trashed items inside frozen projects for admins?

That does mean keep-balance would work without it, but I think it makes sense anyway. It allows admins to search/inspect/recover trashed records without the undesirable side effects of temporarily un-freezing the project. IIRC the rationale for hiding trash is to preserve the "frozen" effect so even trash visibility doesn't change over time, which doesn't seem (to me) necessary/desirable for admins. Does that make sense?

I've never seen this "_in_database" field before, we ususally use _was (i.e. frozen_by_uuid_was) to get the starting value, is there a difference?

They made a new clearer set of "dirty" methods and deprecated the old ones in circumstances where the behavior changed in 5.2. As a result people find the deprecation situation confusing ... maybe better to change back to _was since this isn't an after_ callback where the behavior changed, so I did that.

VAL_FOR_PERM = {:read => 1,
:write => 2,
:unfreeze => 2,
:manage => 3}

Shouldn't unfreeze require manage level?

It doesn't really make a difference here, because the Group hook checks for manage permission when frozen_by_uuid is changing in either direction. Changed the VAL_FOR_PERM to 3 anyway though, to make it less confusing.

• Add test checking that user with only can_write can't unfreeze

That test is here:

      # User with write permission (but not manage) cannot unfreeze                                           
      act_as_user users(:spectator) do
        # First confirm we have write permission on the parent project                                        
        assert Collection.create(name: 'bar', owner_uuid: parent.uuid)
        assert_raises(ArvadosModel::PermissionDeniedError) do
          proj.update_attributes!(frozen_by_uuid: nil)
        end
      end

• Add test that a project with a non-final container request, cannot be frozen

Added.

Parens around the last clause would be clearer

if trash_at || delete_at || (!new_record? && TrashedGroup.where(group_uuid: uuid).any?)

Added.

I'm a little confused by this comment. If it requires manage permission to freeze, it should definitely require manage permission to unfreeze?

Reworded comment. (Yes, it does require manage permission to unfreeze, but the caller was only asking whether the user has write permission, knowing the Group callback will also check manage permission.)

        # "unfreeze" permission means "can write, but only if                                                                  
        # explicitly un-freezing at the same time" (see                                                                        
        # ArvadosModel#ensure_owner_uuid_is_permitted). If the                                                                 
        # permission query above passed the permission level of                                                                
        # :unfreeze (which is the same as :manage), and the parent                                                             
        # isn't also frozen, then un-freeze is allowed.                                                                        

I can't remember if this was stated explicitly in the design doc, but it seems that frozen projects also cannot be moved from their parent projects (but the parent project itself can be moved). That seems fine but should probably be stated somewhere.

Added "moved" to the list of things that count as changes on the API doc page.

18691-freeze-project @ 7b070fc8458f4108d44d6bfb939e36d3cc76af84 -- developer-run-tests: #2969

wb1 retry developer-run-tests-apps-workbench-integration: #3168

18691-freeze-project @ 7b070fc8458f4108d44d6bfb939e36d3cc76af84

This LGTM, thanks!