1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-11 17:32:41 +01:00
Commit graph

159 commits

Author SHA1 Message Date
epriestley
23a238b045 Remove "organization", "communication" and "miscellaneous" app groups
Summary:
Merge "Organization" and "Communication" into "Core". The split between these three was always tenuous, and this is easier to use and nicer looking on the new launcher.

Merge "Miscellaneous" into "Utilities" since they're basically the same thing.

Test Plan: Looked at app launcher.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9334
2014-05-29 15:25:26 -07:00
epriestley
81d95cf682 Make default view of "Applications" app a full-page launcher
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.

Open to feedback.

Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)

{F160052}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5176

Differential Revision: https://secure.phabricator.com/D9297
2014-05-29 12:17:54 -07:00
Chad Little
b2f3001ec4 Replace Sprite-Icons with FontAwesome
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.

Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9052
2014-05-12 10:08:32 -07:00
epriestley
50376aad04 Require multiple auth factors to establish web sessions
Summary:
Ref T4398. This prompts users for multi-factor auth on login.

Roughly, this introduces the idea of "partial" sessions, which we haven't finished constructing yet. In practice, this means the session has made it through primary auth but not through multi-factor auth. Add a workflow for bringing a partial session up to a full one.

Test Plan:
  - Used Conduit.
  - Logged in as multi-factor user.
  - Logged in as no-factor user.
  - Tried to do non-login-things with a partial session.
  - Reviewed account activity logs.

{F149295}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8922
2014-05-01 10:23:02 -07:00
epriestley
730cb65913 Add aural label for "application" breadcrumb in crumbs
Summary: Ref T4843. This is a purely-visual link; label it with the application name.

Test Plan: {F149583}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4843

Differential Revision: https://secure.phabricator.com/D8927
2014-05-01 08:55:45 -07:00
epriestley
d8713f6f0b Make dialogs a little easier to use
Summary:
  - Dialog pages currently have no titles or crumbs, and look shoddy. Add titles and crumbs.
  - Dialog titles aren't always great for crumbs, add an optional "short title" for crumbs.
  - `AphrontDialogResponse` is pure boilerplate. Allow controllers to just return a `DialogView` instead and get the same effect.
  - Building dialogs requires a bit of boilerplate, and we generally construct them with no explicit `"action"`, which has some issues with T4593. Provide a convenience method to set the viewer and get a reasonable, explict submit URI.

Test Plan:
  - Viewed dialog on its own.
  - Viewed dialog as a dialog.

{F132353}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8577
2014-03-21 14:40:05 -07:00
epriestley
b3b0ef3647 Use viwer-based checks for application visibility when rendering home elements
Summary:
Fixes T4619. Currently, even if a viewer can't see Maniphest, they'll still see empty panels on the home page. These panels will always be empty so there's no real policy violation, but it's confusing.

Longer term, dashboards should fix this.

Test Plan: Viewed home page with a user with and without permissions on the apps.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4619

Differential Revision: https://secure.phabricator.com/D8545
2014-03-15 11:28:02 -07:00
epriestley
7176240717 Whitelist controllers which can receive a 'code' parameter
Summary:
Ref T4593. There are a variety of clever attacks against OAuth which involve changing the redirect URI to some other URI on the same domain which exhibits unexpected behavior in response to an OAuth request. The best approach to dealing with this is for providers to lock to a specific path and refuse to redirect elsewhere, but not all providers do this.

We haven't had any specific issues related to this, but the anchor issue in T4593 was only a step away.

To mitigate this in general, we can reject the OAuth2 `'code'` parameter on //every// page by default, and then whitelist it on the tiny number of controllers which should be able to receive it.

