Summary: Currently, debugging (log) messages from the Aphlict client trigger a visible notification in the web UI (when `notification.debug` is enabled). After D9327, the log messages can be quite verbose and seem to "block" legitimate notifications because they are not automatically dismissed.
Test Plan: Enabled `notification.debug` and observed that the debugging notifications expired and closed after a few seconds.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9339
Summary: These settings don't seem to make any difference and are somewhat confusing.
Test Plan: Sent myself test notifications via `/notification/status`. Things seemed to work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9337
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
Summary: Fixes T5195. Currently, the `./bin/repository parents` workflow doesn't respect tracked branches and will attempt to build parents caches for all branches.
Test Plan: For at least one of our repositories, this patch fixes the `Unknown commit` exception. Unfortunately, it doesn't seem to completely solve this problem though, but I suspect that this is due to commits that were overwritten with a `git push --force` or similar.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5195
Differential Revision: https://secure.phabricator.com/D9322
Summary: Fixes T5215. This mentions an old article name.
Test Plan: Read config option.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5215
Differential Revision: https://secure.phabricator.com/D9331
Summary: Point everything at the new canonical URI.
Test Plan: `grep`
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9328
Summary:
Ref T4324. Currently, an Aphlict client (with a corresponding connection to the Aphlict Server) is created for every tab that a user has open. This significantly affects the scalability of Aphlict as a service. Instead, we can use `LocalConnection` instances to coordinate the communication of multiple Aphlict clients to the server.
Similar functionality existed prior to D2704, but was removed as the author was not able to get this functionality working as intended. It seems that the main issue with the initial attempt was the use of the `setTimeout` function, which seemed to be a blocking call which prevented messages from being received. I have instead used an event-based model using a `Timer` object.
Roughly this works as follows:
# The first instance will create an `AphlictClient` and an `AphlictMaster`. The `AphlictClient` will register itself with the `AphlictMaster` and will consequently be notified of incoming messages.
# The `AphlictClient` is then responsible for pinging the `AphlictMaster` at regular intervals. If the client does not ping the master in a given period of time, the master will assume that the client is dead and will remove the client from the pool.
# Similarly, the `AphlictMaster` is required to respond to pings with a "pong" response. The pong response lets the clients know that the `AphlictMaster` is still alive. If the clients do not receive a pong in a given period of time, then the clients will attempt to spawn a new master.
Test Plan: I have tested this on our Phabricator install with a few tabs opened and inspecting the console output. I will upload a screencast of my test results.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4324
Differential Revision: https://secure.phabricator.com/D9327
Summary: If two events start on the same second (somewhat common now, since
start time can be specified) we'll hit a "push" with no range start. Instead,
always set a minimal range start.
Summary:
Elasticsearch 1.0 deprecated the "filter" top-level
parameter in favor of "post_filter" which is applied
after scores and so forth are calculated.
Instead search field.corpus with a term query.
Test Plan:
Tested against Elasticsearch 1.1.1, able to perform
basic queries without query parse errors.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4446
Differential Revision: https://secure.phabricator.com/D9321
Summary: People seem confused and it is a little inconsistent. Also added other app icon types.
Test Plan: Viewed a number of feed stories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9320
Summary: I could just add these options to my local configuration, but I figured I'd submit these upstream since they are (in my opinion) fairly common file formats.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9319
Summary:
Fixes T5199. We try to save these options in user preferences, but logged-out users don't have preferences.
Instead, just use GET links for logged-out users.
Test Plan:
- As a logged-out user, toggled blame and highlight on and off.
- As a logged-in user, toggled blame and highlight on and off.
Reviewers: btrahan, vrana
Reviewed By: vrana
Subscribers: epriestley
Maniphest Tasks: T5199
Differential Revision: https://secure.phabricator.com/D9310
Summary: Fixes T5186. If a project has no secondary tags, we issue a bogus query right now.
Test Plan: Edited a project with no secondary tags.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5186
Differential Revision: https://secure.phabricator.com/D9300
Summary: Fixes T5177. Not sure if checking for panelPHIDs is right, but seemed like a better choice than adding a new property on dashboard.
Test Plan: Create dashboard with no panels. Go to view dashboard. "view" page should have a placeholder that directs user to Manage Dashboard
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5177
Differential Revision: https://secure.phabricator.com/D9312
Summary: We haven't needed this for like three years, so we probably won't ever need it. It's in history if we do.
Test Plan: thought long and hard
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9311
Summary: After T2039, it makes sense to syntax highlight `.arclint` files as JSON.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9305
Summary: Fixes T5175. Not sure if I cleaned out everything, but this seemed like a reasonable first pass. Attempted to delete all code that belonged to Jump Nav feature only.
Test Plan: Open phabricator homepage, verify Jump Nav element is gone, verify the Search bar still autocompletes and jumps to shortcuts.
Reviewers: chad, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5175
Differential Revision: https://secure.phabricator.com/D9301
Summary:
Fixes T4991. Two issues:
- These error messages pass an object to "%s", when they mean to pass a type constant.
- The check for noncreatable credentials is incorrectly in the "edit" branch of the controller.
Test Plan:
- Edited a "SSH Key on disk" credential.
- Tried to create a credential with a bogus type.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4991
Differential Revision: https://secure.phabricator.com/D9299
Summary: Reorder main search typehaead as Jump, Apps, Prjoects, Users, Symbols instead of having projects at the bottom. Ref T5176.
Test Plan: {F159689}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5176
Differential Revision: https://secure.phabricator.com/D9283
Summary: Fixes T4818. Clarify that this does not search for arbitrary text substrings.
Test Plan: `grep`
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4818
Differential Revision: https://secure.phabricator.com/D9278
Summary: Fixes T5170, Create new page for dashboard history
Test Plan: Open dashboard, manage dashboard, click on "View History". Dashboard history should appear. Panel history should appear on panel view page under panel.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5170
Differential Revision: https://secure.phabricator.com/D9280
Summary: Ref T5021. This specific label is a little more clear as "Blocks". See also IRC.
Test Plan: eyeballed it
Reviewers: btrahan, lpriestley, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5021
Differential Revision: https://secure.phabricator.com/D9279
Summary: Updates ObjectList dashboarda and tweaks minor css items elsewhere.
Test Plan: Test my dashboard, editing, and standalone
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9275
Summary: Fixes T5021, UI labels for the fields, "Edit Dependencies" in the action list, transaction strings ("added dependent tasks", etc), UI strings in the dependencies dialog (title/submit/etc)
Test Plan: Open task, edit blocks, dialog should have new term, task history should show "blocks" instead of "dependencies"
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5021
Differential Revision: https://secure.phabricator.com/D9270
Summary:
- Make CSS more resilient with columns
- Add objectlist css
- Fix Maniphest list css
Test Plan:
Tested a number of different panels and dashboards, desktop, tablet, and mobile.
{F159447}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9273
Summary: Fixes T5090. Introduced getIcon into Handle stack which allows you to specify a per handle icon. getIcon falls back ot getTypeIcon.
Test Plan: changed the icon on a project a bunch. verified transactions showed up. verified icon showed up in typeahead. verified icon showed up in tokens that were pre-generated (not typed in). units test passed.
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5090
Differential Revision: https://secure.phabricator.com/D9264
Summary: Fixes T5165. This uses `$this->id`, but that may not always be populated anymore. Use the project ID directly instead.
Test Plan: Clicked a workboard link.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5165
Differential Revision: https://secure.phabricator.com/D9266
Summary: Highlighing and URL are fixed on click - now the edit button too.
Test Plan: click on lines with and without value in "Editr Link" (And without %l in it).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9227
Summary:
Ref T4657. Right now, you have to muck with `events.listeners` to install listeners. Instead, automatically install all subclasses of AutoEventListener.
Primarily, this makes it easier to resolve requests with "drop this file in `src/extensions/`, no warranty", which seems to have worked well so far in resolving things like custom remarkup rules, etc.
Test Plan:
- Added such a listener, had it autoregister.
- Clicked around and saw the effects of normal listeners.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4657
Differential Revision: https://secure.phabricator.com/D9262
Summary: Fixes T4022. Hooks up the project profile controller to understanding URIs like /project/hashtag/ Also, makes handles have the new /project/hashtag/ URI by default, thus upselling that feature super duper heavily.
Test Plan: clicked some project links, noted pretty uri and page working nicely.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4022
Differential Revision: https://secure.phabricator.com/D9260
Summary:
Fixes T5143. Currently, if your allowed domain is "example.com", we reject signups from "@Example.com".
Instead, lowercase both parts before performing the check.
Test Plan:
- Before patch:
- Set allowed domains to "yghe.net".
- Tried "x@yghe.net", no error.
- Tried "x@xxxy.net", error.
- Tried "x@yghE.net", incorrectly results in an error.
- After patch:
- Set allowed domains to "yghe.net".
- Tried "x@yghe.net", no error.
- Tried "x@xxxy.net", error.
- Tried "x@yghE.net", this correctly no longer produces an error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5143
Differential Revision: https://secure.phabricator.com/D9261
Summary: Makes the mobile action menu a little nicer, adds it to /people/
Test Plan: Test myself on my install, mobile and desktop.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9259
Summary:
Fixes T4021. Chooses to keep a "primary" slug based off the name - including all that lovely logic - and allow the user to specify "additional" slugs. Expose these as "hashtags" to the user.
Sets us up for a fun diff where we can delete all the Project => Phriction automagicalness. In terms of this diff, see the TODOs i added.
Test Plan:
added a primary slug as an additional slug - got an error. added a slug in use on another project - got an error. added multiple good slugs and they worked. removed slugs and it worked. made some remark using multiple new slugs and they all linked to the correct project
ran epriestley's case
- Create project "A".
- Give it additional slug "B".
- Try to create project "B".
and i got a nice error about hashtag collision
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4021
Differential Revision: https://secure.phabricator.com/D9250
Summary: Fixes T4985, add manage page, change view page to show only panels. Arguably, PhabricatorDashboardArrangeController is no longer necessary. Also, still trying to figure out if I updated all flows that involve "arrange/{id}". Probably missed some. Also not sure of the Manage Dashboard icon. Please advise.
Test Plan: Create dashboard, add panels, "view/{id}" should show just panels, Manage Dashboard should show timeline and edit links.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4985
Differential Revision: https://secure.phabricator.com/D9258
Summary:
Fixes T5094. In some cases we do slightly expensive transformations to resources (inlining images, replacing URIs, building packages). We can throw cache in front of them easily since URIs are already permanently associated with a single resource.
Also browse around and move some CSS/JS into packages.
Test Plan:
Added logging to verify the caches are working, saw moderately improved performance.
Browsed around looking at resources tab in developer console, saw fewer total requests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5094
Differential Revision: https://secure.phabricator.com/D9175
Summary:
Ref T4398. This code hadn't been touched in a while and had a few crufty bits.
**One Time Resets**: Currently, password reset (and similar links) are valid for about 48 hours, but we always use one token to generate them (it's bound to the account). This isn't horrible, but it could be better, and it produces a lot of false positives on HackerOne.
Instead, use TemporaryTokens to make each link one-time only and good for no more than 24 hours.
**Coupling of Email Verification and One-Time Login**: Currently, one-time login links ("password reset links") are tightly bound to an email address, and using a link verifies that email address.
This is convenient for "Welcome" emails, so the user doesn't need to go through two rounds of checking email in order to login, then very their email, then actually get access to Phabricator.
However, for other types of these links (like those generated by `bin/auth recover`) there's no need to do any email verification.
Instead, make the email verification part optional, and use it on welcome links but not other types of links.
**Message Customization**: These links can come out of several workflows: welcome, password reset, username change, or `bin/auth recover`. Add a hint to the URI so the text on the page can be customized a bit to help users through the workflow.
**Reset Emails Going to Main Account Email**: Previously, we would send password reset email to the user's primary account email. However, since we verify email coming from reset links this isn't correct and could allow a user to verify an email without actually controlling it.
Since the user needs a real account in the first place this does not seem useful on its own, but might be a component in some other attack. The user might also no longer have access to their primary account, in which case this wouldn't be wrong, but would not be very useful.
Mitigate this in two ways:
- First, send to the actual email address the user entered, not the primary account email address.
- Second, don't let these links verify emails: they're just login links. This primarily makes it more difficult for an attacker to add someone else's email to their account, send them a reset link, get them to login and implicitly verify the email by not reading very carefully, and then figure out something interesting to do (there's currently no followup attack here, but allowing this does seem undesirable).
**Password Reset Without Old Password**: After a user logs in via email, we send them to the password settings panel (if passwords are enabled) with a code that lets them set a new password without knowing the old one.
Previously, this code was static and based on the email address. Instead, issue a one-time code.
**Jump Into Hisec**: Normally, when a user who has multi-factor auth on their account logs in, we prompt them for factors but don't put them in high security. You usually don't want to go do high-security stuff immediately after login, and it would be confusing and annoying if normal logins gave you a "YOU ARE IN HIGH SECURITY" alert bubble.
However, if we're taking you to the password reset screen, we //do// want to put the user in high security, since that screen requires high security. If we don't do this, the user gets two factor prompts in a row.
To accomplish this, we set a cookie when we know we're sending the user into a high security workflow. This cookie makes login finalization upgrade all the way from "partial" to "high security", instead of stopping halfway at "normal". This is safe because the user has just passed a factor check; the only reason we don't normally do this is to reduce annoyance.
**Some UI Cleanup**: Some of this was using really old UI. Modernize it a bit.
Test Plan:
- **One Time Resets**
- Used a reset link.
- Tried to reuse a reset link, got denied.
- Verified each link is different.
- **Coupling of Email Verification and One-Time Login**
- Verified that `bin/auth`, password reset, and username change links do not have an email verifying URI component.
- Tried to tack one on, got denied.
- Used the welcome email link to login + verify.
- Tried to mutate the URI to not verify, or verify something else: got denied.
- **Message Customization**
- Viewed messages on the different workflows. They seemed OK.
- **Reset Emails Going to Main Account Email**
- Sent password reset email to non-primary email.
- Received email at specified address.
- Verified it does not verify the address.
- **Password Reset Without Old Password**
- Reset password without knowledge of old one after email reset.
- Tried to do that without a key, got denied.
- Tried to reuse a key, got denied.
- **Jump Into Hisec**
- Logged in with MFA user, got factor'd, jumped directly into hisec.
- Logged in with non-MFA user, no factors, normal password reset.
- **Some UI Cleanup**
- Viewed new UI.
- **Misc**
- Created accounts, logged in with welcome link, got verified.
- Changed a username, used link to log back in.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4398
Differential Revision: https://secure.phabricator.com/D9252
Summary:
Fixes T5154. Currently, "phd stop" terminates daemons relatively abruptly (and other things do too, like killing them). This can leave them with long leases that won't expire any time soon. Normally this isn't a big deal, since it just means an email or an import takes a bit longer (often 2 hours, but up to 24 hours) to run. However:
- We've increased default lease durations a lot fairly recently -- the 2 hours used to be 15 minutes.
- Harbormaster and Drydock add new types of tasks which are more dependent on other tasks, so waiting 2 hours for something to free up can hold up more stuff in queue.
When `phd start` is run, we can be confident (at least, in normal circumstances) that leases are safe to free, since we do a check. This undoes any damage done by abrupt stops in "phd stop" or by users or systems killing stuff.
(It would be nice to make "phd stop" more graceful at some point, but we always have to deal with abrupt termination in some cases no matter how gentle "phd stop" is.)
One sort-of-questionable thing here is that we don't distinguish between tasks which had an active lease and tasks which had been released, since the system itself does not make a distiction. So, for example, if you have a task that retries 5 times and waits an hour between retries, you'll get a retry on every `phd start` now, and could exhaust them all in a few minutes if you cycle `phd start` aggressively. I think this is OK. In the future, we could try to distinguish between these types of tasks, and only free the ones with active leases.
Test Plan:
- Used `phd start` normally, saw it free leases.
- Used `phd start`, killed it real quick so no taskmasters spawned, ran it again an saw no leases freed.
- Used `phd start --keep-leases`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5154
Differential Revision: https://secure.phabricator.com/D9256
Summary: Fixes T5156. If a document has been moved but the new one does not exist or can't be seen by the viewer, render a generic message.
Test Plan: Viewed moved-plus-visible and moved-plus-nonvisible documents.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5156
Differential Revision: https://secure.phabricator.com/D9254
Summary: Fixes T5113. This was caught in the crossfire of cleaning up the DiffusionRequest "commit" properties.
Test Plan: Loaded `/rXnnnn` with some of the `nnn` missing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5113
Differential Revision: https://secure.phabricator.com/D9253
Summary: placeholder text is pretty useful.
Test Plan: placeholder text is pretty useful. also fully supports not breaking everything.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9223
Summary:
Fixes T5144. This was incorrectly checking the //content// version, not the //head// version, so reverts would raise the "conflict" warning.
Also fix a couple of FontAwesome icons.
Test Plan:
- Edited a document.
- Reverted a document.
- Opened two edit tabs. Edited one, tried to edit #2, got a warning.
- Opened two revert tabs. Reverted in one, tried to revert in #2, got a warning.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5144
Differential Revision: https://secure.phabricator.com/D9249
Summary: Looks like a rule for old Firefox is causing layout issues in new Firefox. Prefer new Firefox. Fixes T4987
Test Plan:
Tested the current version of Firefox.
Bug:
{F158209}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4987
Differential Revision: https://secure.phabricator.com/D9240
Summary:
Fixes T5146. When we're rendering a transaction group that includes a comment, we hide the "x added a comment" text, since it's implicit and obvious and cleans the UI up a little.
However, the way this works is really complicated and messy and created the T5146 issue after I made self-subscriptions have a lower priority than comments do.
Clean this code up so it makes a little more sense and gets this case right.
Test Plan: {F158270}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5146
Differential Revision: https://secure.phabricator.com/D9245
