Page MenuHomePhabricator

Provide core policy support for Spaces
ClosedPublic

Authored by epriestley on Jun 4 2015, 8:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 13, 3:42 AM
Unknown Object (File)
Sat, Dec 7, 5:01 PM
Unknown Object (File)
Fri, Dec 6, 7:55 PM
Unknown Object (File)
Tue, Dec 3, 6:15 PM
Unknown Object (File)
Sat, Nov 23, 10:04 PM
Unknown Object (File)
Nov 19 2024, 6:07 AM
Unknown Object (File)
Oct 30 2024, 6:42 AM
Unknown Object (File)
Sep 9 2024, 8:43 PM

Details

Summary

Ref T8424. No UI or interesting behavior yet, but integrates Spaces checks:

  • PolicyFilter now checks Spaces.
  • PolicyAwareQuery now automatically adds Spaces constraints.

There's one interesting design decision here: spaces are stronger than automatic capabilities. That means that you can't see a task in a space you don't have permission to access, even if you are the owner.

I think this is desirable. Particularly, we need to do this in order to exclude objects at the query level, which potentially makes policy filtering for spaces hugely more efficient. I also like Spaces being very strong, conceptually.

It's possible that we might want to change this; this would reduce our access to optimizations but might be a little friendlier or make more sense to users later on.

For now, at least, I'm pursuing the more aggressive line. If we stick with this, we probably need to make some additional UI affordances (e.g., show when an owner can't see a task).

This also means that you get a hard 404 instead of a policy exception when you try to access something in a space you can't see. I'd slightly prefer to show you a policy exception instead, but think this is generally a reasonable tradeoff to get the high-performance filtering at the Query layer.

Test Plan
  • Added and executed unit tests.
  • Put objects in spaces and viewed them with multiple users.
  • Made the default space visible/invisible, viewed objects.
  • Checked the services panel and saw spacePHID constraints.
  • Verified that this adds only one query to each page.

Diff Detail

Repository
rP Phabricator
Branch
spaces6
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 6548
Build 6570: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Provide core policy support for Spaces.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
epriestley added a subscriber: chad.

@chad, not sure if you're already on a plane, but you might have some UX feedback on the "spaces are stronger than automatic capabilities" choice too.

epriestley edited the test plan for this revision. (Show Details)
epriestley updated this object.
btrahan edited edge metadata.

I think spaces being stronger than automatic capabilities is the right call from a product perspective. Mainly, I think losing access to a space should really make you lose access to everything in the space.

This revision is now accepted and ready to land.Jun 4 2015, 10:04 PM

I'm fine seeing how it all plays out. If I filed a security related task, and it gets moved for disclosure reasons, I'd expect to not see it again. Though maybe some final notification that I'd no longer be able to access it? Would emails even get rejected?

If we stick with this rule, I think we'll end up adding some kind of option which lets you selectively weaken it for a few spaces, specifically for the Security use case. In all other use cases (consulting, private stuff, etc.) it seems like strong barriers are the right rule, but the barrier for Security is really a weaker one.

I haven't figure out what the UI for that should look like yet, which is the tricky part. It would approximately boil down to this:

          Name: Security
   View Policy: All Users
   Edit Policy: Members of Project "Security"
Access Control: Members of Project "Security"

Where "Access Control" is an optional policy that the Space requires you to pass between automatic capability checks and standard policy checks, and defaults to off.

However, "Access Control" is a terrible, unclear name, and it's not clear that it's basically a sub-policy of "View Policy" in the presentation above.

In that model, the complete chain of checks would be:

  • Can you see the Space the object is in? If no, you can't see the object.
  • Do you have an automatic capability on the object (e.g., are you the task owner)? If yes, you can see the object.
  • If the space has an Access Control policy: do you pass that policy? If no, you can't see the object.
  • Do you pass the policy on the object itself? If no, you can't see the object.
  • If the object has an Extended Policy: do you satisfy the extended policy? If no, you can't see the object.
src/applications/policy/filter/PhabricatorPolicyFilter.php
456–466

We can just swap the order here to make automatic capabilities stronger than spaces, but then we have to drop the automatic WHERE spacePHID IN (...) constraint in the query and potentially do a lot more filtering in-process, which might be slow.