This is very coarse, kind of overkill, and has some fallout (we can't use `'code'` as a normal parameter in the application), but I think it's relatively well-contained and seems reasonable. A better approach might be to whitelist parameters on every controller (i.e., have each controller specify the parameters it can receive), but that would be a ton of work and probably cause a lot of false positives for a long time.

Since we don't use `'code'` normally anywhere (as far as I can tell), the coarseness of this approach seems reasonable.

Test Plan:
  - Logged in with OAuth.
  - Hit any other page with `?code=...` in the URL, got an exception.
  - Grepped for `'code'` and `"code"`, and examined each use to see if it was impacted.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4593

Differential Revision: https://secure.phabricator.com/D8499
2014-03-12 11:30:04 -07:00
Joshua Spence
e11adc4ad7 Added some additional assertion methods.
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
2014-03-08 19:16:21 -08:00
epriestley
270929dd6b Include Source Sans on-demand with Celerity
Summary:
Unwinds the mess I made in D8422 / D8430:

  - Remove `'fonts'`, since individual fonts can be included via Celerity now.
  - Include Source Sans from the local source when a document uses it as a fontkit.

Test Plan: Browsed Diviner, saw Source Sans.

Reviewers: chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D8431
2014-03-06 11:39:48 -08:00
epriestley
270916a26e Support WOFF files in Celerity and add Source Sans Pro
Summary:
  - Allow Celerity to map and serve WOFF files.
  - Add Source Sans Pro, Source Sans Pro Bold, and the corresponding LICENSE.
  - Add a `font-source-sans-pro` resource for the font.

Test Plan:
  - Changed body `font-face` to `'Source Sans Pro'`.
  - Added `require_celerity_resource('font-source-sans-pro')` in StandardPageView.

Works in Firefox/Chrome/Safari, at least:

{F123296}

{F123297}

{F123298}

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Differential Revision: https://secure.phabricator.com/D8430
2014-03-06 11:28:24 -08:00
Joshua Spence
6270114767 Various linter fixes.
Summary:
- Removed trailing newlines.
- Added newline at EOF.
- Removed leading newlines.
- Trimmed trailing whitespace.
- Spelling fix.
- Added newline at EOF

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8344
2014-02-26 12:44:58 -08:00
epriestley
ba81aa1dfe Remove quick create buttons from application launcher
Summary: Ref T3623. These are obsoleted by the global quick-create menu, so we can simplify the app launcher.

Test Plan: Looked at app launcher, grepped for everything.

Reviewers: chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T3623

Differential Revision: https://secure.phabricator.com/D8104
2014-01-29 17:23:50 -08:00
epriestley
3c71976f86 Make the quick create menu more or less work correctly
Summary:
Ref T3623. I'm sure I didn't get the margins / drop shadow quite right, but this looks and works reasonably well:

{F105637}

Test Plan: Clicked stuff to quick create.

Reviewers: chad, btrahan

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T3623

Differential Revision: https://secure.phabricator.com/D8089
2014-01-28 20:19:20 -08:00
epriestley
049fb2018b Add very basic "quick create" menu
Summary:
Ref T3623. This is like a pre-v0, in that it doesn't have a dropdown yet.

Clicking the button takes you to a page which can serve as a right click / mobile / edit target in the long run, but is obviously not great for desktop use. I'll add the dropdown in the next iteration.

Test Plan: {F105631}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T3623

Differential Revision: https://secure.phabricator.com/D8088
2014-01-28 20:18:01 -08:00
epriestley
11786fb1cc Don't try to set anonymous session cookie on CDN/file domain
Summary:
Ref T2380. If an install has a CDN domain configured, but does not list it as an alternate domain (which is standard/correct, but not incredibly common, see T2380), we'll currently try to set anonymous cookies on it. These will correctly fail security rules.

Instead, don't try to set these cookies.

I missed this in testing yesterday because I have a file domain, but I also have it configured as an alternate domain, which allows cookies to be set. Generally, domain management is due for some refactoring.

Test Plan: Set file domain but not as an alternate, logged out, nuked file domain cookies, reloaded page. No error after patch.

Reviewers: btrahan, csilvers

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2380

Differential Revision: https://secure.phabricator.com/D8057
2014-01-24 12:29:03 -08:00
epriestley
5b1d9c935a After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:

  - User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
  - User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).

