1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 08:12:40 +01:00
Commit graph

216 commits

Author SHA1 Message Date
epriestley
56d44f1503 Modularize the Garbage Collector
Summary:
The GC is a big block of hard-coded application GCs right now. Among other things, this means third parties can't tap into the infrastructure.

Modularize it into `GarbageCollector` classes. This implements only one to prove the new stuff works; I'll followup with the rest in the next diff or few depending on how much mess I run into.

Test Plan: Used `bin/phd debug garbage` to run the collector in debug mode, observed reasonable output and behavior.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7970
2014-01-15 10:02:24 -08:00
epriestley
6cc5f952a4 Fix Herald field type error
Summary: See IRC. This is a likely fix for @DctrWatson's error: we're returning `null` but should return `array()` to indicate no values.

Test Plan: Will make @DctrWatson do it.

Reviewers: btrahan

Reviewed By: btrahan

CC: dctrwatson, aran

Differential Revision: https://secure.phabricator.com/D7950
2014-01-13 16:08:30 -08:00
Chad Little
b74c7a3d37 Simplify PHUIObjectBoxViews handling of Save and Error states
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.

Test Plan: Test out the forms, see errors without the text.

Reviewers: epriestley, btrahan

CC: Korvin, epriestley, aran, hach-que

Differential Revision: https://secure.phabricator.com/D7924
2014-01-10 09:17:37 -08:00
epriestley
efe187d5be Support "Repository's projects" field in Commit and Differential Revision rules
Summary: This also cleans up some code a little bit. Most of the gymnastics are to make sure we call `needProjectPHIDs()` appropriately.

Test Plan: Created new commit and revision rules with this field. Ran commits and revisions through the test console. Field behavior seemed correct.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, dctrwatson

Differential Revision: https://secure.phabricator.com/D7923
2014-01-09 15:56:24 -08:00
Bob Trahan
8867e50d63 Herald - tweak accepted differential revision check slightly
Summary: use the loadReviewedBy function, which seems to do what we want -- returns a reviewer IFF the last thing was an accept

Test Plan: i believe in the power of loadReviewedBy

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran, aarwine

Differential Revision: https://secure.phabricator.com/D7903
2014-01-08 11:53:13 -08:00
epriestley
8c360ab703 Rename "Apply build plans" to "Run build plans" in Herald
Summary: "Run" is clearer than "Apply". This has already been changed in Harbormaster itself.

Test Plan: used eyeballs

Reviewers: btrahan, zeeg

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7889
2014-01-06 12:14:21 -08:00
epriestley
3627e73e5e Apply "enormous changes" rules to pre-commit content rules too
Summary:
Fixes T4276. This adds "Change is enormous" to pre-commit content rules so we can, e.g., just reject these and not worry about them elsewhere.

Also, use the same numeric limits across the mechanisms so there's a consistent definition of an "enormous" changeset.

Test Plan:
  - Set enormous limit to 15 bytes, pushed some changes, got blocked by a rule.
  - Set it back, pushed OK.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4276

Differential Revision: https://secure.phabricator.com/D7887
2014-01-06 12:12:30 -08:00
epriestley
8ddf883d2e Cut Herald rules off at 1GB of diff text
Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.

Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:

  - Add an optional byte limit to `diffusion.rawdiffquery`.
  - Make the query with a 1GB limit.
  - Reduce the diff timeout from 15 hours to 15 minutes.
  - Add a "Changeset is enormous" field. This field is true for changes which are too large to process.

This generally makes behaviors more sane:

  - We'll always make progress in Herald in a reasonable amount of time.
  - Installs can write global rules to handle (or reject) these types of changes.

Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4276

Differential Revision: https://secure.phabricator.com/D7885
2014-01-03 12:27:19 -08:00
epriestley
2cfc3acf32 Allow Herald pre-commit rules to act on repository projects
Summary:
Fixes T4264. Adds:

  - New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects.
  - Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories.
  - The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class.

Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7883
2014-01-03 12:24:28 -08:00
epriestley
140c88e971 Implement basic object rules for Herald
Summary:
Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it.

Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs.

Test Plan:
  - Created non-object rules.
  - Created object rules.
  - Triggered object rules against matching and unmatching objects.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3506, T4264

Differential Revision: https://secure.phabricator.com/D7853
2013-12-30 16:48:14 -08:00
epriestley
472b0f983e Allow Herald Adapters to choose applicable rule types (global, personal, etc).
Summary: Ref T4264. Lays the groundwork for new "Object" rule types. Prevents personal "Hook" rules, which don't make any sense.