I guess adding "Access Control" strengthens the space, but the "Security" space is a surprisingly weak space (highly visible) in that model.

About to leave here in 30, but I did get a chance to install Photoshop on the little Macbook, so will be able to fiddle with UI if you come up with the base case and need something.

Have a safe trip!

I think we're probably still at least a couple days away from the UI settling down, but it should sort of start existing / being clickable soon, at least.

How do you see the Security case playing out? My idea is that users don't put things in the space; the security guys do so the space is very private and not visible. I do like making people get a notification when they lose visibility, similarly to the task I can't find right now about getting a final notification when you are removed from subscribers

For Security, I'd expect to file the task (or Legal, or whatever) and someone higher up (security team) could see and move it to that space.

In WMF's use case, at least, they want the original reporter to still be able to see objects in the "Security" space. I think this is a broadly reasonable thing for Spaces to be able to do, even though I don't think it's a primary use case.

So I imagine it going like this:

  • You create a new task in the "Security" space.
  • You can still see it (because of automatic capabilities, likely because you're CC'd on it, after T4411).
  • You can't see anything else in the "Security" space (because of the "Access Policy" thing above, except we find some more comprehensible name for it).

I think it's important that users be able to file tasks directly into the "Security" space. Otherwise, you can write a Herald rule to email you any time someone files a task that includes, say, "XSS", and you'll get an email about vulnerabilities before they're even looked at.

Given all these constraints, I think while configuring a space it might be cleanest to have the user think about

  • who can view the space and put objects into the space?
    • might as well default to getMostOpenPolicy() here IMO
  • who can view (and comment on, etc) objects in the space?
  • who can edit objects in the space?
  • who can edit the space itself?

I think we could do either be a bit heavy-handed with text or have the latter policies include editable tokens like "users who can view the space", "users who have automatic view capabilities", "object view policies" (can't remove that one maybe), etc. I think we'll need to clean up the workflow for users who lose access at any point to the object so they know what's going on (e.g. the one last notification thing discussed a little already)

Otherwise, you can write a Herald rule to email you any time someone files a task that includes, say, "XSS", and you'll get an email about vulnerabilities before they're even looked at.

Kind of a tangent, but I think if folks have security concerns like that, they can't really rely on people filling out a task form correctly. Rather, I'd expect some sort of custom contact form or email address that they really can't mess up e.g. by picking the wrong space or no space at all. I do expect us to have contact form stuff eventually via Nuance.

We could also not support this use case in Spaces at all and do it through Nuance instead. Maybe that's just better? Simple hard-boundary spaces feel way better to me for all other use cases I'm aware of (contractors, customers, secret projects, skunkworks, internal governance, surprise parties, hiding things from your enemies).

Tentatively, I'm going to build out hard-boundary spaces until they work. It's possible that we'll hit other issues which force us to soften the boundaries, but if we don't it might be the right product call to build the Security workflow through Nuance. Then Security would be hard-boundary space and Nuance would be the interface into it.

This particularly feels good in the Security case where you may want to separate your public response ("we're looking at this now") from your private communication.

We'll need to figure out how this affects WMF's prioritization if we do that, but if it's better for us in the long term I think we should pursue that instead.

might as well default to getMostOpenPolicy() here IMO

Particularly, I think this is the wrong default for every space except "Security", which maybe strengthens the case against having any soft-boundary spaces at all.

Most people I've talked to just want to keep OPS separate from IT separate from ENG. stuff usually doesn't move between.

*who use this feature of JIRA

This revision was automatically updated to reflect the committed changes.

Most people I've talked to just want to keep OPS separate from IT separate from ENG. stuff usually doesn't move between.

FWIW When we evaluated Phabricator not doing JIRA-esque siloing between teams was a significant positive point. From my point of view the break down is more 'stuff people who write code do' (tasks) vs 'inbound things from some sort of other are are overwhelming due to their volume or because the people filing them mess forms up in stereotypical non-technical-people-who-rarely-use-the-tools ways' (tasks today, but a source of friction). Most teams just heads down & code, some 'ops-y' teams have both flows. It sounds like this is more or less the long term thinking for Nuance, so huzzah!