To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.

The comment in the body text describes this in more detail.

This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.

Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3793

Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 14:16:08 -08:00
epriestley
f9ac534f25 Support CSRF for logged-out users
Summary: Fixes T4339. If you're anonymous, we use a digest of your session key to generate a CSRF token. Otherwise, everything works normally.

Test Plan: Logged out, logged in, tweaked CSRF in forms -- I'll add some inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8046
2014-01-23 14:03:54 -08:00
epriestley
69ddb0ced6 Issue "anonymous" sessions for logged-out users
Summary:
Ref T4339. Ref T4310. Currently, sessions look like `"afad85d675fda87a4fadd54"`, and are only issued for logged-in users. To support logged-out CSRF and (eventually) external user sessions, I made two small changes:

  - First, sessions now have a "kind", which is indicated by a prefix, like `"A/ab987asdcas7dca"`. This mostly allows us to issue session queries more efficiently: we don't have to issue a query at all for anonymous sessions, and can join the correct table for user and external sessions and save a query. Generally, this gives us more debugging information and more opportunity to recover from issues in a user-friendly way, as with the "invalid session" error in this diff.
  - Secondly, if you load a page and don't have a session, we give you an anonymous session. This is just a secret with no special significance.

This does not implement CSRF yet, but gives us a client secret we can use to implement it.

Test Plan:
  - Logged in.
  - Logged out.
  - Browsed around.
  - Logged in again.
  - Went through link/register.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T4339

Differential Revision: https://secure.phabricator.com/D8043
2014-01-23 14:03:22 -08:00
epriestley
0727418023 Consolidate use of magical cookie name strings
Summary: Ref T4339. We have more magical cookie names than we should, move them all to a central location.

Test Plan: Registered, logged in, linked account, logged out. See inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8041
2014-01-23 14:01:35 -08:00
epriestley
d392a8f157 Replace "web" and "conduit" magic session strings with constants
Summary: Ref T4310. Ref T3720. We use bare strings to refer to session types in several places right now; use constants instead.

Test Plan: grep; logged out; logged in; ran Conduit commands.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7963
2014-01-14 13:22:34 -08:00
epriestley
eef314b701 Separate session management from PhabricatorUser
Summary: Ref T4310. Ref T3720. Session operations are currently part of PhabricatorUser. This is more tightly coupled than needbe, and makes it difficult to establish login sessions for non-users. Move all the session management code to a `SessionEngine`.

Test Plan:
  - Viewed sessions.
  - Regenerated Conduit certificate.
  - Verified Conduit sessions were destroyed.
  - Logged out.
  - Logged in.
  - Ran conduit commands.
  - Viewed sessions again.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4310, T3720

Differential Revision: https://secure.phabricator.com/D7962
2014-01-14 13:22:27 -08:00
epriestley
3d9e328fb3 Add an "active login sessions" table to Settings
Summary: Ref T4310. Ref T3720. Partly, this makes it easier for users to understand login sessions. Partly, it makes it easier for me to make changes to login sessions for T4310 / T3720 without messing anything up.

Test Plan: {F101512}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3720, T4310

Differential Revision: https://secure.phabricator.com/D7954
2014-01-14 11:05:45 -08:00
epriestley
31b6f69ff7 Allow CelerityResourceResponse to hold resources from multiple maps
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.

Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.

Test Plan:
  - Reloaded pages.
  - Browsed around Differential.

Reviewers: btrahan, hach-que

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4222

Differential Revision: https://secure.phabricator.com/D7876
2014-01-02 11:59:35 -08:00
epriestley
0fa411083f Show an "approval queue" item on the home page for admins, and sort out menu item visibility
Summary:
  - If you're an administrator and there are users waiting for approval, show a count on the home page.
  - Sort out the `isUserActivated()` access check.
  - Hide all the menu widgets except "Logout" for disabled and unapproved users.
  - Add a "Log In" item.
  - Add a bunch of unit tests.

