Project

General

Profile

Actions

Feature #18691

closed

Frozen project support

Added by Peter Amstutz about 1 year ago. Updated 10 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
03/08/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

In the API server, implement the design described in #18390

Write tests


Subtasks 1 (0 open1 closed)

Task #18782: Review 18691-freeze-projectResolvedPeter Amstutz03/08/2022

Actions

Related issues

Related to Arvados Epics - Story #18390: Frozen projectsResolved03/01/202207/31/2022

Actions
Related to Arvados - Feature #18645: Design for search in dialogue window to select ProjectResolvedPeter Amstutz02/09/2022

Actions
Related to Arvados - Bug #19145: Frozen project still writableResolvedTom Clegg06/06/2022

Actions
Blocks Arvados - Feature #18692: Frozen projects workbench supportResolvedDaniel Kutyła05/19/2022

Actions
Actions #1

Updated by Peter Amstutz about 1 year ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz about 1 year ago

Actions #3

Updated by Peter Amstutz about 1 year ago

Actions #5

Updated by Peter Amstutz about 1 year ago

  • Related to Feature #18645: Design for search in dialogue window to select Project added
Actions #6

Updated by Peter Amstutz 12 months ago

  • Target version changed from 2022-02-16 sprint to 2022-03-02 sprint
Actions #7

Updated by Peter Amstutz 12 months ago

  • Assigned To set to Tom Clegg
Actions #8

Updated by Tom Clegg 11 months ago

  • Status changed from New to In Progress
Actions #9

Updated by Peter Amstutz 11 months ago

  • Target version changed from 2022-03-02 sprint to 2022-03-16 sprint
Actions #10

Updated by Tom Clegg 11 months ago

18691-freeze-project @ 3d5ac704cca086a5ce66e6724f7087ff487abe3c --

still todo:
  • 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.
Actions #11

Updated by Tom Clegg 11 months ago

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)

Actions #12

Updated by Peter Amstutz 11 months ago

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.

Actions #13

Updated by Tom Clegg 11 months ago

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
Actions #14

Updated by Peter Amstutz 11 months ago

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?

Actions #15

Updated by Peter Amstutz 11 months ago

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})"

Actions #16

Updated by Peter Amstutz 11 months ago

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"

Actions #17

Updated by Peter Amstutz 11 months ago

    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)

Actions #18

Updated by Peter Amstutz 11 months ago

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

Actions #19

Updated by Peter Amstutz 11 months ago

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.

Actions #20

Updated by Peter Amstutz 11 months ago

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?

Actions #21

Updated by Tom Clegg 11 months ago

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

Actions #22

Updated by Peter Amstutz 11 months ago

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

Actions #23

Updated by Peter Amstutz 11 months ago

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?

Actions #24

Updated by Peter Amstutz 11 months ago

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

Shouldn't unfreeze require manage level?

Actions #25

Updated by Tom Clegg 11 months ago

  • Target version changed from 2022-03-16 sprint to 2022-03-30 Sprint
Actions #26

Updated by Peter Amstutz 11 months ago

  • 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
Actions #27

Updated by Peter Amstutz 11 months ago

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?)

Actions #28

Updated by Peter Amstutz 11 months ago

      # 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.

Actions #29

Updated by Tom Clegg 11 months ago

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

Actions #30

Updated by Peter Amstutz 11 months ago

18691-freeze-project @ 7b070fc8458f4108d44d6bfb939e36d3cc76af84

This LGTM, thanks!

Actions #31

Updated by Tom Clegg 10 months ago

  • Status changed from In Progress to Resolved

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.

We decided to set the dry-run feature aside for now, and revisit if/when needed.

Actions #32

Updated by Peter Amstutz 10 months ago

  • Release set to 46
Actions #33

Updated by Tom Clegg 8 months ago

  • Related to Bug #19145: Frozen project still writable added
Actions

Also available in: Atom PDF