Test Plan: Created new Maniphest (global/personal available) and Ref Hook (global only) rules.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7852
2013-12-30 16:48:07 -08:00
epriestley
f5fb3f05dc Lay most groundwork for Herald object rules
Summary:
Ref T4264. This gets most of the plumbing in for "object" rules, which will bind to a specific object, like a repository or project.

It does not yet let you actually create these rules.

Test Plan: Ran `storage upgrade`, created/edited rules, browsed Herald.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7847
2013-12-27 13:17:10 -08:00
epriestley
f38a565aa5 Use radio buttons with explanatory text to select commit rule types
Summary: Ref T4264. Instead of a dropdown, make this step more informative.

Test Plan: {F93928}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7846
2013-12-27 13:16:33 -08:00
epriestley
79f57cf517 Split Herald rule creation across several steps
Summary:
Ref T4264. Currently, you choose a rule's content type (revision, commit, hook) and rule type (global, personal) on the same screen.

  - I want to make some rule types unavailable for some content types (e.g., personal hooks make little sense).
  - I want to make content type selection use a radio control instead of a dropdown, so it can explain what the content types do in more detail.
  - For new "object" hooks, I want to add a third step where you'll pick an object to bind to.

Split rule creation out into two steps. I think this won't get complicated enough for `PHUIPagedFormView`, but maybe I'll swap it in if this gets messier than I think.

Test Plan: Created some Herald rules, used back/cancel/etc.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4264

Differential Revision: https://secure.phabricator.com/D7845
2013-12-27 13:16:25 -08:00
epriestley
9f38aaa5de Add "raw author name" and "raw committer name" as Herald fields for commit content hooks
Summary:
Ref T4195. A legitimate rule which needs this field is "do not allow commits as root". Interestingly, we have exactly one commit as root in each Phabricator, Arcanist and libphutil.

