Arvados

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.

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.

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

• 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

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

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.

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

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.