[API] Unprivileged user can create subprojects under projects they do not own (but cannot modify the subproject once created)
#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.
#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_uuidonly if current_user has
:managepermission on the object and:
- the new
owner_uuidis a group, and
:writepermission for that group, or
- the new
#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.
#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.
- 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.
#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)