Since the committer and author don't need to be Phabricator accounts (just the Pusher), the existing "Committer" and "Author" fields can't express this rule (they'll be empty).

Test Plan: {F93406}

Reviewers: btrahan

Reviewed By: btrahan

CC: SEJeff, aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7841
2013-12-27 13:16:00 -08:00
epriestley
adcc4ee1db Add a "branches" rule for Herald commit rules
Summary:
Fixes T4195. Allows you to write a rule against a commit's branches.

This completes outstanding work on T4195.

Test Plan: Pushed to Git and Mercurial repositories and verified branches were selected correctly by examining transcripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7820
2013-12-26 10:40:16 -08:00
xiaogaozi
54a0dd8139 Herald - add support for "assignee" conditions
Summary: add support for "assignee" conditions

Test Plan: Create a Herald rule where condition is assignee, and create a task assign to someone.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7813
2013-12-22 08:47:56 -08:00
epriestley
e27bbb9aa5 Enable the "Accepted Differential Revision" field for Herald post-commit hooks
Summary: I implemented this field, but didn't actually enable it.

Auditors: btrahan
2013-12-21 11:11:52 -08:00
epriestley
a64d127e25 Add "is merge commit" Herald field for pre-commit rules
Summary:
Ref T4195. This allows you to write rules which disallow merge commits.

Also make the reject message a little more useful.

Test Plan:
  remote: This push was rejected by Herald push rule H27.
  remote: Change: commit/daed0d448404
  remote:   Rule: No Merges
  remote: Reason: No merge commits allowed. If you must push a merge, include "@force-merge" in the commit message.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7809
2013-12-20 12:39:40 -08:00
epriestley
72c73d644b Add an "Accepted Differential revision" field to Commit and pre-commit Content Herald rules
Summary: Refs T4195. Fixes T3936. You can't currently write rules like "block commits unless they're attached to an **accepted** revision"; allow that.

Test Plan: Pushed commits into a rule with this field, saw it work / not crash.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, mbishopim3

Maniphest Tasks: T3936, T4195

Differential Revision: https://secure.phabricator.com/D7807
2013-12-20 12:39:13 -08:00
epriestley
2436458b90 Implement "Differential Revision" fields in Herald pre-commit content adapter
Summary: Ref T4195. Allows you to write revision-based commit hooks, e.g. block all commits with no corresponding revision.

Test Plan:
Here's are the fields populating:

{F90989}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7806
2013-12-20 12:39:01 -08:00
epriestley
d64dd7e2e8 Allow global Commit herald rules to trigger audits by users
Summary: Ref T4249. Currently, a global rule can only trigger project audits. Although there probably aren't a huge number of use cases for triggering users from global rules, it works fine and it's somewhat confusing not to allow it.

Test Plan: {F90902}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4249

Differential Revision: https://secure.phabricator.com/D7803
2013-12-19 10:33:15 -08:00
epriestley
151f01ae94 Implement "Body" field in Herald pre-commit content hooks
Summary: Ref T4195. Adds support for writing rules against commit message bodies.

Test Plan: Pushed git, hg, svn commits and verified their bodies populated correctly in transcripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7796
2013-12-19 06:56:01 -08:00
epriestley
a5dc9067af Provide convenience method addTextCrumb() to PhabricatorCrumbsView
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.

Test Plan:
  - This was mostly automated, then I cleaned up a few unusual sites manually.
  - Bunch of grep / randomly clicking around.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: hach-que, aran

Differential Revision: https://secure.phabricator.com/D7787
2013-12-18 17:47:34 -08:00
epriestley
5f4df0f3e3 Support "changed filename" and "file content" fields for commit content Herald rules
Summary: Ref T4195. Adds support for diff content rules.

Test Plan: Pushed SVN and Git changes through, saw them generate reasonable transcripts. Mercurial still isn't hooked up to this phase.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7791
2013-12-18 14:18:58 -08:00
epriestley
e115f11f80 Provide basic commit content hooks for Herald
Summary: Ref T4195. This doesn't provide any interesting fields yet (content, affected paths, commit message) but fires the hook correctly.

Test Plan: Added a blocking hook and saw it fire.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7789
2013-12-18 14:18:45 -08:00
epriestley
1ff3ef382d Give Herald rules a standard "Hnnn" object name
Summary: Allow Herald rules to be referred to with `H123`, etc., like other object types are. Herald rules now have proper PHIDs and an increasingly prominent role in triggering application actions. Although I suspect users will rarely use `H123` in Remarkup to mention rules, this can simplify some of the interfaces which relate objects across systems.

Test Plan: Looked at various interfaces and saw `H123` names. Mentioned `H123` in remarkup.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7786
2013-12-18 12:00:18 -08:00
epriestley
181bfffaa1 Truncate object fields in Herald transcripts
Summary:
A few users have hit cases where Herald transcripts of large commits exceed the MySQL packet limit, because one of the fields in the transcript is an enomrous textual diff.

There's no value in saving these huge amounts of data. Transcripts are useful for understanding the action of Herald rules, but can be reconstructed later. Instead of saving all of the data, limit each field to 4KB of data.

For strings, we just truncate at 4KB. For arrays, we truncate after 4KB of values.

Test Plan: Ran unit tests. Artificially decreased limit and ran transcripts, saw them truncate properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: frgtn, aran

Differential Revision: https://secure.phabricator.com/D7783
2013-12-18 11:59:53 -08:00
epriestley
3386920971 Add Herald support for blocking ref changes
Summary: Ref T4195. Allows users to write Herald rules which block ref changes. For example, you can write a rule like `alincoln can not create branches`, or `no one can push to the branch "frozen"`.

Test Plan:
This covers a lot of ground. I created and pushed a bunch of rules, then looked at transcripts, in general. Here are some bits in detail:

Here's a hook-based reject message:

  >>> orbital ~/repos/POEMS $ git push
  Counting objects: 5, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (3/3), done.
  Writing objects: 100% (3/3), 274 bytes, done.
  Total 3 (delta 2), reused 0 (delta 0)
  remote: +---------------------------------------------------------------+
  remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
  remote: +---------------------------------------------------------------+
  remote:             \
  remote:              \                    ^    /^
  remote:               \                  / \  // \
  remote:                \   |\___/|      /   \//  .\
  remote:                 \  /V  V  \__  /    //  | \ \           *----*
  remote:                   /     /  \/_/    //   |  \  \          \   |
  remote:                   @___@`    \/_   //    |   \   \         \/\ \
  remote:                  0/0/|       \/_ //     |    \    \         \  \
  remote:              0/0/0/0/|        \///      |     \     \       |  |
  remote:           0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
  remote:        0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
  remote:                    ,-}        _      *-.|.-~-.           .~    ~
  remote:   \     \__/        `/\      /                 ~-. _ .-~      /
  remote:    \____(Oo)           *.   }            {                   /
  remote:    (    (--)          .----~-.\        \-`                 .~
  remote:    //__\\  \ DENIED!  ///.----..<        \             _ -~
  remote:   //    \\               ///-._ _ _ _ _ _ _{^ - - - - ~
  remote:
  remote:
  remote: This commit was rejected by Herald pre-commit rule H24.
  remote: Rule: No Branches Called Blarp
  remote: Reason: "blarp" is a bad branch name
  remote:
  To ssh://dweller@localhost/diffusion/POEMS/
   ! [remote rejected] blarp -> blarp (pre-receive hook declined)
  error: failed to push some refs to 'ssh://dweller@localhost/diffusion/POEMS/'

Here's a transcript, showing that all the field values populate sensibly:

{F90453}

Here's a rule:

{F90454}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7782
2013-12-17 15:23:55 -08:00
epriestley
baa756a027 Add rulePHID to HeraldEffect
Summary: Ref T4195. Herald rules gained PHIDs only recently, propagate them to HeraldEffect to make some of the hook stuff eaiser.

Test Plan: iiam

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7781
2013-12-17 15:23:45 -08:00
epriestley
6b1ec35cf3 Allow Herald rules to check for revisions with no reviewers
Summary: Fixes T4225. Adds the NON_EXISTS condition to Herald for "Reviewers", and adds a few more conditions which have reasonable meanings.

Test Plan: Used test console to check a revision with reviewers, and another without reviewers. Both produced the expected results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4225

Differential Revision: https://secure.phabricator.com/D7757
2013-12-11 14:46:39 -08:00
Richard van Velzen
16a7eaa700 Fix wrong link to "Create Rule" in Herald mobile view
Summary: The link pointed to `create/`, which gives as `404`.

Test Plan: clicked the link. It worked.

Reviewers: epriestley, #blessed_reviewers, chad

Reviewed By: chad

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7698
2013-12-04 04:57:34 -08:00
epriestley
7f11e8d740 Improve handling of email verification and "activated" accounts
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:

  - Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
    - Migrate all the existing users.
    - When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
    - Just make the checks look at the `isEmailVerified` field.
  - Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
  - Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
    - When the queue is enabled, registering users are created with `isApproved = false`.
    - Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
    - They go to the web UI and approve the user.
    - Manually-created accounts are auto-approved.
    - The email will have instructions for disabling the queue.

I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.

Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.

Test Plan:
  - Ran migration, verified `isEmailVerified` populated correctly.
  - Created a new user, checked DB for verified (not verified).
  - Verified, checked DB (now verified).
  - Used Conduit, People, Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D7572
2013-11-12 14:37:04 -08:00
Bob Trahan
f35ce505a1 Herald - add ability to conditionalize on Maniphest Task projects
Summary: adds FIELD_PROJECTS and deploys it to Maniphest Task Herald Adapter. Went with "projects" because it feels like that could go well in other Adapters that want to conditionalize based on project.

Test Plan: made a new herald rule to be cc'd if project foo was on a task. it worked!

Reviewers: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7564
2013-11-11 14:20:31 -08:00
Jakub Vrana
a29b5b070f Replace some hsprintf() by phutil_tag()
Test Plan: Looked at a diff with inline comment.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7549
2013-11-11 09:23:23 -08:00
James Rhodes
7ffea0463e Use herald to trigger builds of revisions and commits.
Summary:
Depends on D7500.

This seemed like a pretty good idea once I thought of it.  Instead of having some custom triggering logic instead Harbormaster, I figured it best to leverage all of Herald's power so that users can create rules to apply builds to commits and differential revisions.  This gives the added advantage that they can trigger off builds for particular types of revisions and commits, which seems like it could be really useful (e.g. run extra tests against revisions that touch sensitive areas of the code).

Test Plan: Ran the usual daemons + the Harbormaster daemon.  Pushed a commit to the repository and saw both the buildable and build get created when the commit worked picked it up.  Submitted a diff and saw both the buildable and build get created when the Herald rules were evaluated for the diff.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, hwinkel

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7501
2013-11-08 16:58:39 -08:00
epriestley
038f5323c0 Allow Herald's "Reviewers" field to select project reviewers
Summary: The "Reviewers" condition in Differential Revision rules has the wrong typeahead and can't select projects, but should be able to.

Test Plan: {F79273}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7526
2013-11-07 11:23:31 -08:00
Bob Trahan
da84546058 Add filter by object ability to flag query
Summary: See title. Fixes T1809.

Test Plan:
verified each type that has flaggable interface still can be flagged

verified that new custom query filter works

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1809

Differential Revision: https://secure.phabricator.com/D7392
2013-10-25 12:52:00 -07:00
epriestley
2c3d071a26 Improve exception behavior for Herald commit rules which fail to load diff context
Summary:
This code is a little funky right now, and can return `array("error message")` and then try to call `getHunks()` on it. Additionally, each field loads the commit's changes separately.

Instead, load the commit's changes once and cache them, and handle exceptions appropriately.

Test Plan:
  - Created a rule like "changed, added, removed content all match /.*/" to force all fields to generate.
  - Ran it successfully.
  - Faked an error and ran it, got reasonable results.

Reviewers: btrahan

Reviewed By: btrahan

CC: bigo, aran

Differential Revision: https://secure.phabricator.com/D7384
2013-10-23 08:28:58 -07:00
epriestley
2a5c987c71 Lock policy queries to their applications
Summary:
While we mostly have reasonable effective object accessibility when you lock a user out of an application, it's primarily enforced at the controller level. Users can still, e.g., load the handles of objects they can't actually see. Instead, lock the queries to the applications so that you can, e.g., never load a revision if you don't have access to Differential.

This has several parts:

  - For PolicyAware queries, provide an application class name method.
  - If the query specifies a class name and the user doesn't have permission to use it, fail the entire query unconditionally.
  - For handles, simplify query construction and count all the PHIDs as "restricted" so we get a UI full of "restricted" instead of "unknown" handles.

Test Plan:
  - Added a unit test to verify I got all the class names right.
  - Browsed around, logged in/out as a normal user with public policies on and off.
  - Browsed around, logged in/out as a restricted user with public policies on and off. With restrictions, saw all traces of restricted apps removed or restricted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7367
2013-10-21 17:20:27 -07:00
epriestley
19968e31f4 Generalize Herald account sources
Summary: The "user" and "user/project" sources exclude system agents and disabled users, but should not.

Test Plan: Added system agents to Herald rules.

Reviewers: btrahan, bigo

Reviewed By: bigo

CC: aran

Differential Revision: https://secure.phabricator.com/D7319
2013-10-14 19:38:35 -07:00
epriestley
073cb0e78c Make PhabricatorPolicyInterface require a getPHID() method
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
2013-10-14 14:35:47 -07:00
Chad Little
97c690fc0f PHUIPropertyListView
Summary: This builds out and implements PHUIPropertyListView (container) and PHUIPropertyListItemView (section) as well as adding tabs.

Test Plan: Tested each page I edited with the exception of Releeph and Phortune, though those changes look ok to me diff wise. Updated examples page with tabs.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7283
2013-10-11 07:53:56 -07:00
epriestley
650dc0cc30 Remove the "create rules" Herald capability
Summary:
Ref T603. In thinking about this, I think I went mad with power in creating this capability. I can't imagine any reason to give users access to Herald but not let them create rules.

We can restore this later if some install comes up with a good reason to have it, but in the interest of keeping policies as simple as possible, I think we're better off without it. In particular, if you don't want a group of users creating rules, just lock them out of the application entirely.

The "Manage Global Rules" capability is still around, I think that one's super good.

Test Plan: Edited Herald policies, created a rule.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7268
2013-10-09 13:55:44 -07:00
epriestley
3147a6ca57 Improve messaging of special policy rules in applications
Summary: Ref T603. When the user encounters an action which is controlled by a special policy rule in the application, make it easier for applications to show the user what policy controls the action and what the setting is. I took this about halfway before and left a TODO, but turn it into something more useful.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7265
2013-10-09 13:52:04 -07:00
epriestley
7a97a71e20 Move Herald application capabilities to newer infrastructure
Summary: Ref T603. Use the new hotness.

Test Plan: Edited Herald in Applications, tried to create rules / global rules without capabilities, got reasonable error messages.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7263
2013-10-09 13:44:41 -07:00
epriestley
0598600476 Always pass handles to tokenizers, not <phid -> name> maps
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.

Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7248
2013-10-07 12:51:24 -07:00
epriestley
c6f9316a77 Add missing case for personal ruls which create blocking reviewers
Summary: Ref T1279. I only tested the global case. :O

Test Plan: Created a personal "add me as blocking" rule.

Reviewers: btrahan, zeeg

Reviewed By: zeeg

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7261
2013-10-07 11:18:00 -07:00
epriestley
c4ecdfa2a5 Fix missing property on revision adapter. 2013-10-07 03:41:00 -07:00
epriestley
953ff197bf Allow Herald rules to be disabled, instead of deleted
Summary:
Ref T603. Ref T1279. Further improves transaction and policy support for Herald.

  - Instead of deleting rules (which wipes out history and can't be undone) allow them to be disabled.
  - Track disables with transactions.
  - Gate disables with policy controls.
  - Show policy and status information in the headers.
  - Show transaction history on rule detail screens.
  - Remove the delete controller.
  - Support disabled queries in the ApplicationSearch.

Test Plan:
  - Enabled and disabled rules.
  - Searched for enabled/disabled rules.
  - Verified disabled rules don't activate.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279, T603

Differential Revision: https://secure.phabricator.com/D7247
2013-10-06 17:10:29 -07:00