1. 29 Mar, 2020 1 commit
    • Daniel Stone's avatar
      LOCAL: Make 'All Users' space extremely magic · 281f22c2
      Daniel Stone authored
      
      
      Phriction's view policy is ancestral: in order to access /w/foo/bar/baz,
      you must be able to access /w/foo and /w/bar in addition to
      /w/foo/bar/baz itself.
      
      This is fine and makes life easy: by setting restrictive policies on
      top-level pages, we can lessen the risk of someone exposing information
      they shouldn't, by accidentally making
      /w/cold-fusion/secret-research/funding-meeting/2018-09-14 public, when
      the rest of the hierarchy is super locked down.
      
      Phriction also recently gained Spaces support, which is nice: rather
      than trying to lock down with groups and harmonise permissions, we can
      just move top-level wiki pages to a particular Space, and then we don't
      need to worry about groups.
      
      Our clients don't know Spaces even exist, which is great since it avoids
      us having to explain the two-tier permission model to them. The reason
      they don't know it exists is because if you can only see a single Space,
      then Phabricator hides the entire Spaces UI away from you. Great!
      
      Unfortunately one detail ruins everything: /w/ is a top-level page
      itself, it counts for permission checks, and it _must be in a Space_.
      So, there is no way to have wiki documents in mutually-invisible Spaces
      unless you also have a common Space, at which point the whole Spaces UI
      suddenly becomes very visible everywhere.
      
      In order to try to keep our wiki partitioned, but to not confuse our
      clients (and give them the chance to potentially expose confidential
      information!), we:
        - have a magic 'Visible to Everyone' space
        - actually hide that space from everyone with policies
        - hack policy filters to make this space visible to everyone _only
          for the purpose of checking policies on wiki objects_
        - only allow admins to change view/edit policies on the root wiki
          page (see comment for reason why)
      
      This actual patch can obviously never go anywhere near upstream, but on
      the other hand we should probably make them aware of the problem and see
      if they're interested in discussing a solution, which is probably just
      to bless the root page with magic semantics.
      Signed-off-by: Daniel Stone's avatarDaniel Stone <daniels@collabora.com>
      281f22c2
  2. 12 Sep, 2019 2 commits
    • epriestley's avatar
      When users encounter a policy exception for a non-view capability with a... · 4f845d8f
      epriestley authored
      When users encounter a policy exception for a non-view capability with a custom policy, inline the policy rules
      
      Summary:
      Fixes T13411. This looks like the last case where you hit a policy explanation and have permission to see the policy, but we don't currently show you the policy rules.
      
      This implementation is slightly clumsy, but likely harmless.
      
      Test Plan: {F6856421}
      
      Maniphest Tasks: T13411
      
      Differential Revision: https://secure.phabricator.com/D20807
      4f845d8f
    • epriestley's avatar
      When users fail a "CAN_SEE" check, give them an "opaque" policy explanation · 0c7ea8c9
      epriestley authored
      Summary:
      Ref T13411. Currently, if you hit a policy exception because you can't view an object, we disclose details about the view policy of the object, particularly which project's members can see the object for project policies.
      
      Although there's a large amount of grey area here, this feels like a more substantial disclosure than we offer in other contexts. Instead, if you encounter a policy exception while testing "CAN_VIEW" or don't have "CAN_VIEW", present an "opaque" explanation which omits details that viewers who can't view the object shouldn't have access to. Today, this is the name of "Project" policies (and, implicitly, the rulesets of custom policies, which we now disclose in other similar contexts).
      
      Test Plan:
        - Hit policy exceptions for "CAN_VIEW" on an object with a project view policy, saw an opaque explanation.
        - Hit policy exceptions for "CAN_EDIT" on an object with a project edit policy and a view policy I satisfied, saw a more detailed explanation.
      
      Maniphest Tasks: T13411
      
      Differential Revision: https://secure.phabricator.com/D20806
      0c7ea8c9
  3. 28 May, 2019 1 commit
    • epriestley's avatar
      Test for "CAN_INTERACT" on comment edits in a way that survives objects which... · 53b9acfb
      epriestley authored
      Test for "CAN_INTERACT" on comment edits in a way that survives objects which only implement "CAN_VIEW"
      
      Summary:
      Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.
      
      Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.
      
      Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).
      
      Reviewers: amckinley
      
      Maniphest Tasks: T13289
      
      Differential Revision: https://secure.phabricator.com/D20558
      53b9acfb
  4. 09 Feb, 2019 1 commit
  5. 09 Oct, 2017 1 commit
    • Dmitri Iouchtchenko's avatar
      Fix spelling · 9bd6a370
      Dmitri Iouchtchenko authored
      Summary: Noticed a couple of typos in the docs, and then things got out of hand.
      
      Test Plan:
        - Stared at the words until my eyes watered and the letters began to swim on the screen.
        - Consulted a dictionary.
      
      Reviewers: #blessed_reviewers, epriestley
      
      Reviewed By: #blessed_reviewers, epriestley
      
      Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Differential Revision: https://secure.phabricator.com/D18693
      9bd6a370
  6. 03 Mar, 2017 1 commit
    • epriestley's avatar
      Allow task statuses to "lock" them, preventing additional comments and interactions · 0e7a5623
      epriestley authored
      Summary:
      Ref T12335. See that task for discussion. Here are the behavioral changes:
      
        - Statuses can be flagged with `locked`, which means that tasks in that status are locked to further discussion and interaction.
        - A new "CAN_INTERACT" permission facilitates this. For most objects, "CAN_INTERACT" is just the same as "CAN_VIEW".
        - For tasks, "CAN_INTERACT" is everyone if the status is a normal status, and no one if the status is a locked status.
        - If a user doesn't have "Interact" permission:
          - They can not submit the comment form.
          - The comment form is replaced with text indicating "This thing is locked.".
          - The "Edit" workflow prompts them.
      
      This is a mixture of advisory and hard policy checks but sholuld represent a reasonable starting point.
      
      Test Plan: Created a new "Locked" status, locked a task. Couldn't comment, saw lock warning, saw lock prompt on edit. Unlocked a task.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T12335
      
      Differential Revision: https://secure.phabricator.com/D17453
      0e7a5623
  7. 12 Jan, 2017 1 commit
    • epriestley's avatar
      Provide bucketing for commits in Audit · a635da68
      epriestley authored
      Summary:
      Fixes T9430. Fixes T9362. Fixes T9544. This changes the default view of Audit to work like Differential, where commits you need to audit or respond to are shown in buckets.
      
      This is a bit messy and probably needs some followups. This stuff has changed from a compatibility viewpoint:
      
        - The query works differently now (but in a better, modern way), so existing saved queries will need to be updated.
        - I've removed the counters from the home page instead of updating them, since they're going to get wiped out by ProfileMenu soon anyway.
        - When bucketed queries return too many results (more than 1,000) we now show a warning about it. This isn't greaaaat but it seems good enough for now.
      
      Test Plan: {F2351123}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9430, T9362, T9544
      
      Differential Revision: https://secure.phabricator.com/D17192
      a635da68
  8. 08 Jan, 2017 2 commits
    • epriestley's avatar
      (stable) Fix excessively strict "Can Use Application" policy filtering · 40be2d53
      epriestley authored
      Summary:
      Ref T9058. The stricter filtering is over-filtering Handles. For example, in the Phacility cluster, users can not see Almanac services.
      
      So this filtering happens:
      
        - The AlmanacServiceQuery filters the service beacuse they can't see the application.
        - The HandleQuery generates a "you can't see this" handle.
        - But then the HandleQuery filters that handle! It has a "service" PHID and the user can't see Almanac.
      
      This violates the assumption that all application code makes about handles: it's OK to query handles for objects you can't see, and you'll get something back.
      
      Instead, don't do application filtering on handles.
      
      Test Plan:
        - Added a failing test and made it pass.
        - As a user who can not see Almanac, viewed an Instances timeline.
          - Before patch: fatal on trying to load a handle for a Service.
          - After patch: smooth sailing.
      
      Reviewers: chad
      
      Maniphest Tasks: T9058
      
      Differential Revision: https://secure.phabricator.com/D17152
      40be2d53
    • epriestley's avatar
      Fix excessively strict "Can Use Application" policy filtering · f16778fc
      epriestley authored
      Summary:
      Ref T9058. The stricter filtering is over-filtering Handles. For example, in the Phacility cluster, users can not see Almanac services.
      
      So this filtering happens:
      
        - The AlmanacServiceQuery filters the service beacuse they can't see the application.
        - The HandleQuery generates a "you can't see this" handle.
        - But then the HandleQuery filters that handle! It has a "service" PHID and the user can't see Almanac.
      
      This violates the assumption that all application code makes about handles: it's OK to query handles for objects you can't see, and you'll get something back.
      
      Instead, don't do application filtering on handles.
      
      Test Plan:
        - Added a failing test and made it pass.
        - As a user who can not see Almanac, viewed an Instances timeline.
          - Before patch: fatal on trying to load a handle for a Service.
          - After patch: smooth sailing.
      
      Reviewers: chad
      
      Maniphest Tasks: T9058
      
      Differential Revision: https://secure.phabricator.com/D17152
      f16778fc
  9. 02 Jan, 2017 1 commit
    • epriestley's avatar
      Apply application visibility checks during normal object filtering · cf1ccc99
      epriestley authored
      Summary:
      Fixes T9058. Normally, "Query" classes apply an application check and just don't load anything if it fails.
      
      However, in some cases (like email recipient filtering) we run policy checks without having run a Query check first. In that case, one user (the actor) loads the object, then we filter it against other users (the recipeints).
      
      Explicitly apply the application check during normal filtering.
      
      Test Plan: Added a failing test case and made it pass.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9058
      
      Differential Revision: https://secure.phabricator.com/D17127
      cf1ccc99
  10. 09 Nov, 2016 1 commit
  11. 25 Feb, 2016 1 commit
    • epriestley's avatar
      Simplify locking of Almanac cluster services · 944539a7
      epriestley authored
      Summary:
      Fixes T6741. Ref T10246. Broadly, we want to protect Almanac cluster services:
      
        - Today, against users in the Phacility cluster accidentally breaking their own instances.
        - In the future, against attackers compromising administrative accounts and adding a new "cluster database" which points at hardware they control.
      
      The way this works right now is really complicated: there's a global "can create cluster services" setting, and then separate per-service and per-device locks.
      
      Instead, change "Can Create Cluster Services" into "Can Manage Cluster Services". Require this permission (in addition to normal permissions) to edit or create any cluster service.
      
      This permission can be locked to "No One" via config (as we do in the Phacility cluster) so we only need this one simple setting.
      
      There's also zero reason to individually lock //some// of the cluster services.
      
      Also improve extended policy errors.
      
      The UI here is still a little heavy-handed, but should be good enough for the moment.
      
      Test Plan:
        - Ran migrations.
        - Verified that cluster services and bindings reported that they belonged to the cluster.
        - Edited a cluster binding.
        - Verified that the bound device was marked as a cluster device
        - Moved a cluster binding, verified the old device was unmarked as a cluster device.
        - Tried to edit a cluster device as an unprivileged user, got a sensible error.
      
      {F1126552}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T6741, T10246
      
      Differential Revision: https://secure.phabricator.com/D15339
      944539a7
  12. 11 Jan, 2016 1 commit
    • epriestley's avatar
      Fix incorrect key handling in extended policy filtering · f59ebf4c
      epriestley authored
      Summary:
      Via HackerOne. The use of `$key` here should be `$extended_key`.
      
      Exploiting this requires a very unusual group of objects to be subjected to extended policy checks. I believe there is no way to actually get anything bad through the policy filter today, but this could have been an issue in the future.
      
      Test Plan:
        - Added a unit test which snuck something through the policy filter.
        - Fixed use of `$extended_key`.
        - Test now passes.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Differential Revision: https://secure.phabricator.com/D14993
      f59ebf4c
  13. 24 Dec, 2015 1 commit
    • epriestley's avatar
      Implement a "Project Members" object policy rule · 1c572d1d
      epriestley authored
      Summary:
      Fixes T9019. Pretty much ripped from D14467. I added the "policy hint" stuff so that you can create a project with this policy immediately.
      
      I really dislike how the "hint" code works, but we //almost// never need to use it and the badness feels fairly well-contained.
      
      Also pick up a quick feedback fix from D14863.
      
      Test Plan:
        - Added test coverage, got it to pass.
        - Created a project with "Visible To: Project Members".
      
      Reviewers: joshuaspence, chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9019
      
      Differential Revision: https://secure.phabricator.com/D14869
      1c572d1d
  14. 23 Dec, 2015 1 commit
    • epriestley's avatar
      Implement child/descendant query rules in Projects · 70f6bf30
      epriestley authored
      Summary:
      Ref T10010. This adds infrastructure for querying projects by type, depth, parent and ancestor.
      
      I needed to revise the "extended policy check" cycle detection rules. When, e.g., querying a grandchild, they incorrectly detected a cycle because both the child and grandchild needed to check the policy of the grandparent.
      
      Instead, simplify it to just do a basic runaway calldepth check. There are many other safety mechanisms to make it so this can't ever occur.
      
      (Cycle detection does have existing test coverage, and those tests still pass, it just takes a little longer to detect the cycle internally.)
      
      There is still no way to create subprojects in the UI.
      
      Test Plan: Added and executed unit tests.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T10010
      
      Differential Revision: https://secure.phabricator.com/D14862
      70f6bf30
  15. 03 Sep, 2015 1 commit
    • epriestley's avatar
      Modernize OAuthserver and provide more context on "no permission" exception · 9d0332c2
      epriestley authored
      Summary:
      Ref T7173. Depends on D14049. Now that Phacility can install custom exception handlers, this puts enough information on the exception so that we can figure out what to do with it.
      
        - Generally modernize some of this code.
        - Add some more information to PolicyExceptions so the new RequestExceptionHandler can handle them properly.
      
      Test Plan: Failed authorizations, then succeeded authorizations. See next diff.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T7173
      
      Differential Revision: https://secure.phabricator.com/D14050
      9d0332c2
  16. 15 Jun, 2015 1 commit
    • Joshua Spence's avatar
      Extend from Phobject · b6d745b6
      Joshua Spence authored
      Summary: All classes should extend from some other class. See D13275 for some explanation.
      
      Test Plan: `arc unit`
      
      Reviewers: epriestley, #blessed_reviewers
      
      Reviewed By: epriestley, #blessed_reviewers
      
      Subscribers: epriestley, Korvin
      
      Differential Revision: https://secure.phabricator.com/D13283
      b6d745b6
  17. 13 Jun, 2015 2 commits
    • epriestley's avatar
      Allow PolicyRules to serve as "Object Policies" · 46675547
      epriestley authored
      Summary:
      Ref T5681. Ref T8488. This allows policy rules to provide "Object Policies", which are similar to the global/basic policies:
      
        - They show up directly in the dropdown (you don't have to create a custom rule).
        - They don't need to create or load anything in the database.
      
      To implement one, you just add a couple methods on an existing PolicyRule that let Phabricator know it can work as an object policy rule.
      
      {F494764}
      
      These rules only show up where they make sense. For example, the "Task Author" rule is only available in Maniphest, and in "Default View Policy" / "Default Edit Policy" of the Application config.
      
      This should make T8488 easier by letting us set the default policies to "Members of Thread", without having to create a dedicated custom policy for every thread.
      
      Test Plan:
        - Set tasks to "Task Author" policy.
        - Tried to view them as other users.
        - Viewed transaction change strings.
        - Viewed policy errors.
        - Set them as default policies.
        - Verified they don't leak into other policy controls.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T5681, T8488
      
      Differential Revision: https://secure.phabricator.com/D13257
      46675547
    • epriestley's avatar
      Allow different policy rules for different types of objects · 7f98a857
      epriestley authored
      Summary:
      Ref T5681. Policy rules can now select objects they can apply to, so a rule like "task author" only shows up where it makes sense (when defining task policies).
      
      This will let us define rules like "members of thread" in Conpherence, "subscribers", etc., to make custom policies more flexible.
      
      Notes:
      
        - Per D13251, we need to do a little work to get the right options for policies like "Maniphest > Default View Policy". This should allow "task" policies.
        - This implements a "task author" policy as a simple example.
        - The `willApplyRule()` signature now accepts `$objects` to support bulk-loading things like subscribers.
      
      Test Plan:
        - Defined a task to be "visible to: task author", verified author could see it and other users could not.
        - `var_dump()`'d willApplyRule() inputs, verified they were correct (exactly the objects which use the rule).
        - Set `default view policy` to a task-specific policy.
        - Verified that other policies like "Can Use Bulk Editor" don't have these options.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T5681
      
      Differential Revision: https://secure.phabricator.com/D13252
      7f98a857
  18. 05 Jun, 2015 2 commits
    • epriestley's avatar
      Provide core policy support for Spaces · c1c897b9
      epriestley authored
      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.
      
      Reviewers: btrahan, chad
      
      Reviewed By: btrahan
      
      Subscribers: chad, epriestley
      
      Maniphest Tasks: T8424
      
      Differential Revision: https://secure.phabricator.com/D13156
      c1c897b9
    • epriestley's avatar
      Make policy violation dialog more flexible · e595478f
      epriestley authored
      Summary:
      Ref T8424. When users are rejected because they can't see the space an object is in, this isn't really a capability rejection. Don't require a capability when rejecting objects.
      
      This mostly simplifies upcoming changes.
      
      Test Plan:
        - Viewed a capability exception dialog, it looked the same as always.
        - (After additional changes, viewed a space exception dialog.)
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T8424
      
      Differential Revision: https://secure.phabricator.com/D13155
      e595478f
  19. 04 Jun, 2015 1 commit
    • epriestley's avatar
      Add support for "Extended Policies" · d6247ffc
      epriestley authored
      Summary:
      Ref T7703. See that task and inline for a bunch of discussion.
      
      Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.
      
      We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.
      
      Test Plan:
        - Added and executed unit tests.
        - Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
        - Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T7703
      
      Differential Revision: https://secure.phabricator.com/D13104
      d6247ffc
  20. 22 May, 2015 1 commit
  21. 10 Feb, 2015 1 commit
  22. 09 Oct, 2014 1 commit
    • Bob Trahan's avatar
      Policy - fix error message · 45bb7753
      Bob Trahan authored
      Summary: We were saying "Object Restricted Object"; instead say "Restricted Object". Fixes T6104.
      
      Test Plan: made a restricted paste and a restricted task and saw good error messages.  {F215281}  {F215282}
      
      Reviewers: epriestley, chad
      
      Reviewed By: chad
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6104
      
      Differential Revision: https://secure.phabricator.com/D10668
      45bb7753
  23. 23 Jul, 2014 1 commit
    • Joshua Spence's avatar
      Rename `PHIDType` classes · 97a8700e
      Joshua Spence authored
      Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
      
      Test Plan: Ran unit tests.
      
      Reviewers: epriestley, #blessed_reviewers
      
      Reviewed By: epriestley, #blessed_reviewers
      
      Subscribers: epriestley, Korvin, hach-que
      
      Maniphest Tasks: T5655
      
      Differential Revision: https://secure.phabricator.com/D9986
      97a8700e
  24. 10 Feb, 2014 1 commit
    • epriestley's avatar
      Make the "you can't edit away your edit capability" policy check generic · e4e4810b
      epriestley authored
      Summary:
      Ref T4379. Currently, you can edit away your edit capability in Projects. Prevent this in a general way.
      
      Since some objects have complex edit policies (like "the owner can always edit"), we can't just check the value itself. We also can't fairly assume that every object has a `setEditPolicy()` method, even though almost all do right now. Instead, provide a way to pretend we've completed the edit and changed the policy.
      
      Test Plan: Unit tests, tried to edit away my edit capability.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T4379
      
      Differential Revision: https://secure.phabricator.com/D8179
      e4e4810b
  25. 06 Dec, 2013 1 commit
    • epriestley's avatar
      Fix an error in the PolicyFilter algorithm · faaaff0b
      epriestley authored
      Summary:
      `PhabricatorPolicyFilter` has a bug right now where it lets through objects incorrectly if:
      
        - the query requests two or more policies;
        - the object satisfies at least one of those policies; and
        - policy exceptions are not enabled.
      
      This would be bad, but there's only one call in the codebase which satisfies all of these conditions, in the Maniphest batch editor. And it's moot anyway because edit operations get another policy check slightly later. So there is no policy/security impact from this flaw.
      
      (The next diff relies on this behavior, which is how I caught it.)
      
      Test Plan:
        - Added a failing unit test and made it pass.
        - Grepped the codebase for `requireCapabilities()` and verified that there is no security impact. Basically, 99% of callsites use `executeOne()`, which throws anyway and moots the filtering.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Differential Revision: https://secure.phabricator.com/D7721
      faaaff0b
  26. 19 Nov, 2013 1 commit
    • epriestley's avatar
      Never raise policy exceptions for the omnipotent viewer · c2079640
      epriestley authored
      Summary:
      Fixes T4109. If a revision has a bad `repositoryPHID` (for example, because the repository was deleted), `DifferentialRevisionQuery` calls `didRejectResult()` on it, which raises a policy exception, even if the viewer is omnipotent. This aborts the `MessageParser`, because it does not expect policy exceptions to be raised for an omnipotent viewer.
      
      Fix this in two ways:
      
        # Never raise a policy exception for an omnipotent viewer. I think this is the expected behavior and a reasonable rule.
        # In this case, load the revision for an omnipotent viewer.
      
      This feels a little gross, but it's the only place where we do this in the codebase right now. We can clean this up later on once it's more clear what the circumstances of checks like these are.
      
      Test Plan: Set a revision to have an invalid `repositoryPHID`, ran message parser on it, got a clean parse.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T4109
      
      Differential Revision: https://secure.phabricator.com/D7603
      c2079640
  27. 14 Oct, 2013 3 commits
    • epriestley's avatar
      Make PhabricatorPolicyInterface require a getPHID() method · 073cb0e7
      epriestley authored
      Summary:
      Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
      
      Some policy objects don't have real PHIDs:
      
        PhabricatorTokenGiven
        PhabricatorSavedQuery
        PhabricatorNamedQuery
        PhrequentUserTime
        PhabricatorFlag
        PhabricatorDaemonLog
        PhabricatorConduitMethodCallLog
        ConduitAPIMethod
        PhabricatorChatLogEvent
        PhabricatorChatLogChannel
      
      Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
      
      Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
      
      Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7306
      073cb0e7
    • epriestley's avatar
      Rename ACTION_ACCEPT into ACTION_ALLOW · 6c1b00fa
      epriestley authored
      Summary: Ref T603. This is "Allow" in the UI, I just mistyped it when I created the constant.
      
      Test Plan: `grep`
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7296
      6c1b00fa
    • epriestley's avatar
      Allow custom policies to be loaded and exeucuted by the policy filter · 67b17239
      epriestley authored
      Summary: Ref T603. Adds code to actually execute custom policies. (There's still no way to select them in the UI.)
      
      Test Plan:
        - Added and executed unit tests.
        - Edited policies in existing applications.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7292
      67b17239
  28. 09 Oct, 2013 2 commits
    • epriestley's avatar
      Allow "Default View" policies to be set to Public · f4582dc4
      epriestley authored
      Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions.
      
      Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy.
      
      Reviewers: btrahan, asherkin
      
      Reviewed By: asherkin
      
      CC: asherkin, aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7276
      f4582dc4
    • epriestley's avatar
      Add defualt view and default edit policies for tasks · 1ee455c4
      epriestley authored
      Summary: Ref T603. Allow global default policies to be configured for tasks.
      
      Test Plan:
        - Created task via web UI.
        - Created task via Conduit.
        - Created task via email.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7267
      1ee455c4
  29. 07 Oct, 2013 2 commits
    • epriestley's avatar
      Allow applications to define new policy capabilities · b1b1ff83
      epriestley authored
      Summary:
      Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.
      
      Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.
      
      That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.
      
      Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.
      
      Also provide more context, and more useful exception messages.
      
      Test Plan:
        - See screenshots.
        - Also triggered a policy exception and verified it was dramatically more useful than it used to be.
      
      Reviewers: btrahan, chad
      
      Reviewed By: btrahan
      
      CC: chad, aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7260
      b1b1ff83
    • epriestley's avatar
      Remove dead `rejectImpossiblePolicy()` method · 68c854b9
      epriestley authored
      Summary: Ref T603. Apparently we made all policies possible at some point. Go us! This has no callsites.
      
      Test Plan: `grep`, notice it's a private method
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7259
      68c854b9
  30. 04 Oct, 2013 1 commit
    • epriestley's avatar
      Tighten up some policy interactions in Herald · c8127edf
      epriestley authored
      Summary:
      Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change:
      
        - Introduces "create" and "create global" permisions for Herald.
          - Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent.
        - Add some application policy helper functions.
        - Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right.
        - Don't let users who can't use application X create Herald rules for application X.
        - Remove Maniphest/Pholio rules when those applications are not installed.
      
      Test Plan:
        - Restricted access to Maniphest and uninstalled Pholio.
        - Verified Pholio rules no longer appear for anyone.
        - Verified Maniphest ruls no longer appear for restricted users.
        - Verified users without CREATE_GLOBAL can not create global ruls.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603
      
      Differential Revision: https://secure.phabricator.com/D7219
      c8127edf
  31. 01 Oct, 2013 2 commits
    • epriestley's avatar
      Fix a variable name PolicyFilter. · 120cc28d
      epriestley authored
      120cc28d
    • epriestley's avatar
      Treat invalid policies as broadly similar to "no one" · 4dfdd0d3
      epriestley authored
      Summary:
      Ref T3903. Ref T603. We currently overreact to invalid policies. Instead:
      
        - For non-omnipotent users, just reject the viewer.
        - For omnipotent users, we already shortcircuit and permit the viewer.
        - Formalize and add test coverage for these behaviors.
      
      Also clean up some strings.
      
      The practical effect of this is that setting an object to an invalid policy (either intentionally or accidentally) doesn't break callers who are querying it.
      
      Test Plan:
        - Created a Legalpad document and set view policy to "asldkfnaslkdfna".
        - Verified this policy behaved as though it were "no one".
        - Added, executed unit tests.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      CC: aran
      
      Maniphest Tasks: T603, T3903
      
      Differential Revision: https://secure.phabricator.com/D7185
      4dfdd0d3