1. 15 Dec, 2014 5 commits
    • epriestley's avatar
      Add conduit.getcapabilities and a modern CLI handshake workflow · 288498f8
      epriestley authored
      Summary:
      Ref T5955.
      
        - Add `conduit.getcapabilities` to help arc (and other clients) determine formats, protocols, etc., the server supports.
        - Fixes T3117. Add a more modern version of the handshake workflow that allows all generated tokens to remain valid for an hour.
        - Generally, add a CLI token type. This token type expires after an hour when generated, then becomes permanent if used.
      
      Test Plan:
        - See D10988.
        - Ran `conduit.getcapabilities` and inspected output.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T3117, T5955
      
      Differential Revision: https://secure.phabricator.com/D10989
      288498f8
    • epriestley's avatar
      Accept Conduit tokens as an authentication mechanism · 0507626f
      epriestley authored
      Summary:
        - Ref T5955. Accept the tokens introduced in D10985 as an authentication token.
        - Ref T3628. Permit simple `curl`-compatible decoding of parameters.
      
      Test Plan:
        - Ran some sensible `curl` API commands:
      
      ```
      epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/user.whoami?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk" ; echo
      {"result":{"phid":"PHID-USER-cvfydnwadpdj7vdon36z","userName":"admin","realName":"asdf","image":"http:\/\/local.phacility.com\/res\/1410737307T\/phabricator\/3eb28cd9\/rsrc\/image\/avatar.png","uri":"http:\/\/local.phacility.com\/p\/admin\/","roles":["admin","verified","approved","activated"]},"error_code":null,"error_info":null}
      ```
      
      ```
      epriestley@orbital ~/dev/phabricator $ curl -g "http://local.phacility.com/api/differential.query?api.token=api-f7dfpoyelk4mmz6vxcueb6hcbtbk&ids[]=1" ; echo
      {"result":[{"id":"1","phid":"PHID-DREV-v3a67ixww3ccg5lqbxee","title":"zxcb","uri":"http:\/\/local.phacility.com\/D1","dateCreated":"1418405590","dateModified":"1418405590","authorPHID":"PHID-USER-cvfydnwadpdj7vdon36z","status":"0","statusName":"Needs Review","branch":null,"summary":"","testPlan":"zxcb","lineCount":"6","activeDiffPHID":"PHID-DIFF-pzbtc5rw6pe5j2kxtlr2","diffs":["1"],"commits":[],"reviewers":[],"ccs":[],"hashes":[],"auxiliary":{"phabricator:projects":[],"phabricator:depends-on":[],"organization.sqlmigration":null},"arcanistProjectPHID":null,"repositoryPHID":null,"sourcePath":null}],"error_code":null,"error_info":null}
      ```
      
        - Ran older-style commands like `arc list` against the local install.
        - Ran commands via web console.
        - Added and ran a unit test to make sure nothing is using forbidden parameter names.
        - Terminated a token and verified it no longer works.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T3628, T5955
      
      Differential Revision: https://secure.phabricator.com/D10986
      0507626f
    • epriestley's avatar
      Add Conduit Tokens to make authentication in Conduit somewhat more sane · 39f2bbae
      epriestley authored
      Summary:
      Ref T5955. Summary of intended changes:
      
      **Improve Granularity of Authorization**: Currently, users have one Conduit Certificate. This isn't very flexible, and means that you can't ever generate an API token with limited permissions or IP block controls (see T6706). This moves toward a world where you can generate multiple tokens, revoke them individually, and assign disparate privileges to them.
      
      **Standardize Token Management**: This moves Conduit to work the same way that sessions, OAuth authorizations, and temporary tokens already work, instead of being this crazy bizarre mess.
      
      **Make Authentication Faster**: Authentication currently requires a handshake (conduit.connect) to establish a session, like the web UI. This is unnecessary from a security point of view and puts an extra round trip in front of all Conduit activity. Essentially no other API anywhere works like this.
      
      **Make Authentication Simpler**: The handshake is complex, and involves deriving hashes. The session is also complex, and creates issues like T4377. Handshake and session management require different inputs.
      
      **Make Token Management Simpler**: The certificate is this huge long thing right now, which is not necessary from a security perspective. There are separate Arcanist handshake tokens, but they have a different set of issues. We can move forward to a token management world where neither of these problems exist.
      
      **Lower Protocol Barrier**: The simplest possible API client is very complex right now. It should be `curl`. Simplifying authentication is a necessary step toward this.
      
      **Unblock T2783**: T2783 is blocked on nodes in the cluster making authenticated API calls to other nodes. This provides a simpler way forward than the handshake mess (or enormous-hack-mess) which would currently be required.
      
      Test Plan:
        - Generated tokens.
        - Generated tokens for a bot account.
        - Terminated tokens (and for a bot account).
        - Terminated all tokens (and for a bot account).
        - Ran GC and saw it reap all the expired tokens.
      
      NOTE: These tokens can not actually be used to authenticate yet!
      
      {F249658}
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T5955
      
      Differential Revision: https://secure.phabricator.com/D10985
      39f2bbae
    • epriestley's avatar
      Lock `phabricator.show-prototypes` · 2c7be52f
      epriestley authored
      Summary:
      Two goals:
      
        - If an attacker compromises an administrator account (without compromising the host itself), they can currently take advantage of vulnerabilities in prototype applications by enabling the applications, then exploiting the vulnerability. Locking this option requires CLI access to enable prototypes, so installs which do not have prototypes enabled have no exposure to security issues in prototype applications.
        - Making this very slightly harder to enable is probably a good thing, given the state of the world and support.
      
      Test Plan: Verified that web UI shows the value is locked and instructs the user to update via the CLI.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D10993
      2c7be52f
    • epriestley's avatar
      Prevent Phame blogs from using invalid skins · 20379791
      epriestley authored
      Summary: Via HackerOne. An attacker with access to both Phame and the filesystem could potentially load a skin that lives outside of the configured skin directories, because we had insufficient checks on the actual skin at load time.
      
      Test Plan: Attempted to build a blog with an invalid skin; got an exception instead of a mis-load of a sketchy skin.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D10992
      20379791
  2. 14 Dec, 2014 1 commit
  3. 12 Dec, 2014 4 commits
    • epriestley's avatar
      Allow repositories to be bound to an AlmanacService · 4505724c
      epriestley authored
      Summary:
      Ref T2783. This is primarily exploratory and just figuring out what we're blocked on:
      
        - Allow a Repository to be bound to a Service. The Service may eventually define multiple read/write nodes, etc.
          - There's no UI to do this binding yet, you have to touch the database manually.
        - If a repository is bound to a Service, effect Conduit calls via calls to the remote service instead of executing them in-process.
          - These don't actually work yet since there's no authentication (see T5955).
      
      Test Plan:
        - Made a nice Service with a nice Binding to a nice Interface on a nice Device.
        - Force-associated a repository with the service using a raw MySQL query.
        - Saw Phabricator try to make a remote call to the service (on localhost) and fail because of missing auth stuff.
        - Also ran `almanac.queryservices`.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T2783
      
      Differential Revision: https://secure.phabricator.com/D10982
      4505724c
    • Bob Trahan's avatar
      Home - limit "status" queries to 100 and show 99+ if we hit that · 2b99b4ad
      Bob Trahan authored
      Summary: Fixes T6595. This diff has two issues as is... 1) the differential data fetching is pretty cheesey, but it looks like we can't just issue three separate databases to get the right data? 2) the translations break, since I am turning this into a string (and not an int) so the whole pluralization bit fails. I think 1 is okay as is and 2 needs to be fixed though I am not sure how to best do that...
      
      Test Plan: loaded home page and it looked nice...!
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6595
      
      Differential Revision: https://secure.phabricator.com/D10979
      2b99b4ad
    • Bob Trahan's avatar
      Phriction - validateTransactions that need parent ancestry to complete successfully · 905fc217
      Bob Trahan authored
      Summary:
      Fixes T6651, T6682. Since policy is defined by ancestry, you can't make things outside the core tree.
      
      An alternative fix would be to automagically stub everything in these cases. This has potential negative policy implications - consider making a public document with several levels of depth that automagically stubs out its ancestry as public - and additionally the PhabricatorApplicationTransactionEditor framework would make this very tricky code (i.e. you are expected to validateTransactions in said hook *and* return an error if things aren't valid and not do some automagic stubbing, etc.)
      
      Test Plan: tried to move a doc from location/that/exists to locationz/thatz/dontz/existz/ and got an error message with links to each missing doc. tried to create a doc at locatonz/thatz/dontz/existsz/ and got an error message with links to each missing doc.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6682, T6651
      
      Differential Revision: https://secure.phabricator.com/D10978
      905fc217
    • Bob Trahan's avatar
      Transactions - change show all key from "~" to "@" · 3a85d0bf
      Bob Trahan authored
      Summary: (some) international keyboard layouts can not type "~" in a way as to trigger this so use "@" instead. Save the suggested "+" as that seems like it would be useful for some future "adding stuff" keyboard workflow. Pretty stoked to get this squared away as I am quite confident our unreleased product will now be a huge smashing success. Ref T6683.
      
      Test Plan: made sure my choice was okay via https://en.wikipedia.org/wiki/Dead_key#Dead_keys_on_various_keyboard_layouts; used the "@" key to show all transactions
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6683
      
      Differential Revision: https://secure.phabricator.com/D10983
      3a85d0bf
  4. 11 Dec, 2014 10 commits
    • Bob Trahan's avatar
      Maniphest - load subscribers in getApplicationTransactionsObject · dda8e64a
      Bob Trahan authored
      Summary: Fixes T6734. This is a very generic fix, which basically attaches the subscribers if necessary. This seems like a good idea given there's some crazy generic code doing this sort of thing? This would end up being a new pattern for these types of objects that may be loaded by a general object query but then get some editor action against them.
      
      Test Plan: I can't actually reproduce this in my sandbox so I'll verify live again.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6734
      
      Differential Revision: https://secure.phabricator.com/D10976
      dda8e64a
    • epriestley's avatar
      Fix a stray comma on File previews · bc559886
      epriestley authored
      Summary:
      There's a comma to the lower-left of my profile picture here:
      
      {F248962}
      
      This is on a page like https://secure.phabricator.com/F248948
      
      What's happening is that some `render()` method is returning a valid result like `array($stuff, null)`. This is getting passed to JS as an array, which is implicitly `join()`'ing it into a string, adding a comma.
      
      Instead, make sure we render these to strings on the server side before shipping them to the client.
      
      Test Plan: No more comma on file previews.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D10974
      bc559886
    • Bob Trahan's avatar
      Maniphest - fix bug updating tasks with blocked relationships · a2126631
      Bob Trahan authored
      Summary: Ref T5604. Found this trying to open T5604 live. Basically this internal query needs the needSubscriberPHID set to true.
      
      Test Plan: doing it live
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T5604
      
      Differential Revision: https://secure.phabricator.com/D10975
      a2126631
    • Bob Trahan's avatar
      Transactions - make quotes work for older transactions · b718b429
      Bob Trahan authored
      Summary: Fixes T6731. I don't really understand the intent behind the two view classes here, but to get this to work I need to pass yet more data to the lower-level class.
      
      Test Plan: Viewed a task with many comments. Clicked "show older". Quoted everything I could. Verified for each quote that it quoted correctly, inlcuding linking to the prior transaction.
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6731
      
      Differential Revision: https://secure.phabricator.com/D10973
      b718b429
    • Chad Little's avatar
      Move 'Query' in Maniphest Search to be above the fold · f5301e8e
      Chad Little authored
      Summary: (Needed a clean branch). Moves the field up and renames to Query
      
      Test Plan: Visit Maniphest Search, see new field, test a query
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Differential Revision: https://secure.phabricator.com/D10971
      f5301e8e
    • Chad Little's avatar
      Properly ensure timeline items have a 1px spacer · e63b62fa
      Chad Little authored
      Summary: Some browsers, mobile, didn't show 1px space between objects (due to font renderings). This enforces the size for consistent cross-browser display.
      
      Test Plan: Test IE10
      
      Reviewers: btrahan, epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Differential Revision: https://secure.phabricator.com/D10970
      e63b62fa
    • epriestley's avatar
      Don't run Herald when applying inverse edge transactions · 5050389f
      epriestley authored
      Summary: Fixes T6727. Repro is: mention a task on another task, in a comment.
      
      The inverse edge editor applying the "alincoln mentioned this in <other task>" transaction doesn't have enough data to execute Herald rules.
      
      Just don't try to execute the rules, since they don't make much sesne from a product perspective and are tricky from a technical perspective.
      
      Test Plan: Commented on `T1` with `T2` in comment body and a Herald rule that examines subscribers.
      
      Reviewers: btrahan
      
      NOTE: Cowboy committing this since any task mention fatals.
      5050389f
    • Bob Trahan's avatar
      Maniphest - use subscribers framework properly · 7d968705
      Bob Trahan authored
      Summary: Fixes T5604. This should fix some random bugs, lets us move forward more easily, and all that good stuff about killing code debt.
      
      Test Plan:
      - Conduit method maniphest.createtask
        - verified creating user subscribed
        - verified subscription transaction
      - Conduit method maniphest.update
        - verified subscribers set as specified to ccPHIDs parameter
        - verified subscription transaction
      - Herald
        - verified herald rule to add subscriber worked
        - verified no subscribers removed accidentally
      - edit controller
        - test create and verify author gets added IFF they put themselves in subscribers control box
        - test update gets set to exactly what user enters
      - lipsum generator'd tasks work
      - bulk add subscribers works
      - bulk remove subscriber works
      - detail controller
        - added myself by leaving a comment
        - added another user via explicit action
        - added another user via implicit mention
      - task merge via search attach controller
      - mail reply handler
        - add subscriber via ./bin/mail receive-test
        - unsubscribe via ./bin/mail receive-test
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T5604
      
      Differential Revision: https://secure.phabricator.com/D10965
      7d968705
    • epriestley's avatar
      Implement more correct white-icon menu behavior · 3297bc2e
      epriestley authored
      Summary: I didn't get this quite right.
      
      Test Plan:
        - Clicked to open, saw white, then closed by:
          - Clicking document outside menu;
          - clicking menu icon again;
          - clicking a different menu icon.
        - In all three cases, got correct close + un-white behavior.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D10967
      3297bc2e
    • epriestley's avatar
      Make alert icons stay white while menus are open · eb9c3c66
      epriestley authored
      Summary: I think this is what you're after?
      
      Test Plan: clicky clicky
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Subscribers: epriestley
      
      Differential Revision: https://secure.phabricator.com/D10966
      eb9c3c66
  5. 10 Dec, 2014 10 commits
  6. 09 Dec, 2014 5 commits
  7. 08 Dec, 2014 5 commits
    • Chad Little's avatar
      Update message and notification icons to use fonts · 34fb98da
      Chad Little authored
      Summary: Cleans up spacing, updates to fonts instead of images. Fixed some mobile issues.
      
      Test Plan:
      Test with and without counts on desktop, tablet, mobile. Test layout in FF, Chrome, IE.
      
      {F246745}
      
      {F246746}
      
      Reviewers: btrahan, epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Differential Revision: https://secure.phabricator.com/D10948
      34fb98da
    • lkassianik's avatar
      Clicking the search icon in empty search field should link to advanced search · 45060183
      lkassianik authored
      Summary: Fixes T6664, clicking search icon in empty search field should link to advanced search
      
      Test Plan: navigate to home page, click search icon or click into search box and hit enter. Advanced search page should open.
      
      Reviewers: #blessed_reviewers, epriestley
      
      Reviewed By: #blessed_reviewers, epriestley
      
      Subscribers: Korvin, epriestley
      
      Maniphest Tasks: T6664
      
      Differential Revision: https://secure.phabricator.com/D10947
      45060183
    • epriestley's avatar
      When a worker task fails permanently, log the reason · 139c63bd
      epriestley authored
      Summary: Ref T6238. This makes debugging permanent task failures easier (we log reasoning for temporary failures already, just not permanent ones).
      
      Test Plan: Saw more useful permanent failure log.
      
      Reviewers: btrahan
      
      Reviewed By: btrahan
      
      Subscribers: epriestley
      
      Maniphest Tasks: T6238
      
      Differential Revision: https://secure.phabricator.com/D10945
      139c63bd
    • Joshua Spence's avatar
      Fix an undefined variable · 9e54e6e8
      Joshua Spence authored
      Summary:
      The `$timeline` variable is undefined. I was seeing the following error in the logs:
      
      ```
      EXCEPTION: (RuntimeException) Undefined variable: timeline at [<phutil>/src/error/PhutilErrorHandler.php:210]
         #0 PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<phabricator>/src/applications/harbormaster/controller/HarbormasterStepEditController.php:205]
         #1 HarbormasterStepEditController::processRequest() called at [<phabricator>/src/aphront/AphrontController.php:33]
         #2 AphrontController::handleRequest(AphrontRequest) called at [<phabricator>/webroot/index.php:103]
      ```
      
      Test Plan: Created a build step without a fatal error.
      
      Reviewers: btrahan, hach-que, epriestley, #blessed_reviewers
      
      Reviewed By: epriestley, #blessed_reviewers
      
      Subscribers: Korvin, epriestley
      
      Differential Revision: https://secure.phabricator.com/D10941
      9e54e6e8
    • Chad Little's avatar
      Remove text-rendering CSS rule · 46b77582
      Chad Little authored
      Summary: This rule is making text actually more difficult to read on Windows, as well as is broken on Win/Chrome, and evidentally has performance issues on mobile browsers.
      
      Test Plan: Turn it off, can still read web page
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin, epriestley
      
      Differential Revision: https://secure.phabricator.com/D10940
      46b77582