Test Plan: Ran unit tests, clicked around as unapproved/approved/logged-in/logged-out users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, chad

Differential Revision: https://secure.phabricator.com/D7574
2013-11-13 11:24:38 -08:00
epriestley
c8320923c4 Implement most of the administrative UI for approval queues
Summary:
Nothing fancy here, just:

  - UI to show users needing approval.
  - "Approve" and "Disable" actions.
  - Send "Approved" email on approve.
  - "Approve" edit + log operations.
  - "Wait for Approval" state for users who need approval.

There's still no natural way for users to end up not-approved -- you have to write directly to the database.

Test Plan: See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7573
2013-11-13 11:24:18 -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
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
epriestley
d5f41ef70e Return HTTP 500, not HTTP 200, on exception pages
Summary: Ref T4064. The response code here isn't normally relevant, but we can hit these via `git clone http://../`, etc., and it's clearly more correct to use HTTP 500.

Test Plan: Added a fake `throw new Exception()` and verified I got an HTTP 500 response.

Reviewers: jamesr, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4064

Differential Revision: https://secure.phabricator.com/D7507
2013-11-05 15:25:12 -08:00
epriestley
c7f23f522a Accept and route VCS HTTP requests
Summary:
Mostly ripped from D7391, with some changes:

  - Serve repositories at `/diffusion/X/`, with no special `/git/` or `/serve/` URI component.
    - This requires a little bit of magic, but I got the magic working for Git, Mercurial and SVN, and it seems reasonable.
    - I think having one URI for everything will make it easier for users to understand.
    - One downside is that git will clone into `X` by default, but I think that's not a big deal, and we can work around that in the future easily enough.
  - Accept HTTP requests for Git, SVN and Mercurial repositories.
  - Auth logic is a little different in order to be more consistent with how other things work.
  - Instead of AphrontBasicAuthResponse, added "VCSResponse". Mercurial can print strings we send it on the CLI if we're careful, so support that. I did a fair amount of digging and didn't have any luck with git or svn.
  - Commands we don't know about are assumed to require "Push" capability by default.

No actual VCS data going over the wire yet.

Test Plan:
Ran a bunch of stuff like this:

  $ hg clone http://local.aphront.com:8080/diffusion/P/
  abort: HTTP Error 403: This repository is not available over HTTP.

...and got pretty reasonable-seeming errors in all cases. All this can do is produce errors for now.

Reviewers: hach-que, btrahan

Reviewed By: hach-que

CC: aran

Maniphest Tasks: T2230

Differential Revision: https://secure.phabricator.com/D7417
2013-10-29 15:32:40 -07:00
epriestley
314673f4f6 Fix an issue with rendering PHID lists containing null in Maniphest
Summary: See IRC. Someone got a `null` in CCPHIDs somehow. Moving to subscriptions should prevent this, but paper over it for now.

Test Plan: Will have @dctrwatson check.

Reviewers: btrahan, dctrwatson

Reviewed By: btrahan

CC: dctrwatson, aran

Differential Revision: https://secure.phabricator.com/D7330
2013-10-16 12:46:34 -07:00
epriestley
8c1c6fec5a Modernize policies in Paste and Macro
Summary:
Ref T603. Fixes T2823. This updates Paste and Macro.

  - **Paste**
    - Added default view policy.
    - I didn't add a "create" policy, since I can't come up with any realistic scenario where you'd give users access to pastes but not let them create them.
  - **Macro**
    - Added a "manage" policy, which covers creating and editing macros. This lets an install only allow "People With An Approved Sense of Humor" or whatever to create macros.
    - Removed the "edit" policy, since giving individual users access to specific macros doesn't make much sense to me.
    - Changed the view policy to the "most public" policy the install allows.
    - Added view policy information to the header.

Also fix a couple of minor things in Maniphest.

Test Plan:
  - Set Paste policy, created pastes via web and Conduit, saw they got the right default policies.
  - Set Macro policy, tried to create/edit macros with valid and unauthorized users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2823, T603

