Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Added also a small inline documentation to better leave what I find.
Closes T15358
Test Plan:
Applied this change change and way more applications' Configure pages finally rendered in web browser.
For example, `/applications/view/PhabricatorAuditApplication/` is now correctly rendered.
(However, some pages expose followup exceptions to be handled in separate tasks.)
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15358
Differential Revision: https://we.phorge.it/D25197
Summary:
Fix numerous PHP 8.1 "strlen(null)" exceptions which block rendering the initial Account
Creation page in a fresh Phorge installation.
The strlen() was used in Phabricator to check if a generic value was a non-empty string.
For this reason, Phorge adopts phutil_nonempty_string() that checks that.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If your phutil_nonempty_string() throws an exception, just
report it to Phorge to evaluate and fix together that specific corner case.
Closes T15279
Test Plan: After these code changes the account creation page got displayed (though without CSS and JS).
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15279
Differential Revision: https://we.phorge.it/D25137
Summary: Ref T13682. Allow users to manually attach files which are referenced (but not attached) via the UI.
Test Plan: Reference files via `{F...}`, then attached them via the UI workflow.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21837
Summary: Ref T13658.
Test Plan:
This is non-exhaustive.
- Started a JIRA auth provider creation workflow.
- Same for LDAP.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21779
Summary:
Ref T13658. This just scrubs some of the simple references from the codebase.
Most of what's left is in documentation which won't be relevant for a fork and/or which I need to separately revise (or more-or-less delete) at some point anyway.
I removed the "install RHEL" and "install Ubuntu" scripts outright since I don't have any reasonable way to test them and don't plan to maintain them.
Test Plan: Grepped for "phacility", "epriestley"; ran unit tests.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21678
Summary:
Fixes T12919. Fixes T13636. Prior to this change, some well-known resource paths don't route on sites like ResourceSite.
- `/robots.txt`: Make it route on ResourceSite and just deny the whole site.
- `/favicon.ico`: Make it route on ResourceSite.
- `/status/`: Make it route on ResourceSite.
- 404: Make it render a 404 on ResourceSite.
Test Plan:
- Visited all URIs on ResourceSite, got sensible responses.
- Visited all URIs on main site.
- Visited 404 while logged out, got login page.
Maniphest Tasks: T13636, T12919
Differential Revision: https://secure.phabricator.com/D21606
Summary:
Ref T13588. This has never been meaningful, but a "final private" method is specifically forbidden in PHP8.
Remove meaningless "final" from these methods, per new lint checks.
Test Plan: Ran `arc lint --everything` to identify affected methods, then `... | xargs -n1 arc lint --apply-patches`.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21540
Summary:
Depends on D20739. Ref T13366. Slightly modularize/update components of order views, and make orders viewable from either an account context (existing view) or an external context (new view).
The new view is generally simpler so this mostly just reorganizes existing code.
Test Plan: Viewed orders as an account owner and an external user.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20740
Summary:
When a TOS-like Legalpad document is marked "Require this document to use Phabricator", the login prompt shows a "Manage" button, but that button doesn't work.
When we're presenting a document as a session gate, don't show "Manage".
Test Plan: Viewed a required document during a session gate (no "Manage" button) and normally (saw "Manage" button).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20312
Summary:
Ref T4369. Ref T12297. Ref T13242. Ref PHI1010. I want to take a quick look at `transaction.search` and see if there's anything quick and obvious we can do to improve performance.
On `secure`, the `__profile__` flag does not survive POST like it's supposed to: when you profile a page and then submit a form on the page, the result is supposed to be profiled. The intent is to make it easier to profile Conduit calls.
I believe this is because we're hooking the profiler, then rebuilding POST data a little later -- so `$_POST['__profile__']` isn't set yet when the profiler checks.
Move the POST rebuild a little earlier to fix this.
Also, remove the very ancient "aphront.default-application-configuration-class". I believe this was only used by Facebook to do CIDR checks against corpnet or something like that. It is poorly named and long-obsolete now, and `AphrontSite` does everything we might reasonably have wanted it to do.
Test Plan: Poked around locally without any issues. Will check if this fixes the issue on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242, T12297, T4369
Differential Revision: https://secure.phabricator.com/D20046
Summary:
Ref T12509.
- Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
- Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
- Extract the CSRF logic from `PhabricatorUser` to a standalone engine.
I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.
For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.
It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).
This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.
Test Plan:
- As a logged-in user, submitted forms normally (worked).
- As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
- As a logged-out user, hit the success and error cases.
- Visually inspected tokens for correct format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19946
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Depends on D19914. Ref T11351. Some of the Phoilo rabbit holes go very deep.
`PhabricatorApplicationTransactionInterface` currently requires you to implement `willRenderTimeline()`. Almost every object just implements this as `return $timeline`; only Pholio, Diffusion, and Differential specialize it. In all cases, they are specializing it mostly to render inline comments.
The actual implementations are a bit of a weird mess and the way the data is threaded through the call stack is weird and not very modern.
Try to clean this up:
- Stop requiring `willRenderTimeline()` to be implemented.
- Stop requiring `getApplicationTransactionViewObject()` to be implemented (only the three above, plus Legalpad, implement this, and Legalpad's implementation is a no-op). These two methods are inherently pretty coupled for almost any reasonable thing you might want to do with the timeline.
- Simplify the handling of "renderdata" and call it "View Data". This is additional information about the current view of the transaction timeline that is required to render it correctly. This is only used in Differential, to decide if we can link an inline comment to an anchor on the same page or should link it to another page. We could perhaps do this on the client instead, but having this data doesn't seem inherently bad to me.
- If objects want to customize timeline rendering, they now implement `PhabricatorTimelineInterface` and provide a `TimelineEngine` which gets a nice formal stack.
This leaves a lot of empty `willRenderTimeline()` implementations hanging around. I'll remove these in the next change, it's just going to be deleting a couple dozen copies of an identical empty method implementation.
Test Plan:
- Viewed audits, revisions, and mocks with inline comments.
- Used "Show Older" to page a revision back in history (this is relevant for "View Data").
- Grepped for symbols: willRenderTimeline, getApplicationTransactionViewObject, Legalpad classes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19918
Summary:
Ref T13216. See PHI980. Currently, each application in {nav Applications > X > Configure} has a "Can Configure Application" permission which is hard-coded to "Administrators".
There's no technical reason for this, there just hasn't been a great use case for unlocking it. I think when I originally wrote it our protections against locking yourself out of things weren't that great (i.e., it was easier to set the policy to something that prevented you from editing it after the new policy took effect). Our protections are better now.
The major goal here is to let installs open up Custom Forms for given applications (mostly Maniphest) to more users, but the other options mostly go hand-in-hand with that.
Also, in developer mode, include stack traces for policy exceptions. This makes debugging weird stuff (like the indirect Config application errors here) easier.
Test Plan:
- Granted "Can Configure Application" for Maniphest to all users.
- Edited custom forms as a non-administrator.
- Configured Maniphest as a non-administrator.
- Installed/uninstalled Maniphest as a non-administrator.
- Tried to lock myself out (got an error message).
{F6015721}
Reviewers: amckinley, joshuaspence
Reviewed By: joshuaspence
Subscribers: joshuaspence
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19822
Summary: This reverts D18524. See that revision for discussion.
Test Plan: Viewed home menu, saw application names as menu items.
Differential Revision: https://secure.phabricator.com/D19308
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary:
Depends on D18934. Ref T13046. Add support for the new export flow to a second application.
My goal here is mostly just to make sure that this is general enough to work in more than one place, and exporting user accounts seems plausible as a useful feature, although we do see occasional requests for this feature exactly (like <https://discourse.phabricator-community.org/t/users-export-to-csv/968>).
The exported data may not truly be useful for much (no disabled/admin/verified/MFA flags, no external account data, no email addresses for policy reasons) but we can expand it as use cases arise.
Test Plan: Exported user accounts in several formats.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18935
Summary:
Depends on D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.
Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.
In the future, this can replace the existing "Export to Excel" feature in Maniphest.
For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.
For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).
Test Plan: Downloaded pull logs in JSON format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046, T5954
Differential Revision: https://secure.phabricator.com/D18919
Summary:
Depends on D18805. Ref T13025. Fixes T10268.
Instead of using a list of IDs for the bulk editor, power it with SearchEngine queries. This gives us the full power of SearchEngine and lets us use a query key instead of a list of 20,000 IDs to avoid issues with URL lengths.
Also, split it into a base `BulkEngine` and per-application subclasses. This moves us toward T10005 and universal support for bulk operations.
Also:
- Renames most of "batch" to "bulk": we're curently inconsitent about this, I like "bulk" better since I think it's more clear if you don't regularly interact with `.bat` files, and newer stuff mostly uses "bulk".
- When objects in the result set can't be edited because you don't have permission, show the status more clearly.
This probably breaks some stuff a bit since I refactored so heavily, but it seems mostly OK from poking around. I'll clean up anything I missed in followups to deal with remaining items on T13025.
Test Plan:
{F5302300}
- Bulk edited from Maniphest.
- Bulk edited from a workboard (no more giant `?ids=....` in the URL).
- Hit most of the error conditions, I think?
- Clicked the "Cancel" button.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13025, T10268
Differential Revision: https://secure.phabricator.com/D18806
See PHI238. When an install uninstalls "Legalpad", we were incorrectly failing
to mark sessions as "Signed All Required Documents" by bailing early.
Test Plan: Uninstalled Legalpad, logged in.
Summary:
Depends on D18790. Ref T13024. Fixes T8335. Currently, "unapproved" and "disabled" users are bundled together. This prevents users from completing some registration steps (verification, legalpad documents, MFA enrollment) before approval.
Separate approval out and move it to the end so users can do all the required enrollment stuff on their end before we roadblock them.
Test Plan: Required approval, email verification, signatures, and MFA. Registered an account. Verified email, signed documents, enrolled in MFA, and then got prompted to wait for approval.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024, T8335
Differential Revision: https://secure.phabricator.com/D18791
Summary:
Depends on D18789. Ref T13024. See PHI223. Currently, if `security.require-multi-factor-auth` and Legalpad "Signature Required" documents are //both// set, it's not possible to survive account registration, since MFA is requiried to sign and signatures are required to add MFA.
Instead, check for signatures before requiring MFA enrollment. This makes logical sense, since it's silly to add MFA if you don't agree to a Terms of Service or whatever.
(Note that if you already have MFA, we prompt for that first, before either of these steps, which also makes sense.)
Test Plan: Configured `security.require-multi-factor-auth`. Added a signature-required document. Loaded a page as a new user. Went through signature workflow, then through the MFA enrollment workflow.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18790
Summary: Depends on D18788. Ref T13024. Currently, we prompt users to sign from newest to oldest. This seems less intuitive than oldest to newest.
Test Plan: Dumped document order, saw it swap to oldest-first.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18789
Summary: Depends on D18786. Ref T13024. I'm going to change the order this occurs in, but move it to a separate method and clean it up a little first.
Test Plan: Added a new document as required, reloaded, signed it, got logged into a session.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18788
Summary:
Depends on D18702. Ref T13008. This replaces the old hard-coded single rate limit with multiple flexible limits, and defines two types of limits:
- Rate: reject requests if a client has completed too many requests recently.
- Connection: reject requests if a client has too many more connections than disconnections recently.
The connection limit adds +1 to the score for each connection, then adds -1 for each disconnection. So the overall number is how many open connections they have, at least approximately.
Supporting multiple limits will let us do limiting by Hostname and by remote address (e.g., a specific IP can't exceed a low limit, and all requests to a hostname can't exceed a higher limit).
Configuring the new limits looks something like this:
```
PhabricatorStartup::addRateLimit(new PhabricatorClientRateLimit())
->setLimitKey('rate')
->setClientKey($_SERVER['REMOTE_ADDR'])
->setLimit(5);
PhabricatorStartup::addRateLimit(new PhabricatorClientConnectionLimit())
->setLimitKey('conn')
->setClientKey($_SERVER['REMOTE_ADDR'])
->setLimit(2);
```
Test Plan:
- Configured limits as above.
- Made a lot of requests, got cut off by the rate limit.
- Used `curl --limit-rate -F 'data=@the_letter_m.txt' ...` to upload files really slowly. Got cut off by the connection limit. With `enable_post_data_reading` off, this correctly killed the connections //before// the uploads finished.
- I'll send this stuff to `secure` before production to give it more of a chance.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13008
Differential Revision: https://secure.phabricator.com/D18703
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary: Adds a `MenuName` method to applications that `ProfileMenuItem` uses instead of the application name if set. This improves the home/menu/new user experience at little cost. Also renamed the label from Applications to Favorites, since this menu gets altered to provide more than just applications. This also allows instances to set back to Maniphest if they so choose. Overall I think this direction resolves 95% of my concerns, with maybe a small potential downside which I don't really anticipate. We already name Dashboard panels by their object, and that hasn't really caused confusion. I think these links are similar. I click 'Tasks' and get presented a list of my tasks from Maniphest.
Test Plan: Review each of the name changes as a default new install and a modified install.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18524
Summary: Adds a very basic list of all inline comments, threaded, and their status. Kept this a little simpler than the mock, mostly because sorting here feels a little strange given threads would be all over the place. Not sure sorted is needed in practice anyways. I'd probably lean towards just adding a JS checkbox to hide certain rows if needed in the future.
Test Plan:
Test various commenting structures:
- Leave Comment
- Update Diff
- Leave new comment
- Reply to comment
- Reply to comment as revision author
- Mark items as done
- Update diff again
{F4996915}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18112
Summary: We seem to already support this, just takes it fully there. We don't need to see things like "Flag", etc, on certain subpages of projects/people/etc.
Test Plan: Review Members, Subproject pages, no longer see "Flag for Later" which only is for the Project itself. Check manage, still there.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17897
Summary:
Ref T12685. I provided this incorrect (`return new` rather than `throw`) implementation earlier; it can now be replaced with a proper implementation.
This caused application policy edits to spew this into the daemon log:
```
[2017-05-14 15:35:27] EXCEPTION: (Error) Call to undefined method PhutilMethodNotImplementedException::setActor() at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:69]
```
Test Plan:
- Used `bin/worker execute --id <id>` to execute a previously-failing task.
- Saw a feed story publish.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12685
Differential Revision: https://secure.phabricator.com/D17876
Summary: Still needs some cleanup, but ready for review in broad outline form.
Test Plan:
Made lots of policy changes to the Badges application and confirmed expected rows in `application_xactions`, confirmed expected changes to `phabricator.application-settings`.
See example output (not quite working for custom policy objects) here:
{F4922240}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, chad, epriestley
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17757
Summary: Makes it more clear whose authority actions have been taken under.
Test Plan: {F4916376}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17741
Summary:
Ref T11476. This is a bit hacky, but makes `Application` extend `LiskDAO` so we can apply transactions to it with an `Editor` class.
Also fixes schema stuff so builds should produce a clean bill of health again.
This might only get you slightly further, yell if you run into more trouble.
Test Plan:
- Ran `bin/storage upgrade -f` and got no warnings.
- Browsed around, nothing exploded?
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17738
Summary: Part of the groundwork for T11476.
Test Plan: ran `./bin/storage upgrade` and observed expected DB tables
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11476
Differential Revision: https://secure.phabricator.com/D17736
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12174. This fixes more bugs than it creates, I think:
- Dashboards now show the whole menu.
- Project and home items now show selected state correctly.
- The "choose global vs personal" thing is now part of MenuEngine, and the same code builds it for Home and Favorites.
- Home now handles defaults correctly, I think.
Maybe regression/bad/still buggy?:
- Mobile home is now whatever the default thing was, not the menu?
- Title for dashboard content or other items that render their own content is incorrectly always "Configure Menu" (this was preexisting).
Test Plan:
- Created, edited, reordered, disabled, deleted and pinned personal and global items on home, favorites, and projects.
- Also checked User profiles.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12174
Differential Revision: https://secure.phabricator.com/D17273
Summary: Ref T12136. This just yanks the band-aid off. Fundamentally these were useful well before Dashboards and advanced bucketing, but not so much any more. They also have some performance hit.
Test Plan: Add some tasks and diffs onto a new instance, see there is no count on the home menu bar.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12136
Differential Revision: https://secure.phabricator.com/D17238
Summary:
Ref T12140. The major effect of this change is that uninstalling "Home" (as we do on admin.phacility.com) no longer uninstalls the user menu (which is required to access settings or log out).
This also simplifies the code a bit, by consolidating how menus are built into MenuBarExtensions instead of some in Applications and some in Extensions.
Test Plan:
- While logged in and logged out, saw main menus in the correct order.
- Uninstalled Favorites, saw the menu vanish.
- Uninstalled Home, still had a user menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12140
Differential Revision: https://secure.phabricator.com/D17239
Summary: Ref T5867. I sure love Javascript.
Test Plan: Navigated between Home, Diffusion and Differential, opening the user profile menu. Saw appropraite help items.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5867
Differential Revision: https://secure.phabricator.com/D17214
Summary:
Still lots to fix here, punting up since I'm running into a few roadblocks.
TODO:
[] Sort Personal/Global correctly
[] Quicksand in Help Items correctly on page changes
Test Plan: Verify new menus work on desktop, tablet, mobile. Test logged in menus, logged out menus. Logging out via a menu, verify each link works as expected. Help menus get build when using an app like Maniphest, Differential. Check that search works, preferences still save.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12107
Differential Revision: https://secure.phabricator.com/D17209
Summary:
Fixes T9058. Normally, "Query" classes apply an application check and just don't load anything if it fails.
However, in some cases (like email recipient filtering) we run policy checks without having run a Query check first. In that case, one user (the actor) loads the object, then we filter it against other users (the recipeints).
Explicitly apply the application check during normal filtering.
Test Plan: Added a failing test case and made it pass.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9058
Differential Revision: https://secure.phabricator.com/D17127
Summary: Ref T11957.
Test Plan:
- Viewed an existing project profile.
- Viewed a user profile.
- Created a new project.
- Edited a profile menu.
- Added new profile items.
- Grepped for renamed symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17028
Summary:
Fixes T11946. When a logged-out viewer is loading a page on a non-public install, there are two policy issues which prevent them from loading global settings:
- They can not see the Settings application itself.
- They can not see the global settings object.
Allow them to see Settings by making mandatory applications always visible. (This doesn't make any application pages public.)
Allow them to see the global settings object explicitly.
Test Plan:
Changed default language, viewed logged-out page:
{F2076924}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11946
Differential Revision: https://secure.phabricator.com/D16983
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:
- Rebuild Celerity map to fix grumpy unit test.
- Fix one issue on the policy exception workflow to accommodate the new code.
Test Plan:
- `arc unit --everything`
- Viewed policy explanations.
- Viewed policy errors.
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D16831
Summary: Ref T8628.
Test Plan: Performed an action that uses the redirect controller (trying to visit a repo page while not logged in). Logged in and was redirected as expected
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D16571
Summary: Fixes T11541. `PhabricatorApplication::getIconURI()` has been returning only null for a while (I assume in preparation to remove it). I removed the method and all the remaining call sites.
Test Plan: Removed the method and then clicked around. Things didn't explode!
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T11541
Differential Revision: https://secure.phabricator.com/D16470
Summary: Getting rid of some code! This method has no callsites so it should be safe to remove completely. Ref T9690
Test Plan: Removed method and clicked around to make sure nothing broke.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: yelirekim, epriestley
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D16439
Summary: Fixes T11243. Seems reasonable to open this stuff in a new window so you don't put any application state in Herald, etc., at risk -- looking in this menu for help with a currently-executing workflow is reasonable and normal.
Test Plan: Clicked a help menu link, saw it open in a new page.
Reviewers: avivey, chad
Reviewed By: chad
Maniphest Tasks: T11243
Differential Revision: https://secure.phabricator.com/D16230
