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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Fixes T3218.
- Currently, Paste pages don't clear notifications about the paste (notably, token notifications).
- Currently, Paste pages don't show tooltips on tokens.
- `buildApplicationPage()` stopped respecting `pageObjects` (which controls whether "this page has been updated" is shown). Restore that.
- Make `pageObjects` imply "clear notifications on this stuff".
Test Plan: Viewed a tokened Paste. Verified it cleared the notification and hovering over a token showed a tip.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3218
Differential Revision: https://secure.phabricator.com/D5971
Summary:
Normalize the unit test environment by installing all applications.
The immediate issue this fixes is that `testDropUnknownSenderMail` depends on Maniphest being installed. Some possible fixes are:
# Don't rely on the Maniphest mail receiver for the test (e.g., write a stub/dummy/mock receiver).
# Explicitly make sure Maniphest is installed before running the test.
# Normalize the test environment to install all applications.
I don't like (1) much because it turns a pretty good 10 line test into a bunch of stub classes or mock junk. I'll do it if we have more uses after a few more diffs, but so far running these tests against real code hasn't created a dependency mess and we get more coverage.
I don't like (2) much because I think requiring tests to do this will do more harm than good. The number of issues we'll hypothetically uncover by exposing unrealized application interdependencies is probably very small or maybe zero, and they're probably all trivial. But tests with an undeclared but implicit dependency on an application (e.g., Differential tests depend on Differential) are common.
So here's (3), which I think is reasonable.
I also simplified some of this code a little bit, and moved the Application object cache one level down (this was sort of a bug -- installation status is variant across requests).
Test Plan: Added unit test.
Reviewers: wez, btrahan
Reviewed By: wez
CC: aran
Differential Revision: https://secure.phabricator.com/D5938
Summary:
Ref T1205. Continuation of D5915.
Currently, `PhabricatorMetaMTAReceivedMail` has //all// the logic for routing mail. In particular:
- New mail receivers in applications must edit it.
- Mail receivers don't drop out when applications are uninstalled.
Applications have some logic in subclasses of `PhabricatorMailReplyHandler`, but this class is a bit of a mess. It is also heavily based on the assumption that mail receivers are objects (like revisions), but this is not true in at least two cases today (creating new tasks with `bugs@`, creating a new Conpherence thread) and likely other cases in the future (e.g., revision-by-mail).
Move this logic into a new `PhabricatorMailReceiver` classtree. This is similar to `PhabricatorMailReplyHandler` but a bit cleaner and more general. I plan to heavily reduce the responsibilities of `PhabricatorMailReplyHandler` or possibly eliminate it entirely.
For now, the new classtree doesn't do much of interest. The only behavioral change this diff causes is that Phabricator will now reject mail to an application when that application is uninstalled.
I also moved all the `ReplyHandler` classes into `mail/` directories in their respective applications.
Test Plan: Unit tests, used receive test to route mail to various objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: Afaque_Hussain, edward, aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5922
Summary:
Also join concepts of installed and enabled applications.
Also respect uninstalled Maniphest where disabled Maniphest was checked.
Test Plan:
Visited T1, D1.
Uninstalled Maniphest then visited T1, D1.
Disabled Maniphest then visited T1.
Visited /config/edit/maniphest.enabled/.
Reviewers: epriestley, Afaque_Hussain, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5602
Summary:
Currently, `isInstalled()` and `getAllInstalledApplications()` are inconsistent:
- `isInstalled()` returns true for beta apps, even if `phabricator.show-beta-applications` is false.
- `getAllInstalledApplications()` omits beta apps if `phabricator.show-beta-applications` is false.
Making the beta config control installs (not just homepage visibility) makes far more sense as we roll out more thorough application integrations.
Make `isInstalled()` respect beta, and clean up some callsites.
D5602 builds on this.
Test Plan: Installed/uninstalled beta apps, verified Conpherence menu/panel and other application integrations dropped out of the UI.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D5603