Differential Revision: https://secure.phabricator.com/D7317
2013-10-16 10:35:52 -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
c8127edfe9 Tighten up some policy interactions in Herald
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
2013-10-04 15:15:48 -07:00
epriestley
0883ea6977 Move "unlisted" apps to Query, use Query for app preferences
Summary:
  - Demagic "unlisted" apps.
  - Respect policies in application visibility settings.

Test Plan: Viewed applications, settings.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7212
2013-10-04 06:46:47 -07:00
epriestley
a6c4117ec4 Fix controller-level access rules
Summary:
Ref T603. I had to partially revert this earlier because it accidentally blocked access to Conduit and File data for installs without "policy.allow-public", since the applications are available to "all users" but some endpoints actually need to be available even when not logged in.

This readjusts the gating in the controller to properly apply application visibility restrictions, and then adds a giant pile of unit test coverage to make sure it sticks and all the weird cases are covered.

Test Plan:
  - Added and executed unit tests.
  - Executed most of the tests manually, by using logged in / admin / public / disabled users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7211
2013-10-03 19:05:47 -07:00
epriestley
1296c3d347 Fix two excessively aggressive policy checks
Summary:

  - Some applications need public access regardless of policy configuration.
  - The file data endpoint should ignore policies.
2013-10-03 14:38:08 -07:00
epriestley
c830461b00 Allow application policies to be edited
Summary:
Ref T603. Enables:

  - Application policies can be edited.
  - Applications can define custom policies (this will be used for setting defaults, like "what is the default visibiltiy of new tasks", and meta-policies, like "who can create a task?").

Test Plan: Edited application policies. A future diff does more with custom policies.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7205
2013-10-03 12:40:08 -07:00
epriestley
0d83e1d66f If a user can't see an application, prevent them from using its controllers
Summary:
Ref T603. Broadly, this allows you to implement a policy like "Only users in Engineering can use Differential."

This isn't complete, and there will be a long tail of special cases to deal with. Some examples:

  - If you can't use Differential, should you still be able to attach/detach revisions from tasks?
    - You currently will be able to.
    - This actually seems pretty reasonable.
    - But in other cases it might not be: the "send user a message" action should probably require access to Conpherence.
  - If you can't use Differential, should you still be able to see feed stories about it?
    - You currently will be able to, if you can see the revisions.
    - This seems not-so-reasonable and we should probably lock it down.
  - If you can't use Differential, can users CC you on revisions?
    - Currently, they can, and you can't do anything about it.
    - Probably they shouldn't be able to? This seems challenging to explain in the UI.
  - If you can't use Differential, can you write a Herald rule against it?
    - You currently will be able to.
    - Seems like you obviously shouldn't be able to.
    - I think this is a general issue right now (you can still write Differential herald rules even if you uninstall the application, I believe).

There are probably a few more things I haven't thought of. However, there are a finite number of these things and I suspect there aren't //too/ many more than this -- I can't come up with like 100 of them, and half of the ones above have easy fixes.

Despite the rough edges, I think this accomplishes 95% of what installs expect from it.

Test Plan: Restricted Differential and saw it vanish from the home page.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7203
2013-10-03 12:39:41 -07:00
epriestley
f75c13b987 Use ApplicationSearch in Applications application
Summary: Ref T603. OMG SO META

Test Plan: See screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7197
2013-10-02 13:13:07 -07:00
epriestley
4b39cc321b Fix application order, curing "Log Out" of wanderlust
Summary:
Fixes T3894. The "Log Out" icon has moved away from its rightmost position in the menubar.

In rP2e5ac12, I added a "Policy" application. This was the root cause.

The reordering logic (below) is slightly wrong. The `array_select_keys()` call is actually using the //strings// (like "Admnistration") to select the groups, not the correct constants (like "admin"). Use the constants instead and get the expected group ordering.

