- In ProjectQuery, always load the viewer's membership in the project because we need it to perform a CAN_VIEW test.
- Add storage for the view, edit and join policies.
- A user can always view a project if they are a member.
- A user can always join a project if they can edit it.
- Editing a project requires both "view" and "edit" permissions, and edit does not imply view.
- This has no effect on the application yet.
Details
Details
- Reviewers
vrana btrahan - Maniphest Tasks
- T603: Support permissions/policies in all Phabricator applications
- Commits
- Restricted Diffusion Commit
rPbd0be1c650ea: Add View, Edit and Join policies to PhabricatorProject
See next diff.
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
I found an inline thing on CAN_EDIT that might be a problem or I am confused about.
I was thinking about whether the join to get if the viewer is a membership is better accomplished by something like
if ($need_members) {
// grab 'em all
} else {
// just grab viewers membership data
}
// do that loop to set if the viewer is a memberThis feels more simple to me but is an extra query always. I also think the join optimization is probably important as projects are going to be used everywhere. So I'm babbling.
| src/applications/project/storage/PhabricatorProject.php | ||
|---|---|---|
| 65 | is this case intentionally blank? I think the can join case ends up calling this recursively too so probably an error here. | |
| 84 | maybes some idx with false as the default action? | |
Comment Actions
I think the extra join is nearly free since it can use an entire primary key and is 1:1 and we'll be hitting that table anyway in the "needMembers" case, but let me add a bunch of data to my local and make sure that's actually true.
| src/applications/project/storage/PhabricatorProject.php | ||
|---|---|---|
| 65 | This is intentionally blank, yeah. It just means that no users have automatic edit capabilities as a result of special relationships to the object. Broadly, you can edit/view an object if you have an automatic capability or a normal capability. For example, a revision's owner and reviewers will always be able to view it, even if the general policy is "no one" or otherwise excludes them, since it would be silly and counterintuitive to own a revision you couldn't view. Likewise, if you're a member of a project, we always let you view it, which is why the VIEW branch has a special check for membership. In the JOIN case, we always let you join a project if you can edit it, since you could just go to the "edit members" and add yourself anyway, and making edit imply join makes the UI simpler and more consistent. In the EDIT case, no one gets automatic capabilities, since projects don't have anything like an owner (we keep track of the original project creator, but they might later be removed from the project, so I don't think they should get any special privileges). At runtime, we'll fail to get an automatic capability and fall through to the normal check (i.e., whatever the "Who can edit this?" dropdown is set to). | |
| 84 | It's guaranteed to exist since we throw on line 81 if it doesn't (basically, default is throw rather than false, since you did something wrong). | |
Comment Actions
I added ~30k projects and ~400k memberships and the join does have a small-but-measurable cost (3-4ms on 7-8ms without it); I'll add the needmembers condition.