Summary: See PHI1707, which has a Jupyter notebook which fails to diff nicely when modified. The root cause seems to be that the document does not end in a newline.
Test Plan: Applied patch, diffed the file, got a Jupyter diff out of it.
Differential Revision: https://secure.phabricator.com/D21159
Summary:
Ref T13455. Make "hidden" a changeset property similar to other changeset properties.
We don't need to render this on the server, so we make a request (to update the setting) and just discard the response.
Test Plan: {F7375468}
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21158
Summary: Ref T13516. Mark low-importance changes (generated code, deleted files) and owned-with-authority changes in the filetree.
Test Plan: {F7375327}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21157
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.
Restores the inline tips in the path tree.
Test Plan: {F7374175}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21154
Summary: Ref T13516. Apply basic UI styling to the new UI and make some more interaction work.
Test Plan: {F7374096}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21153
Summary: Ref T13516. Generate a tree structure based on the page changesets. Still missing styles and a whole lot of behavior.
Test Plan: {F7373967}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21152
Summary: Ref T13516. This glues "FormationView" to "ChangesetList". The actual tree is not functional in any meaningful way yet.
Test Plan: {F7373838}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21151
Summary:
Ref T13515. This restores the "Open in Editor" behavior to Diffusion, and makes "\" work there.
The URI pattern is now sent as a structured template to the client, so the code will work properly if a file path contains "%l".
Test Plan:
- Clicked "Open in Editor" and pressed "\" in Diffusion when viewing a file.
- Clicked a line, hit "\", got the file opened to that line.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21149
Summary: Ref T13515. External editor URIs currently depend on repositories having callsigns, but callsigns are no longer required. Add some variables to support configuring this feature for repositories that do not have callsigns.
Test Plan: Changed settings to use new variables, saw links generate appropriately.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21147
Summary:
Ref T13515.
- Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
- Generate a list of functions from an authority in the parser.
- Generate a list of protocols from configuration.
Test Plan: {F7370872}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21146
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.
This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.
Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:
- Set a cookie on the redirect which identifies the form we just saved.
- On page startup: if this cookie exists, save the value and clear it.
- If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.
This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?
Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21144
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.
The parser also can't identify invalid variables.
Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.
(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)
Test Plan:
- Added a bunch of tests for the actual parser.
- Used "Open in Editor" in Differential.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21143
Summary: Ref T13515. No callsites actually use this, most editors don't support it, it doesn't seem terribly useful for the ones that do, it makes template-based APIs for line-number substitution complicated, and we can probably just loop on `window.open()` anyway.
Test Plan: Grepped for affected symbols, found no more references. Loaded settings page, saw no more setting.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21142
Summary:
Ref T13515. Adding "\" ("Open in External Editor") made this slighlty worse, but it was already pretty bad.
Long ago the keys had a special style on them, but this got changed and dropped somewhere around D16568 -- although at the time, I think they still had a grey background (see T11654).
Some later change removed this background.
Put the background back and separate the keystrokes into groups.
Test Plan: {F7370615}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21141
Summary:
Ref T13515. It's not intuitive that these settings are "Display Preferences", even thought they're intenrally related to some of the other display preferences.
Give them a separate group.
Test Plan: {F7370500}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21140
Summary:
Ref T13455. Update the other "view state" properties to work like "highlight" now works.
Some complexity here arises from these concerns:
- In "View Standalone", we render the changeset inline. This is useful for debugging/development, and desirable to retain.
- In all other cases, we render the changeset with AJAX.
So the client needs to be able to learn about the "state" properties of the changeset on two different flows. Prior to this change, each pathway had a fair amount of unique code.
Then, some bookkeeping issues:
- At inital rendering time, we may not know which renderer will be selected: it may be based on the client viewport dimensions.
- Prior to this change, the client didn't separate "value of the property for the changeset as rendered" and "desired value of the property".
Test Plan:
- Viewed changes in Differential, Diffusion, and in standalone mode.
- Toggled renderer, character sets, and document engine (this one isn't terribly useful). Reloaded, saw them stick.
- Started typing a comment, cancelled it, hit the undo UI.
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21138
Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.
The storage generates a "PhabricatorChangesetViewState" configuration object as an output.
When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.
The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.
Test Plan:
- Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
- Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
- Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.
Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13455
Differential Revision: https://secure.phabricator.com/D21137
Summary:
Ref T13515. We "shield" some changesets, including generated code and intradiffs with no intermediate changes.
These files don't get shielded if they have inline comments.
But, if the viewer has collapsed all the comments, we can shield the file again.
Test Plan:
- Created a change affecting files A and B, with three diffs:
- Touch A and B.
- Touch B only.
- Touch nothing.
- Added an inline to A and collapsed it.
- Viewed Diff 1 vs Diff 2:
- Saw A collapse with a note about inlines.
- Saw B changes, normally.
- Viewed Diff 2 vs Diff 3:
- Saw A collapse with a note about inlines.
- Saw B collapse normally.
- Uncollapsed the inline, viewed 1v2 and 2v3, saw A expand in both cases.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21136
Summary: Ref T13515. See PHI1661. If a file is selected, add a keystroke to click the "Open in External Editor" link.
Test Plan: In Safari, Chrome, and Firefox: used "J" to select a file, then "\" to open it in an external editor. (In Safari and Chrome, this prompts.)
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21135
Summary: Fixes T13512. Archived packages in Owners are missing hinting, but should have it.
Test Plan:
Before:
{F7369122}
After:
{F7369128}
Maniphest Tasks: T13512
Differential Revision: https://secure.phabricator.com/D21134
Summary:
Ref PHI1292. Enable fulltext searchs in paste. Maybe this should only index a snippet instead of the entire content?
Also updates table names in `PhabricatorPasteQuery`.
Test Plan: Created some pastes, indexed them, searched for them.
Reviewers: amckinley
Subscribers: codeblock, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20650
Summary: Ref T13511. Currently, Ferret fulltext field functions (like "title:") are hard-coded. Modularize them so extensions may define new ones.
Test Plan: Added a new custom field which emits data for the indexer, searched for "animal-noises:moo", "animal-noises:-", etc., in global search and application search.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21131
Summary:
Ref T13511. Ferret functions currently define "aliases", and some applications override the default aliases.
This probably isn't really the right model, since it means the available function aliases in global search depend on the types of documents you're searching for. This isn't fundamentally unworkable but is kind of weird.
Regardless, these don't actually work. Searching for "description:x" is a syntax error.
Since they don't work, it's a good bet no one is relying on them. Just get rid of them until there's a clearer argument for the feature.
Test Plan: Grepped for "getFunctionMap", got no other hits. Ran some queries with the alias functions, got syntax errors.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21130
Summary:
Ref T13501. Depends on D21127. With the "prefix" behavior removed in D21127, we now have two virtually identical copies of the same code.
The newer one in Ferret is better: it slices utf8 correctly and is slightly more efficient on large inputs. Pull it out and make all callers call into it.
Test Plan:
- Grepped for all affected symbols.
- Ran `bin/search index --force ...` to reindex various objects (tasks, files).
- Searched for things in the UI.
Maniphest Tasks: T13501
Differential Revision: https://secure.phabricator.com/D21128
Summary:
Ref T13501. The older ngram code has some "prefix" behavior that tries to handle cases where a user issues a very short (one or two character) query.
This code doesn't work, presumably never worked, and can not be made to work (or, at least, I don't see a way, and am fairly sure one does not exist).
If the user searches for "xy", we can find trigrams in the form "xy*" using the index, but not in the form "*xy". The code makes a misguided effort to look for " xy", but this will only find "xy" in words that begin with "xy", like "xylophone".
For example, searching Files for "om" does not currently find "random.txt".
Remove this behavior. Without engaging the trigram index, these queries fall back to an unidexed "LIKE" table scan, but that's about the best we can do.
Test Plan: Searched for "om", hit "random.txt".
Maniphest Tasks: T13501
Differential Revision: https://secure.phabricator.com/D21127
Summary: Ref T13511. This function does nothing interesting and has no callers.
Test Plan: Grepped for callers.
Maniphest Tasks: T13511
Differential Revision: https://secure.phabricator.com/D21126
Summary:
Ref T13507. We currently compress normal responses, but do not compress file data responses because most files we serve are images and already compressed.
However, there are some cases where large files may be highly compressible (e.g., huge XML files stored in LFS) and we can benefit from compressing responses.
Make a reasonable guess about whether compression is beneficial and enable compression if we guess it is.
Test Plan:
- Used `curl ...` to download an image with `Accept-Encoding: gzip`. Got raw image data in the response (as expected, because we don't expect images to be worthwhile to recompress).
- Used `curl ...` to download a text file with `Accept-Encoding: gzip`. Got a compressed response. Decompressed the response into the original file.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21125
Summary:
See <https://hackerone.com/reports/850114>.
An attacker with administrator privileges can configure "notification.servers" to connect to internal services, either directly or with chosen parameters by selecting an attacker-controlled service and having it issue a "Location" redirect.
Generally, we allow this attack to occur. The same administrator can use an authentication provider or a VCS repository to perform the same attack, and we can't reasonably harden these workflows without breaking things that users expect to be able to do.
There's no reason this particular variation of the attack needs to be allowable, though, and the current behavior isn't consistent with how other similar things work.
- Hide the "notification.servers" configuration, which also locks it. This is similar to other modern service/server configuration.
- Don't follow redirects on these requests. Aphlict should never issue a "Location" header, so if we encounter one something is misconfigured. Declining to follow this header likely makes the issue easier to debug.
Test Plan:
- Viewed configuration in web UI.
- Configured a server that "Location: ..." redirects, got a followed redirect before and a failure afterward.
{F7365973}
Differential Revision: https://secure.phabricator.com/D21123
Summary:
Ref T13507. Now that we handle processing of "Content-Encoding: gzip" headers by default, this setup check can get a decompressed body back. Since it specifically wants a raw body back, disable this behavior.
Also, "@" a couple things which can get in the way if they fail now that error handling is more aggressive about throwing on warnings.
Test Plan: Ran setup check after other changes in T13507, got clean result.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21122
Summary: Ref T13507. If we believe the server can accept "Content-Encoding: gzip" requests, make the claim in an "X-Conduit-Capabilities" header in responses. Clients can use request compression on subsequent requests.
Test Plan: See D21119 for the client piece.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21120
Summary: Ref T13507. See that task for discussion.
Test Plan: Faked different response behaviors and hit both variations of this error.
Maniphest Tasks: T13507
Differential Revision: https://secure.phabricator.com/D21116
Summary:
See PHI1692. Currently, the Aphlict log is ridiculously verbose. As an initial pass at improving this:
- When starting in "debug" mode, pass "--debug=1" to Node.
- In Node, separate logging into "log" (lower-volume, more-important messages) and "trace" (higher-volume, less-important messages).
- Only print "trace" messages in "debug" mode.
Test Plan: Ran Aphlict in debug and non-debug modes. Behavior unchanged in debug mode, but log has more sensible verbosity in non-debug mode.
Differential Revision: https://secure.phabricator.com/D21115
Summary:
See PHI1692. Currently, it's hard to get a local profile or "--trace" of some Diffusion API methods, since they always proxy via HTTP -- even if the local node can serve the request.
This always-proxy behavior is intentional (so we always go down the same code path, to limit surprises) but inconvenient when debugging. Allow an operator to connect to a node which can serve a request and issue a `--local` call to force in-process execution.
This makes it straightforward to "--trace" or "--xprofile" the call.
Test Plan: Ran `bin/conduit call ...` with and without `--local` using a Diffusion method on a clustered repository. Without `--local`, saw proxy via HTTP. With `--local`, saw in-process execution.
Differential Revision: https://secure.phabricator.com/D21114
Summary: Ref T13509. In `title:big "red" dog`, keep "title" sticky across all three terms, since this seems like it's probably the best match for intent.
Test Plan: Added unit tests; ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21111
Summary:
Ref T13509. Since `title:- cat` is now ambiguous, forbid spaces after operators.
Also, forbid spaces inside operators, although this has no effect today.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21109
Summary:
Ref T13509. Currently, functions are "sticky", but this stickness is in the query execution layer.
Instead:
- move stickiness to the query compiler; and
- make it so that functions are not sticky if their arguments are quoted.
For example:
- `title:x y` previously meant `title:x title:y` (and still does). The "title:" is sticky.
- `title:"x" y` previously meant `title:x title:y`. It now means `title:x all:y`. The "title:" is not sticky because the argument is quoted.
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21108
Summary: Ref T13509. Parse "xyz:-" as "xyz is absent" and "xyz:~" as "xyz is present". These are new operators which the compiler emits separately from "not" and "substring".
Test Plan: Added unit tests, ran unit tests.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21107
Summary:
Ref T13509. Certain query tokens like `title:=""` are currently accepted by the parser but discarded, and have no impact on the query. This isn't desirable.
Instead, require that tokens making an assertion about field content must be nonempty.
Test Plan: Added unit tests, made them pass.
Maniphest Tasks: T13509
Differential Revision: https://secure.phabricator.com/D21106
Summary: Ref T13490. This simplifies some client behavior in the general case.
Test Plan: Called API method, saw URIs.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21105
Summary:
See PHI1697. If a diff is not attached to a revision (for example, if it was created with "arc diff --only"), but is attached to a repository, it is supposed to be visible only to users who can see that repository.
It currently skips this extended policy check and may incorrectly be visible to too many users.
(Once a diff is attached to a revision, this rule is enforced properly via the revision policy.)
Test Plan:
- Set repository R to be visible only to Alice.
- As Alice, created a diff from a working copy of repository R with "arc diff --only".
- As Bailey, viewed the diff.
- Before: visible diff.
- After: policy exception (as expected).
Differential Revision: https://secure.phabricator.com/D21103
Summary: Ref T13490. This simplifies mostly-theoretical cases where you're accessing Phabricator via arc-over-ssh and the Conduit protocol + domain may differ from the production protocol + domain.
Test Plan: Called API via web UI, saw sensible URI values in results.
Maniphest Tasks: T13490
Differential Revision: https://secure.phabricator.com/D21102
Summary: Ref T13505. See that task for details. When a class has exactly one "@task" block, this API returns a string. Some day, this should be made more consistent.
Test Plan: Viewed a class with exactly one "@task", no more fatal. Viewed classes with zero and more than one "@task" attributes, got clean renderings.
Maniphest Tasks: T13505
Differential Revision: https://secure.phabricator.com/D21062
Summary: Ref T13505. See that task for discussion.
Test Plan: Ran `diviner generate` locally, found a page fataling on this `strlen()`, applied patch, got a sketchy but not-broken page.
Maniphest Tasks: T13505
Differential Revision: https://secure.phabricator.com/D21061
Summary: See PHI1684. Expose the published state of the "Done" checkbox to the API.
Test Plan: Made API calls on a comment in all four states, got correct published states via the API in all cases.
Differential Revision: https://secure.phabricator.com/D21059
Summary:
See <https://discourse.phabricator-community.org/t/upgrade-from-sep-30-2016/3702/>. A user performing an upgrade from 2016 to 2020 ran into an issue where this setup query is overheating.
This is likely caused by too many rows changing state during query execution, but the particulars aren't important since this setup check isn't too critical and will catch the issue eventually. It's fine to just move on if this query fails for any reason.
Test Plan: Forced the query to overheat, loaded setup issues, got overheating fatal. Applied patch, no more fatal.
Differential Revision: https://secure.phabricator.com/D21057
Summary:
See PHI1688. If many refs with a large amount of shared ancestry are deleted from a repository, we can spend much longer than necessary marking their mutual ancestors as unreachable over and over again.
For example, if refs A, B and C all point near the head of an obsolete "develop" branch and have about 1K shared commits reachable from no other refs, deleting all three refs will lead to us performing 3,000 mark-as-unreachable operations (once for each "<ref, commit>" pair).
Instead, we can stop exploring history once we reach an already-unreachable commit.
Test Plan:
- Destroyed 7 similar refs simultaneously.
- Ran `bin/repository refs`, saw 7 entries appear in the `oldref` table.
- Ran `bin/repository discover` with some debugging statements added, saw sensible-seeming behavior which didn't double-mark any newly-unreachable refs.
Differential Revision: https://secure.phabricator.com/D21056
Summary:
Depends on D21053. Ref T11968. Three things have changed:
- Overseers can no longer use FutureIterator to continue execution of an arbitrary list of futures from any state. Use FuturePool instead.
- Same with repository daemons.
- Probably (?) fix an API change in the Harbormaster exec future.
Test Plan:
- Ran "bin/phd debug task" and "bin/phd debug pull", no longer saw Future-management related errors.
- The Harbormaster future is easiest to test by just seeing if production works once this change is deployed there.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11968
Differential Revision: https://secure.phabricator.com/D21054
Summary: Jira allows creating projects which contain number in names, phabricator will not allow such projects but it should
Test Plan: Pasted URL with Jira project which contain number in project name and it was parsed and resolved properly in phabricator
Reviewers: epriestley, Pawka, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21040
Summary:
Ref T13493. Google returns a lower-quality account identifier ("email") and a higher-quality account identifier ("id"). We currently read only "email".
Change the logic to read both "email" and "id", so that if Google ever moves away from "email" the transition will be a bit easier.
Test Plan: Linked/unlinked a Google account, looked at the external account identifier table.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21028
Summary:
Fixes T5028. Older versions of Git (apparently, from before 2010) did not provide a way to extract the raw body of a commit message from "git log", so we approximate it with "subject" and "wrapped body".
In newer versions of Git, the raw body can be extracted exactly.
Adjust how we extract messages based on the version of Git, and try to be more faithful to edge cases: particularly, be more careful to extract the correct number of trailing newlines.
Test Plan:
- Added "var_dump()" + "die(1)" later in this method, then pushed various commit messages. Used "&& false" to force execution down the old path (either path should work in modern Git).
- Observed more faithful extraction of messages, including a more faithful extraction of the number of trailing newlines. Extraction is fully faithful if we can go down the "%B" path, which we should be able to in nearly all modern cases.
- Not all messages extract faithfully or consistently across the old and new versions, but the old extraction is destructive so this is likely about as close as we can realistically ever get.
Maniphest Tasks: T5028
Differential Revision: https://secure.phabricator.com/D21027
Summary:
Depends on D21022. Ref T13493. The JIRA API has changed from using "key" to identify users to using "accountId".
By reading both identifiers, this linkage "just works" if you run against an old version of JIRA, a new version of JIRA, or an intermediate version of JIRA.
It also "just works" if you run old JIRA, upgrade to intermediate JIRA, everyone refreshes their link at least once, then you upgrade to new JIRA.
This is a subset of cases and does not include "sudden upgrade to new JIRA", but it's strictly better than the old behavior for all cases it covers.
Test Plan: Linked, unlinked, and logged in with JIRA. Looked at the "ExternalAccountIdentifier" table and saw a sensible value.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21023
Summary: Depends on D21019. Ref T13493. There are no more barriers to removing readers and writers of "accountID"; the new "ExternalAccountIdentity" table can replace it completely.
Test Plan: Linked and unlinked OAuth accounts, logged in with OAuth accounts, tried to double-link OAuth accounts, grepped for affected symbols.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21022
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.
Remove this key.
Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.
This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".
Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493, T6703
Differential Revision: https://secure.phabricator.com/D21019
Summary: Depends on D21017. Ref T13493. Update the Asana integration so it reads the "ExternalAccountIdentifier" table instead of the old "accountID" field.
Test Plan: Linked an Asana account, used `bin/feed republish` to publish activity to Asana.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21018
Summary:
Depends on D21015. When we sync an external account and get a list of account identifiers, write them to the database.
Nothing reads them yet and we still write "accountId", this just prepares us for reads.
Test Plan: Linked, refreshed, unlinked, and re-linked an external account. Peeked at the database and saw a sensible-looking row.
Differential Revision: https://secure.phabricator.com/D21016
Summary: Depends on D21014. Ref T13493. Make these objects all use destructible interfaces and destroy sub-objects appropriately.
Test Plan:
- Used `bin/remove destroy --trace ...` to destroy a provider, a user, and an external account.
- Observed destruction of sub-objects, including external account identifiers.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21015
Summary:
Depends on D21013. Ref T13493. When users log in with most providers, the provider returns an "ExternalAccount" identifier (like an Asana account GUID) and the workflow figures out where to go from there, usually a decision to try to send the user to registration (if the external account isn't linked to anything yet) or login (if it is).
In the case of password providers, the password is really a property of an existing account, so sending the user to registration never makes sense. We can bypass the "external identifier" indirection layer and just say "username -> internal account" instead of "external GUID -> internal mapping -> internal account".
Formalize this so that "AuthProvider" can generate either a "map this external account" value or a "use this internal account" value.
This stops populating "accountID" on "password" "ExternalAccount" objects, but this was only an artifact of convenience. (These records don't really need to exist at all, but there's little harm in going down the same workflow as everything else for consistency.)
Test Plan: Logged in with a username/password. Wiped the external account table and repeated the process.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21014
Summary:
Depends on D21012. Ref T13493. Currently, auth adapters return a single identifier for each external account.
Allow them to return more than one identifier, to better handle cases where an API changes from providing a lower-quality identifier to a higher-quality identifier.
On its own, this change doesn't change any user-facing behavior.
Test Plan: Linked and unlinked external accounts.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21013
Summary:
Ref T13493. This check was introduced in D4647, but the condition can never be reached in modern Phabricator because the table has a unique key on `<accountType, accountDomain, accountID>` -- so no row can ever exist with the same value for that tuple but a different ID.
(I'm not entirely sure if it was reachable in D4647 either.)
Test Plan: Used `SHOW CREATE TABLE` to look at keys on the table and reasoned that this block can never have any effect.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21012
Summary:
Depends on D21010. Ref T13493. External accounts may have multiple different unique identifiers, most often when v1 of the API makes a questionable choice (and provies a mutable, non-unique, or PII identifier) and v2 of the API uses an immutable, unique, random identifier.
Allow Phabricator to store multiple identifiers per external account.
Test Plan: Storage only, see followup changes.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21011
Summary:
Ref T13493. The "AuthAccountView" UI element currently exposes raw account ID values, but I'm trying to make these many-to-one.
This isn't terribly useful as-is, so get rid of it. This element could use a design refresh in general.
Test Plan: Viewed the UI element in "External Accounts".
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21010
Summary:
See PHI1647, which asks for "vscode://" to be a configurable protocol on hosted Phacility instances.
I made the configuration editable in D21008, but this can reasonably just come upstream too.
Test Plan: Viewed config in Config, set my editor URI to `vscode://blahblah`.
Differential Revision: https://secure.phabricator.com/D21009
Summary:
Ref T13493. I'm updating callers to `getAccountID()` to prepare to move it to a separate table.
This callsite once supported this flow:
- External users with no accounts send mail to `bugs@`.
- This creates tasks in Maniphest.
- They're CC'd when the tasks are updated.
However, after T12237 we never actually send this mail (since their addresses are necessarily unverified).
I left this code in in case this needed to be revisited, but it hasn't been an issue. Just remove it and treat these users as undeliverable.
Test Plan: As a cursory test for nothing being horribly broken, sent some object mail.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21007
Summary:
Ref T13395. No library with this name loads any more, so we can't version check it.
(Ideally, the version check stuff would be more graceful when it fails now, since it's required to load "Config" after I moved it off a separate page.)
Test Plan: Loaded "Config".
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20995
Summary: Fixes T2543. This mode has been a stable prototype for a very long time now; promote it so "--draft" can promote out of "experimental" in Arcanist.
Test Plan: See T2543 for discussion.
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D20983
Summary: Ref T13395. Moves a small amount of remaining "libphutil/" code into "phabricator/" and stops us from loading "libphutil/".
Test Plan: Browsed around; there are likely remaining issues.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20981
Summary: Ref T13395. Move cache classes, syntax highlighters, other markup classes, and sprite sheets to Phabricator.
Test Plan: Attempted to find any callers for any of this stuff in libphutil or Arcanist and couldn't.
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D20977
Summary:
`diffusion.branchquery` can return dictionary instead of array if some branches are filtered out.
Eg.:
```
{
"result": [
{
"shortName": "master",
"commitIdentifier": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
"refType": "branch",
"rawFields": {
"objectname": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
"objecttype": "commit",
```
might become:
```
{
"result": {
"1": {
"shortName": "master",
"commitIdentifier": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
"refType": "branch",
"rawFields": {
"objectname": "2817b0d8f79748ddfad0220c46d9b20bea34f460",
"objecttype": "commit",
```
Reproduction - find repository which has couple of branches, setup to track only some of them, execute `diffusion.branchquery` API call - result is dictionary instead of array
Test Plan: Apply patch, execution `diffusion.branchquery` call - result is no longer dictionary if it was one before
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D20973
Summary: Ref T10635. An install with large blocks of remarkup (4MB) in test details is reporting slow page rendering. This is expected, but I've mostly given up on fighting this unless I absolutely have to. Degrade the interface more aggressively.
Test Plan:
- Submitted a large block of test details in remarkup format.
- Before patch: they rendered inline.
- After patch: degraded display.
- Verified small blocks are not changed.
{F7180727}
{F7180728}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T10635
Differential Revision: https://secure.phabricator.com/D20970
Summary: Ref T13486. Currently, "Zarbo" sorts above "alice", but this isn't expected for a list of (mostly) human usernames.
Test Plan: Loaded a task with subscribers with mixed-case usernames.
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20969
Summary:
Depends on D20966. Ref T13486. Curtains currently render subscribers in a plain text list, but the new ref list element is a good fit for this.
Also, improve the sorting and ordering behavior.
This makes the subscriber list take up a bit more space, but it should make it a lot easier to read at a glance.
Test Plan: Viewed object subscriber lists at varying limits and subscriber counts, saw sensible subscriber lists.
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20967
Summary:
Ref T13486. When a curtain element like "Author" in Maniphest has a very long username, the wrapping and overflow behavior is poor: the date is obscured.
Adjust curtain elements which contain lists of references to other objects to improve wrapping behavior (put the date on a separate line) and overflow behavior (so we get a "..." when a name overflows).
Test Plan: {F7179376}
Maniphest Tasks: T13486
Differential Revision: https://secure.phabricator.com/D20966
Summary: Fixes T13485. GitHub has deprecated the "access_token" URI parameter for API authentication. Update to "Authorization: token ...".
Test Plan: Linked and unlinked a GitHub account locally.
Maniphest Tasks: T13485
Differential Revision: https://secure.phabricator.com/D20964
Summary: Ref T13480. Creating a rule in Herald currently uses the older radio-button flow. Update it to the "clickable menu" flow to simplify it a little bit.
Test Plan: Created new personal, object, and global rules. Hit the object rule error conditions.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20956
Summary:
Fixes T13463. Currently, if you use the web UI to set "Related Tasks" for a revision, the resulting commit does not link to the tasks.
If you use "Ref ..." in the message instead, the resulting commit does link to the tasks.
Broadly, this should all be cleaner (see T3577) but we can step toward better behavior by just copying these edges when commits are published.
Test Plan:
- Created a revision.
- Used the web UI to edit "Related Tasks".
- Landed the revision.
- Saw the commit link to the tasks as though I'd used "Ref ..." in the message.
Maniphest Tasks: T13463
Differential Revision: https://secure.phabricator.com/D20961
Summary:
Depends on D20933. Ref T13362. This reorganizes Config a bit and attempts to simplify it.
Subsections are now in a landing page console and groupings have been removed. We "only" have 75 values you can edit from the web UI nowadays, which is still a lot, but less overwhelming than it was in the past. And the trend is generally downward, as config is removed/simplified or moved into application settings.
This also gets rid of the "gigantic blobs of JSON in the UI".
Test Plan: Browsed all Config sections.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20934
Summary: Depends on D20931. Ref T13362. Move all "Console"-style interfaces to use a consistent layout based on a new "LauncherView" which just centers the content.
Test Plan: Viewed all affected interfaces.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20933
Summary: Depends on D20930. Ref T13362. Put all the "Services" parts of Config in their own section.
Test Plan: Clicked through each section. This is just an organization / UI change with no significant behavioral impact.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20931
Summary:
Ref T13362. Config is currently doing a ton of stuff and fairly overwhelming. Separate out "Modules/Extensions" so it can live in its own section.
(This stuff is mostly useful for development and normal users rarely need to end up here.)
Test Plan: Visited seciton, clicked around. This is just a visual change.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20930
Summary:
Ref T13484. If you load a subproject S which has a mangled/invalid `parentPath`, the query currently tries to execute an empty edge query and fatals.
Instead, we want to deny-by-default in the policy layer but not fail the query. The subproject should become restricted but not fatal anything related to it.
See T13484 for a future refinement where we could identify "broken / data integrity issue" objects explicilty.
Test Plan:
- Modified the `projectPath` of some subproject in the database to `QQQQ...`.
- Loaded that project page.
- Before patch: fatal after issuing bad edge query.
- After patch: "functionally correct" policy layer failure, although an explicit "data integrity issue" failure would be better.
Maniphest Tasks: T13484
Differential Revision: https://secure.phabricator.com/D20963
Summary: Ref T13480. Some Herald fields need audit information, which recent changes to Herald adapters discarded. For now, just load it unconditionally.
Test Plan: Triggered an Audit-related rule locally.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20962
Summary:
Fixes T13468. See that task for discussion. The older source-rendering code mixes "line number" / "1-based" lists with "block number" / "0-based" lists and then has other bugs which cancel this out.
For block-based diffs, build an explicit block-based mask with only block numbers. This sort of sidesteps the whole issue.
Test Plan: Viewed the diff with the original reproduction case, plus various other block-based diffs, including one-block image diffs, in unified and side-by-side mode. Didn't spot any oddities.
Maniphest Tasks: T13468
Differential Revision: https://secure.phabricator.com/D20959
Summary:
Fixes T13475. Sometimes, we issue a "no op" / "default permit" / "unchallenged" MFA token, when a user with no MFA configured does something which is configured to attempt (but not strictly require) MFA.
An example of this kind of action is changing a username: usernames may be changed even if MFA is not set up.
(Some other operations, notably "Sign With MFA", strictly require that MFA actually be set up.)
When a user with no MFA configured takes a "try MFA" action, we see that they have no factors configured and issue a token so they can continue. This is correct. However, this token causes the assocaited timeline story to get an MFA badge.
This badge is incorrect or at least wildly misleading, since the technical assertion it currently makes ("the user answered any configured MFA challenge to do this, if one exists") isn't explained properly and isn't useful anyway.
Instead, only badge the story if the user actually has MFA and actually responded to some kind of MFA challege. The badge now asserts "this user responded to an MFA challenge", which is expected/desired.
Test Plan:
- As a user with no MFA, renamed a user. Before patch: badged story. After patch: no badge.
- As a user with MFA, renamed a user. Got badged stories in both cases.
Maniphest Tasks: T13475
Differential Revision: https://secure.phabricator.com/D20958
Summary: Fixes T13480. Adds the remaining missing Owners package rules for Herald commit adapters.
Test Plan: Created hooks which care about these fields, pushed commits, saw sensible transcript values.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20957
Summary: Ref T13480. The Herald "Commit" rules still use raw commit data properties to identify authors and committers. Instead, use repository identities.
Test Plan: Wrote a Herald rule using all four fields, ran it against various commits with and without known authors. Checked transcript for sensible field values.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20955
Summary:
Ref T13480. Currently, Herald commit hook rules use a raw address resolution query to identify the author and committer for a commit. This will get the wrong answer when the raw identity string has been explicitly bound to some non-default user (most often, it will fail to identify an author when one exists).
Instead, use the "IdentityEngine" to properly resolve identities.
Test Plan: Authored a commit as `X <y@example.com>`, a raw identity with no "natural" matches to users (e.g., no user with that email or username). Bound the identity to a particular user in Diffusion. Wrote a Herald pre-commit content rule, pushed the commit. Saw Herald recognize the correct user when evaluating rules.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20953
Summary:
Ref T13480. Currently, some Herald field types are rendered in an unfriendly way on transcripts. Particularly, PHID lists are rendered as raw PHIDs.
Improve this by delegating rendering to Value objects and letting "PHID List" value objects render more sensible handle lists. Also improve "bool" fields a bit and make more fields render an explicit "None" / empty value rather than just rendering nothing.
Test Plan: Viewed various transcripts, including transcripts covering boolean values, the "Always" condition, large blocks of text, and PHID lists.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20951
Summary:
Ref T13480. Currently, when Herald renders a transcript, it puts display labels into array keys. This is a bad pattern for several reasons, notably that the values must be scalar (so you can't add icons or other markup later) and the values must be unique (which is easily violated because many values are translated).
Instead, keep values as list items.
Test Plan: Viewed Herald transcripts, saw no (meaningful) change in rendering output.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20949
Summary: Ref T13480. When Herald renders rules, it partly uses a very old handle pre-loading mechanism where PHIDs are extracted and loaded upfront. This was obsoleted a long time ago and was pretty shaky even when it worked. Get rid of it to simplify the code a little.
Test Plan: Viewed Herald rules rendered into static text with PHID list actions, saw handles. Grepped for all affected methods.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20948
Summary: Ref T13480. Add an "Author's packages" field to Herald to support writing rules like "if affected packages include X, and author's packages do not include X, raise the alarm".
Test Plan: Wrote and executed rules with the field, saw a sensible field value in the transcript.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20947
Summary: Ref T13480. These fields don't serve a specific strong use case, but are broadly reasonable capabilities after "state" vs "change" actions were relaxed by T13283.
Test Plan: Wrote rules using the new fields, added and removed projects (and did neither) to fire them / not fire them. Inspected the transcripts to see the project PHIDs making it to the field values.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20946
Summary:
Fixes T13479. The behavior of "git rev-parse --show-toplevel" has changed in Git 2.25.0, and it now fails in bare repositories.
Instead, use "git rev-parse --git-dir" to sanity-check the working copy. This appears to have more stable behavior across Git versions, although it's a little more complicated for our purposes.
Test Plan:
- Ran `bin/repository update ...` on an observed, bare repository.
- ...on an observed, non-bare ("legacy") repository.
- ...on a hosted, bare repository.
Maniphest Tasks: T13479
Differential Revision: https://secure.phabricator.com/D20945
Summary: Ref T13472. Ref T13395. These classes are only used by Phabricator and not likely to find much use in Arcanist.
Test Plan: Grepped libphutil and Arcanist for removed symbols.
Maniphest Tasks: T13472, T13395
Differential Revision: https://secure.phabricator.com/D20939
Summary:
See <https://hackerone.com/reports/758002>. The link rules don't test that their parameters are flat text before using them in unsafe contexts.
Since almost all rules are lower-priority than these link rules, this behavior isn't obvious. However, two rules have broadly higher priority (monospaced text, and one variation of link rules has higher priority than the other), and the latter can be used to perform an XSS attack with input in the general form `()[ [[ ... | ... ]] ]` so that the inner link rule is evaluated first, then the outer link rule uses non-flat text in an unsafe way.
Test Plan:
Tested examples in HackerOne report. A simple example of broken (but not unsafe) behavior is:
```
[[ `x` | `y` ]]
```
Differential Revision: https://secure.phabricator.com/D20937
Summary: Maniphest object has `getURI` method, let's use it
Test Plan: Create event in task - URI generated as expected in email notification
Reviewers: epriestley, Pawka, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20935
Summary:
Ref T13362. Some applications moved to fixed-width a while ago but I was generally unsatisfied with where they ended up and have been pushing them back to full-width.
Push Config back to full-width. Some of the subpages end up a little weird, but this provides more space to work with to make some improvements, like makign `maniphest.statuses` more legible in the UI>
Test Plan: Grepped for `setFixed(`, updated each page in `/config/`. Browsed each controller, saw workable full-width UIs.
Maniphest Tasks: T13362
Differential Revision: https://secure.phabricator.com/D20925