Test Plan: Loaded page, "Log Out" is in the rightmost position.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T3894

Differential Revision: https://secure.phabricator.com/D7177
2013-09-30 09:38:04 -07:00
Bob Trahan
b902005bed Kill PhabricatorObjectDataHandle
Summary: Ref T603. Killing this class is cool because the classes that replace it are policy-aware. Tried to keep my wits about me as I did this and fixed a few random things along the way. (Ones I remember right now are pulling a query outside of a foreach loop in Releeph and fixing the text in UIExample to note that the ace of hearts if "a powerful" card and not the "most powerful" card (Q of spades gets that honor IMO))

Test Plan: tested the first few changes (execute, executeOne X handle, object) then got real mechanical / careful with the other changes.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran, FacebookPOC

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D6941
2013-09-11 12:27:28 -07:00
Chad Little
9ef0ea91c4 Remove dust pattern for common bg color.
Summary: Depends on D6769, removes 'dust' and uses a similar color background.

Test Plan: Review colors in sandbox.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D6772
2013-08-19 18:15:52 -07:00
epriestley
1834584e98 Provide contextual help on auth provider configuration
Summary:
Ref T1536.

  - Move all the provider-specific help into contextual help in Auth.
  - This provides help much more contextually, and we can just tell the user the right values to use to configure things.
  - Rewrite account/registration help to reflect the newer state of the word.
  - Also clean up a few other loose ends.

Test Plan: {F46937}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6247
2013-06-20 11:18:48 -07:00
epriestley
73c2c1d2e6 Send old login code to the bottom of the sea
Summary:
Ref T1536. This is extremely reachable and changes the login code to the new stuff.

Notes:

  - I've hard-disabled password registration since I want installs to explicitly flip it on via config if they want it. New installs will get it by default in the future, but old installs shouldn't have their auth options change.
  - Google doesn't let us change the redirect URI, so keep the old one working.
  - We need to keep a bit of LDAP around for now for LDAP import.
  - **Facebook:** This causes substantive changes in what login code is executed.

Test Plan:
  - Logged in / logged out / registered, hit new flows.
  - Logged in with google.
  - Verified no password registration by default.

Reviewers: btrahan, chad

Reviewed By: chad

CC: wez, nh, aran, mbishopim3

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6222
2013-06-19 01:33:27 -07:00
epriestley
fe29b27582 Fix several issues with "Logout"
Summary:
Ref T1536.

  - When we render a dialog on a page by itself, put it on a dust background.
  - Currently, we render "Logout" in two different places. Stop doing that.
  - Make sure the surviving one has workflow so we get a modal ajax dialog if possible.

Test Plan: {F46731}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6226
2013-06-18 11:05:28 -07:00
epriestley
7cf7f36452 Restyle username/password login for reduced hideousness
Summary: Ref T1536. Error state is a bit gross but we need to sort that out in general.

Test Plan:
{F46549}

{F46550}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6208
2013-06-16 16:31:14 -07:00
Jakub Vrana
32f91557f8 Store hash of session key
Summary:
This prevents security by obscurity.
If I have read-only access to the database then I can pretend to be any logged-in user.

I've used `PhabricatorHash::digest()` (even though we don't need salt as the hashed string is random) to be compatible with user log.

Test Plan:
Applied patch.
Verified I'm still logged in.
Logged out.
Logged in.

  $ arc tasks

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6080
2013-05-30 17:30:06 -07:00
Bob Trahan
4295de508f Conpherence - add createthread method
Summary: Ref T3166. I moved the create logic into a static method in the editor class to keep things tidy.

Test Plan: created a conpherence from UI. purdy. tried errors and got UI to show "required". for conduit, created a thread with all the bells and whistles and it worked. verified i got proper exceptions with bum conduit calls

Reviewers: epriestley

Reviewed By: epriestley

CC: chad, aran, Korvin

Maniphest Tasks: T3166

Differential Revision: https://secure.phabricator.com/D6083
2013-05-30 16:37:51 -07:00