Bug #3214

[API] Unprivileged user can create subprojects under projects they do not own (but cannot modify the subproject once created)

Added by Peter Amstutz almost 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
07/18/2014
Due date:
% Done:

100%

Estimated time:
(Total: 1.00 h)
Story points:
1.0

Subtasks

Task #3290: Review 3214-permission-to-use-owner-uuidResolvedTom Clegg

Associated revisions

Revision 230ac956
Added by Tom Clegg almost 6 years ago

Merge branch '3214-permission-to-use-owner-uuid'
closes #3214

History

#1 Updated by Brett Smith almost 6 years ago

  • Status changed from New to In Progress
  • Assigned To set to Brett Smith

#2 Updated by Brett Smith almost 6 years ago

Right now this happens because users are generally allowed to assign ownership of their own objects to someone else, and that's effectively the case that ensure_owner_uuid_is_permitted sees. I have a patch to special-case groups on the API server end. I'd also like to extend it to avoid presenting this option on Workbench.

#3 Updated by Brett Smith almost 6 years ago

Thinking about this more, maybe this needs to be a pure Workbench fix? It definitely needs a Workbench component; it may not make as much sense to special-case this on the API server end.

#4 Updated by Tom Clegg almost 6 years ago

Workbench should hide the "create subproject" button if the current project is not writable by current_user.

Also, API server should permit changing/setting an object's owner_uuid only if current_user has :manage permission on the object and:
  • the new owner_uuid is a group, and current_user has :write permission for that group, or
  • the new owner_uuid is current_user.uuid

#5 Updated by Ward Vandewege almost 6 years ago

  • Assigned To deleted (Brett Smith)
  • Target version changed from 2014-07-16 Sprint to 2014-08-06 Sprint

#6 Updated by Tom Clegg almost 6 years ago

  • Story points set to 1.0

#7 Updated by Tom Clegg almost 6 years ago

  • Assigned To set to Tom Clegg

#8 Updated by Tom Clegg almost 6 years ago

The "create subproject" button was already hidden, but the button in the top nav dropdown labelled "New project" was really a "create subproject of current project" button. Now it creates a top-level project (owner_uuid == current_user.uuid) instead.

Also fixed the permission rules on the API server so you can't set owner_uuid to a project (or group or user) for which you don't have write permission.

At f38d011

#9 Updated by Brett Smith almost 6 years ago

Reviewing f38d011. My comments are all on subjective points, so as far as I'm concerned you're welcome to merge without addressing them.

In ensure_owner_uuid_is_permitted:

  • I think it might help improve the comments' readability to avoid talking about the "existing" owner, in preference to the "old" and "new" owner. Since either one could be the "existing" owner depending on whether you're considering the database record (old) or self object (new).
  • I also think it might improve readability in the old owner check to merge the "unless" condition into the preceding "if". (Whenever I see a conditional that only contains another conditional, it feels like when I almost sneeze but don't. My brain is geared up for something to happen and it's unsettling when it doesn't.)

In the associated tests: we've gotten good about following the style that functional tests should only make one request to the controller, and personally I've started feeling like it's nice for unit tests to similarly be as small as possible. If nothing else, it can help narrow down where the source of the error is. The very specific assertion messages you provide go a long way to mitigate that (which is great, thanks!), but separate tests better cover situations like code erroring out outside assertions.

Thanks.

#10 Updated by Tom Clegg almost 6 years ago

I agree on all counts. Addressed in 3c2d730 and 68aa07e. Better now?

#11 Updated by Brett Smith almost 6 years ago

Tom Clegg wrote:

I agree on all counts. Addressed in 3c2d730 and 68aa07e. Better now?

Yes, 68aa07e looks good to merge to me. Thank you.

#12 Updated by Anonymous almost 6 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:230ac956317ec9389252240de934edc168098a76.

#13 Updated by Tom Clegg almost 6 years ago

  • Subject changed from Unprivileged user can create subprojects under projects they do not own (but cannot modify the subproject once created) to [API] Unprivileged user can create subprojects under projects they do not own (but cannot modify the subproject once created)

Also available in: Atom PDF