Summary: This allows the SavedQuery to modify what the result list looks like (e.g., include display flags and similar).
Test Plan: Looked at some ApplicationSearch apps.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6346
Summary:
Ref T603. Ref T2625. Use cursors to page Differential queries, not offsets.
The trick here is that some queries are ordered. In these cases, we either need to pass some kind of tuple or do a cursor lookup. For example, if you are viewing revisions ordered by `dateModified`, we can either have the next page be something like:
?afterDateModified=2398329373&afterID=292&order=modified
...or some magical token:
?afterToken=2398329373:292&order=modified
I think we did this in Conpherence, but one factor there was that paging orders update with some frequency. In most cases, I think it's reasonable to pass just the ID and do a lookup to get the actual clause value (e.g., go look up object ID 292 and see what its dateModified is) and I think this is much simpler in general.
Test Plan: Set page size in Differential to 3, and paged through result lists ordered by date created and date modified.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6345
Summary:
Ref T603. Ref T2625. Makes `DifferentialRevisionQuery` do policy checks.
Note that it still uses inefficient offset-based paging, but it's rare to page through revisions. I'll switch to cursor paging in a future diff.
Test Plan: Viewed a bunch of Differential interfaces, home page, etc. This shouldn't actually materially impact anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6344
Summary:
Currently, when querying for responsible users (revisions where a given user is either the author or a reviewer), we do this:
- If the query passes a bunch of hard-coded special cases, use a special hard-coded UNION.
- Otherwise, use a very complicated JOIN/WHERE clause.
This is bad for several reasons:
- Tons and tons of hard-coding and special casing.
- The JOIN/WHERE clause performs very poorly for large datasets.
- (As a material consequence, the homepage issues a responsible query which barely misses the hard-coded special cases and goes down the slow path.)
Instead, //always// use the UNION strategy to execute a "responsible" query. Specifically, if we have responsible PHIDs, temporarily add them to the author list and build a normal query, then repeat for reviewers, then UNION any clauses we built.
Fixes T3377. Ref T603. Ref T2625. Depends on D6342.
There's various folklore about UNION ALL / UNION DISTINCT performance. UNION DISTINCT is simpler here and the number of rows is small, although we could use UNION ALL in the form:
SELECT * FROM ((SELECT ...) UNION ALL (SELECT ...) ORDER) GROUP LIMIT
...if we find that there's some performance benefit at some point.
Test Plan: Used DarkConsole to examine queries. Viewed home page and Differential dashboard/authors/subscribers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625, T3377
Differential Revision: https://secure.phabricator.com/D6343
I don't actually have Phabricator installed locally so I have no idea if this works, but in theory this should add one of the + buttons in the left sidebar to quickly add a new macro.
Summary: This makes it namespace/database/connection aware and a little easier to find. Also use pht() / PhutilConsole.
Test Plan: Ran `bin/storage probe`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6341
Summary: They seem to look OK?
Test Plan: {F48529} {F48530}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6350
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.
I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.
Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6338
Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.
Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6337
Summary: Ref T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.
Test Plan: Executed all four modes of `differential.find`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6335
Summary:
Ref T603.
- Primarily, this gets rid of a `DifferentialRevisionListData` callsite.
- Also modernize and clean up some UI stuff.
Test Plan:
{F48260}
{F48261}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6334
Summary:
- Add GC support to conduit logs.
- Add Query support to conduit logs.
- Record the actual user PHID.
- Show client name.
- Support querying by specific method, so I can link to this from a setup issue.
@wez, this migration may not be fast. It took about 8 seconds for me to migrate 800,000 rows in the `conduit_methodcalllog` table. This adds a GC which should keep the table at a more manageable size in the future.
You can safely delete all data older than 30 days from this table, although you should do it by `id` instead of `dateCreated` since there's no key on `dateCreated` until this patch.
Test Plan:
- Ran GC.
- Looked at log UI.
- Ran Conduit methods.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Differential Revision: https://secure.phabricator.com/D6332
Summary:
Ref T603. Ref T2625.
Long chain of "doing the right thing" here: I want to clean this up, so I can clean up the Conduit logs, so I can add a setup issue for deprecated method calls, so I can remove deprecated methods, so I can get rid of `DifferentialRevisionListData`, so I can make Differntial policy-aware.
Adds modern infrastructure and UI to all of the Conduit interfaces (except only partially for the logs, that will be the next diff).
Test Plan:
{F48201}
{F48202}
{F48203}
{F48204}
{F48206}
This will get further updates in the next diff:
{F48205}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6331
Summary:
Depends on D6329. This fixes `http://www.example.com/D123`, which currently gets the "D123" rendered, after addition of the Asana rule. It also removes a hack for object refernces.
Basically, the "hyperlink" rule needs to happen after rules which specialize hyperlinks (Youtube, Asana) but before rules which apply to general text (like the Differential and Maniphest rules). Allow these rules to specify that they have higher or lower priority.
Test Plan: Asana rules, Differential rules and Diffusion rules now all markup correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6330
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.
Test Plan:
Viewed some commits?
iiam
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6339
Summary:
Addes a button group to filter tasks by agents, non-agents or all.
Fixes T3394
Test Plan: View task list, filter by agents, filter by non agents. Make sure the correct tasks display.
Reviewers: epriestley, dctrwatson
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3394
Differential Revision: https://secure.phabricator.com/D6328
Summary: Fixes T3400. Users are crafty. Attempt to outwit them.
Test Plan: Added all kinds of nonsense to my PATH to hit all the errors. Verified sensible-looking error messages which I couldn't figure out any way to misread or outwit.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3400
Differential Revision: https://secure.phabricator.com/D6318
Summary:
Ref T3116. This is a large amount of schema for V0 but it seems relatively complete to the desired features in T3116.
The only thing of note that is missing is documentSignatures should have some sort of "signedStatus". "Un-signing" seemed weird to me, though I could imagination "pending signature". "Pending signature" could be done via edges pretty easily.
Plan is to have "Document" be at the top level and own policy. "DocumentBody" will store a version of title and text for each and every "edit" on a larger Document. "Edges" are to be used to tie Authors => Document for V0ish. Transactions are going to be used to store all the various edits possible here. Oh and DocumentSignatures will do what you expect, but include documentVersion as part of the key.
Test Plan: just some schema. `storage update` worked though!
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D6323
Summary: I have simply copied the existing css into the oblivious skin. I don't know if this is the right approach (code duplication), but considering this skin should be isolated (and will potentially differ) I think this makes sense.
Test Plan: Use a code block on a phame post.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Differential Revision: https://secure.phabricator.com/D6313
Summary:
Fixes T3436. Currently, when installs have configuration options which we don't know about, we raise a fairly confusing/ambiguous message about the options being unknown. Instead:
- Keep a list of previously valid (but now deleted) config, with explanatory reasons for what happened to it. Present this information, along with altenate wording ("Obsolete Config" instead of "Unknown Config") where applicable.
- Show a list of all the places the config is defined.
- Provide an active link to delete it from the web UI.
- Provide a command to delete it from the CLI.
- Allow `bin/config delete` to delete configuration options which no longer have a definition.
Test Plan:
- Set an auth key in database, local and file config.
- Walked through the setup issue, cleaning it up.
- Set an invalid key and made sure I still got a reasonable error (this now has better cleanup instructions).
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3436
Differential Revision: https://secure.phabricator.com/D6317
Summary: Currently you can't refresh the live blog or a blog post after clicking "View Live" due to POST action. I have removed the setRenderAsForm call on the "View Live" actions. I am unsure if this has any unintended consequences but I have tested and not found any.
Test Plan: Click the "View Live" action within a blog post or blog, and observe that the request occurs via GET not POST
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6314
Summary: Ref T2852. Setting followers (like CCs) is a separate API call, but we don't need to do anything complicated.
Test Plan: Synchronized revisions and verified the parent task got followers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6308
Summary:
Ref T2852.
Before trying related users, try using the feed story's actor. This is the most correct voice to act in.
Test Plan: Ran `feed/republish`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6305
Summary:
Ref T2852.
The parent task is open unless the revision is in the states "closed" or "abandoned". If it's in "needs review", it remains open. This last bit is slightly unlike Differential, but consistent with the Google Doc and generally seems like a better fit. There's no way to put the task in a "Waiting on Others" state in Asana like we can in Differential.
The subtasks are closed unless the revision is in the state "needs review". This is generally consistent with Differential.
Test Plan:
Made a series of changes to a revision and synchronized it repeatedly:
- requested changes
- commandeered
- requested review
- abandoned
Verified task and subtasks synchronized states correctly in Asana.
{F47554}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6304
Summary:
Ref T2852. Depends on D6302. This now creates, destroys, and synchronizes subtasks.
- After finishing the parent task stuff, we pull a list of all known subtasks.
- We load all those subtasks.
- If we fail to load any, we delete their objects and edges on the Phabricator side.
- Of the remaining subtasks, we find subtasks for users who aren't related to the object any more and delete them in Asana and locally (for example, if alincoln is removed as a reviewer, we delete his subtask).
- For all the related users, we either synchronize their existing task or create a new one for them.
- Then we write edges for any new tasks we added.
This doesn't handle a few weird edge cases in any specific way:
- If a subtask is moved under a different parent, we ignore it.
- If a new subtask is created that we don't know about, we ignore it.
- If a subtask we know about is deleted, we just respawn it. This is consistent with "DON'T EDIT THESE". You can force sync to stop by deleting the parent.
Addititionally:
- Make the "don't edit" warning more compelling and visceral.
Test Plan:
- Kind of ran it a bit.
- There are like 3,000 edge cases here so this is hard to test exhaustively.
- Forced a few of the edge cases to happen.
- Nothing seems immediately broken in an obvious way?
{F47551}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6303
Summary:
Ref T2852. This is highly incomplete but seems structurally sound. Some additional context is available in the Google doc.
- Add a workspace ID configuration. Without it, nothing else activates.
- Add a worker which reacts to feed stories.
- Feed stories about things which aren't Differential objects are ignored.
- We load the revision, or fail permanently if we can't.
- We get all the related user PHIDs (author, reviewers, CCs).
- We check if any of them have linked Asana accounts, or fail permanently if they don't.
- We check for an "ASANATASK" edge from the revision.
- If we do not find one, we create a new task.
- If we do find one, we load the task.
- If we succeed, we check the chronological key of the most recent synchronized feed story ("cursor").
- If this story is the same or newer, we update the task to synchronize it to the current state of the revision.
- If we fail to load the task, we fail permanently ("asana task has been deleted").
- We then publish the actual story text to the task.
Not in yet:
- Updating followers requires separate API calls which we don't do yet.
- No subtasks yet.
- No sync of open/closed state.
Test Plan: {F47546}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6302
Summary: Currently if disqus is used and a phame post is loaded over ssl, the disqus api is not loaded over https. This fixes that by honouring the protocol being used by the html document.
Test Plan: Open a phame post over https
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6311
Summary: Ref T2852. Reduce the number of magical strings in use, and prepare the Asana bridge for eventual workspace/project support (a little bit).
Test Plan: Verified enriched links still work properly.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6301
Summary:
Ref T2852. Add a `log()` method to `PhabricatorWorker` to make debugging easier.
I renamed the similar Drydock-specific method.
Test Plan:
Used logging in a future revision:
...
<<< [36] <http> 211,704 us
Updating main task.
>>> [37] <http> https://app.asana.com/api/1.0/tasks/6153776820388
...
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6296
Summary:
Ref T1536. Ref T2852. Currently, after refreshing the token we don't actually return it. This means that code relying on token refresh fails once per hour (for Asana) in a sort of subtle way. Derp.
Update `bin/auth refresh` to make this failure more clear.
Test Plan: Set `force refresh` flag and verified a return value.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536, T2852
Differential Revision: https://secure.phabricator.com/D6295
Summary:
Ref T2852.
- Broadly, we support "I have a Ref, I need a PHID" well but not "I have a PHID, I need a Ref".
- Add DoorkeeperExternalObjectQuery, and use it to query ExternalObjects.
- Allow external objects to be imported by their internal PHIDs. Basically, if we have an edge pointing at an ExternalObject, we can say "load all the data about this" from just the PHID and have it hit all the same code.
- Allow construction of Refs from ExternalObjects. This makes the "I have a PHID, I need a Ref" easier.
Test Plan:
- Verified Asana links still enrich properly at display time.
- Used in future revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6294
Summary:
Ref T2852. When we pull edge data and an edge has none, we currently populate `null` in the results. This is inconvenient and makes `idx()`'ing it clumsy. Instead, populate `array()` for empty.
(We've barely used edge data anywhere so far, which is why this hasn't come up before, but I have some use cases for it now.)
Test Plan:
- Trivial / used in future diff.
- Verified existing edge data callsites don't care about this API change (there are only 3).
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6293
Summary:
Ref T2852. I want to model Asana integration as a response to feed events. Currently, we queue one feed event for each HTTP hook.
Instead, always queue one feed event and then have it queue any necessary followup events (now, http hooks; soon, asana).
Add a script to make it easy to reproducibly fire feed event publishing.
Test Plan:
Republished a feed event and verified it hit configured HTTP hooks correctly.
$ ./bin/feed republish 5765774156541908292 --trace
>>> [2] <connect> phabricator2_feed
<<< [2] <connect> 1,660 us
>>> [3] <query> SELECT story.* FROM `feed_storydata` story JOIN `feed_storyreference` ref ON ref.chronologicalKey = story.chronologicalKey WHERE (ref.chronologicalKey IN (5765774156541908292)) GROUP BY story.chronologicalKey ORDER BY story.chronologicalKey DESC
<<< [3] <query> 595 us
>>> [4] <connect> phabricator2_differential
<<< [4] <connect> 760 us
>>> [5] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [5] <query> 478 us
>>> [6] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [6] <query> 449 us
>>> [7] <connect> phabricator2_user
<<< [7] <connect> 1,062 us
>>> [8] <query> SELECT * FROM `user` WHERE phid in ('PHID-USER-lqiz3yd7wmk64ejugvov')
<<< [8] <query> 540 us
>>> [9] <connect> phabricator2_file
<<< [9] <connect> 951 us
>>> [10] <query> SELECT * FROM `file` WHERE phid IN ('PHID-FILE-gq6dlsysvxbn3dgwvky7')
<<< [10] <query> 498 us
>>> [11] <query> SELECT * FROM `user_status` WHERE userPHID IN ('PHID-USER-lqiz3yd7wmk64ejugvov') AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo
<<< [11] <query> 507 us
Republishing story...
>>> [12] <query> SELECT story.* FROM `feed_storydata` story JOIN `feed_storyreference` ref ON ref.chronologicalKey = story.chronologicalKey WHERE (ref.chronologicalKey IN (5765774156541908292)) GROUP BY story.chronologicalKey ORDER BY story.chronologicalKey DESC
<<< [12] <query> 685 us
>>> [13] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [13] <query> 489 us
>>> [14] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [14] <query> 512 us
>>> [15] <query> SELECT * FROM `user` WHERE phid in ('PHID-USER-lqiz3yd7wmk64ejugvov')
<<< [15] <query> 601 us
>>> [16] <query> SELECT * FROM `file` WHERE phid IN ('PHID-FILE-gq6dlsysvxbn3dgwvky7')
<<< [16] <query> 405 us
>>> [17] <query> SELECT * FROM `user_status` WHERE userPHID IN ('PHID-USER-lqiz3yd7wmk64ejugvov') AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo
<<< [17] <query> 551 us
>>> [18] <query> SELECT story.* FROM `feed_storydata` story JOIN `feed_storyreference` ref ON ref.chronologicalKey = story.chronologicalKey WHERE (ref.chronologicalKey IN (5765774156541908292)) GROUP BY story.chronologicalKey ORDER BY story.chronologicalKey DESC
<<< [18] <query> 507 us
>>> [19] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [19] <query> 428 us
>>> [20] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [20] <query> 419 us
>>> [21] <query> SELECT * FROM `user` WHERE phid in ('PHID-USER-lqiz3yd7wmk64ejugvov')
<<< [21] <query> 591 us
>>> [22] <query> SELECT * FROM `file` WHERE phid IN ('PHID-FILE-gq6dlsysvxbn3dgwvky7')
<<< [22] <query> 406 us
>>> [23] <query> SELECT * FROM `user_status` WHERE userPHID IN ('PHID-USER-lqiz3yd7wmk64ejugvov') AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo
<<< [23] <query> 593 us
>>> [24] <http> http://127.0.0.1/derp/
<<< [24] <http> 746,157 us
[2013-06-24 20:23:26] EXCEPTION: (HTTPFutureResponseStatusHTTP) [HTTP/500] Internal Server Error
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6291
Summary: Currently setting "disqus.shortname" via config isn't actually used in the instantiation of disqus. This fix uses the shortname configured.
Test Plan: Open a phame post with disqus enabled and a shortname other than "phabricator" specified.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6310
Summary: When I ignore setup issues, I want them to look dealt with, and keep yellow for new ones. Also updated callout colors.
Test Plan: Ignored a number of issues.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6300
Summary: Ref T3322. Depends on D6297. Here are some Phabricator tweaks to complment D6297.
Test Plan: {F47522}
Reviewers: garoevans
Reviewed By: garoevans
CC: aran, chad
Maniphest Tasks: T3322
Differential Revision: https://secure.phabricator.com/D6298
Summary:
Ref T1536. This is equivalent to logging out and logging back in again, but a bit less disruptive for users. For some providers (like Google), this may eventually do something different (Google has a "force" parameter which forces re-auth and is ostensibly required to refresh long-lived tokens).
Broadly, this process fixes OAuth accounts with busted access tokens so we can do API stuff. For other accounts, it mostly just syncs profile pictures.
Test Plan:
Refreshed LDAP and Oauth accounts, linked OAuth accounts, hit error conditions.
{F47390}
{F47391}
{F47392}
{F47393}
{F47394}
{F47395}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6290
Summary:
Ref T1536.
- Allow providers to customize the look of external accounts.
- For username/password auth, don't show the account view (it's confusing and not useful).
- For OAuth accounts, show token status.
Test Plan:
{F47374}
{F47375}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6289
Summary:
Ref T2852. Give OAuth providers a formal method so you can ask them for tokens; they issue a refresh request if necessary.
We could automatically refresh these tokens in daemons as they near expiry to improve performance; refreshes are blocking in-process round trip requests. If we do this for all tokens, it's a lot of requests (say, 20k users * 2 auth mechanisms * 1-hour tokens ~= a million requests a day). We could do it selectively for tokens that are actually in use (i.e., if we refresh a token in response to a user request, we keep refreshing it for 24 hours automatically). For now, I'm not pursuing any of this.
If we fail to refresh a token, we don't have a great way to communicate it to the user right now. The remedy is "log out and log in again", but there's no way for them to figure this out. The major issue is that a lot of OAuth integrations should not throw if they fail, or can't reasonably be rasied to the user (e.g., activity in daemons, loading profile pictures, enriching links, etc). For now, this shouldn't really happen. In future diffs, I plan to make the "External Accounts" settings page provide some information about tokens again, and possibly push some flag to accounts like "you should refresh your X link", but we'll see if issues crop up.
Test Plan: Used `bin/auth refresh` to verify refreshes. I'll wait an hour and reload a page with an Asana link to verify the auto-refresh part.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6280
Summary: Ref T2852. Provide a script for inspecting/debugging OAuth token refresh.
Test Plan: Ran `bin/auth refresh` with various arguments, saw token refreshes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6276
Summary:
Ref T2852. Primarily, this expands API access to Asana. As a user-visible effect, it links Asana tasks in Remarkup.
When a user enters an Asana URI, we register an onload behavior to make an Ajax call for the lookup. This respects privacy imposed by the API without creating a significant performance impact.
Test Plan: {F47183}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6274
Summary:
- `DoorkeeperObjectRef` is a convenience object to keep track of `<applicationType, applicationDomain, objectType, objectID>` tuples.
- `DoorkeeperBridge` provides pull/push between Phabricator and external systems.
- `DoorkeeperBridgeAsana` is a bridge to Asana.
Test Plan:
Ran this snippet and got a task from Asana:
{P871}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6273
Summary:
Ref T2852. This table holds data about external objects and allows us to write edges to them.
Objects are identified with an `<applicationType, applicationDomain, objectType, objectID>` tuple. For example, Asana tasks will be, e.g., `<asana, asana.com, asana:task, 93829279873>` or similar.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6271
Summary: ...also make it so in Pholio when you add an inline comment the preview refreshes. Fixes T2649.
Test Plan: played around in pholio leaving commentary. noted that a new inline comment would refresh the preview.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2649
Differential Revision: https://secure.phabricator.com/D6267
Summary: Ref T1536. This is missing a call.
Test Plan: Viewed a public blog with Facebook comments.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6288
Summary: Ref T2222. Currently, we load inline comments by `commentID` here, but we always pass every commentID associated with the revision. Instead, just load non-draft comments by revision ID. This simplifies querying a little bit and is likely faster anyway (draft comments are currently loaded separately).
Test Plan: Looked at some revisions and verified inlines showed up correctly and in the right places.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6270
Summary:
Ref T2222. My path forward here wasn't very good -- I was thinking I could set `transactionPHID` for the inline comments as I migrated, but it must be unique and an individual DifferentialComment may have more than one inline comment. Dropping the unique requirement just creates more issues for us, not fewer.
So the migration in D6266 isn't actually useful. Undo it -- this can't be a straight revert because some installs may already have upgraded.
Test Plan: Ran new migrations, verified the world ended up back in the same place as before (made comments, viewed reivsions).
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6269
Summary: Ref T1536. After DB-driven auth config, we need to load this differently.
Test Plan: Ran `bin/auth ldap`.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6287
Summary: Ref T2852. Asana supports a link directly to this panel, I just wasn't able to find it.
Test Plan:
Clicked the link and got to the apps panel.
{F47346}
Reviewers: isaac_asana, btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6285
Summary:
Validates the keys on page load, alerts if there is a problem.
Fixes T3432
Test Plan: Use some good and bad keys, ensure we get the error at the right time.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6284
Summary:
D6278 kind of got closed and commited, this is the actual direction.
Ref T3432
Depends on D6277
Test Plan: Keep using the site
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6283
Summary: Ref T2222. I didn't translate this query properly; reproduce the original.
Test Plan: When viewing a revision with non-draft inline comments by a user other than the viewer, the inline comments now appear on the changesets themselves.
Reviewers: kawakami, btrahan, garoevans
Reviewed By: garoevans
CC: aran, mbishopim3
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6281
Summary:
If D6277 is the way to go, then this will be it's implementation.
Depends on D6277
Test Plan: Keep using the site
Reviewers: epriestley
CC: aran, Korvin
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6278
Summary: Casting the defaul custom field storage to an int, need to stop that!
Test Plan:
Use some custom fields, make sure they work as expected.
Fixes T3444
Reviewers: epriestley, mbishopim3
Reviewed By: mbishopim3
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T3444
Differential Revision: https://secure.phabricator.com/D6282
Summary: I assume this is a bug!
Test Plan: Look at it
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6279
Summary:
Currently, it takes one frame for animations to set their first value. For fading stuff in, that means it briefly appears at 100% opacity, then jumps to 0%, then fades in from there.
Instead, immediately tween to the initial value.
Test Plan: Comments in Pholio fade in nicely. Preview is still a janky pile of mess until D6267.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6268
Summary:
Ref T2222. This adds PHIDs to all Differential comments so I can migrate the inlinecommment table to transaction_comment in the next diff.
@wez, this will issue a few million queries for Facebook (roughly, one for each Differential comment ever made). It's safe to skip the `.php` half of the patch, bring Phabricator up normally, and then apply this patch with Phabricator running if that eases the migration, although the next few diffs will probably be downtime-required migrations so maybe it's easier to just schedule some downtime.
Test Plan: Ran migration locally. Verified existing comments and new comments received PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6266
Summary:
Currently it's not allowed to be left blank (even with required: false)
Fixes T3343
Test Plan: Use the custom date field.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T3343
Differential Revision: https://secure.phabricator.com/D6251
Summary: Ref T2222. I need this to migrate the Differential comment tables, because they are large. We have the similar `LiskMigrationIterator` already, but it won't work here because I intend to destroy the original objects after migrating them.
Test Plan:
Wrote a script to iterate over the `differential_comment` table, got reasonable output:
{P869}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6264
Summary:
Ref T2222. See D6260.
Push all this junk behind a Query so I can move the storage out from underneath it.
Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6262
Summary:
Ref T2222. Ref T1460. Depends on D6260.
This creates the new tables, but doesn't start using them. I added three new fields for {T1460}, to represent fixed/done/replied states.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1460, T2222
Differential Revision: https://secure.phabricator.com/D6261
Summary:
Ref T2222.
I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.
I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:
- Wrap the new objects in the old objects for reads/writes.
- Migrate all the existing data to the new table.
- Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.
This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:
- The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
- The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
- The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.
This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.
Test Plan: Viewed a revision; made revision comments
Reviewers: btrahan
Reviewed By: btrahan
CC: edward, chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6260
Summary: Ref T1536. Like Google, GitHub is actually strict about callback URIs too. Keep them pointed at the old URIs until we can gradually migrate.
Test Plan: Logged in with GitHub.
Reviewers: garoevans, davidreuss, btrahan
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6265
Summary: Removes extra padding on rendering notifications in jx-notification.
Test Plan: test a notification
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6259
Summary: Changes it to a dialog view, tweaks some layout bugs on full width forms.
Test Plan: Tested loging in and resetting my password. Chrome + Mobile
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, nrp
Differential Revision: https://secure.phabricator.com/D6257
Summary: Used Differential emails as a formatting guide. This also includes "Image X: <inline comment>" similarly to how Differential has "<file path><line(s)>: <inline comment>" or what have you. Fixes T3138.
Test Plan: inserted some debugging code and verified the mail body format looked okay ish
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3138
Differential Revision: https://secure.phabricator.com/D6258
Summary: in applyExternalEffects, for subscriber transactions, we now re-load subscribers. also fixes a bug where a user can get emailed 2x when they take an action on a mock they created.
Test Plan: made some mocks. verified one copy sent to creator and one to each subscriber. (note having problems with email so I verified the phids mail was supposed to be sent to and did not get the actual email delivered)
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3315
Differential Revision: https://secure.phabricator.com/D6206
Summary:
Ref T1536.
- When users try to add a one-of provider which already exists, give them a better error (a dialog explaining what's up with reasonable choices).
- Disable such providers and label why they're disabled on the "new provider" screen.
Test Plan:
{F47012}
{F47013}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6256
Summary: Ref T1536. Mostly, this puts "username/password" (which is probably a common selection) first on the list.
Test Plan: {F47010}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6254
Summary: Ref T1536. This "should never happen", but can if you're developing custom providers. Improve the robustness of this interface in the presence of missing provider implementations.
Test Plan: {F47008}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6253
Summary: Not sure how to test this, but assume it's coming from this hover.
Test Plan: n/a
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3425
Differential Revision: https://secure.phabricator.com/D6255
Summary: Generally prefer 'cards' to represent individual 'items' or 'action items', so I think it works here.
Test Plan: Reload setup issues pages.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6252
Summary:
Ref T1536.
- Move all the provider-specific help into contextual help in Auth.
- This provides help much more contextually, and we can just tell the user the right values to use to configure things.
- Rewrite account/registration help to reflect the newer state of the word.
- Also clean up a few other loose ends.
Test Plan: {F46937}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6247
Summary: Ref T1536. This is the last major migration. Moves us over to the DB and drops all the config stuff.
Test Plan:
- Ran the migration.
- Saw all my old config brought forward and respected, with accurate settings.
- Ran LDAP import.
- Grepped for all removed config options.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, wez
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6243
Summary:
Ref T1536. This sets us for the "Config -> Database" migration. Basically:
- If stuff is defined in the database, respect the database stuff (no installs have anything defined yet since they can't reach the interfaces/code).
- Otherwise, respect the config stuff (all installs currently do this).
Test Plan: Saw database stuff respected when database stuff was defined; saw config stuff respected otherwise.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6240
Summary:
Adds the phids of users entered into any user project query to handles phids for handle loading
Fixes T3395
Test Plan: Load page that was previous breaking
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3395
Differential Revision: https://secure.phabricator.com/D6250
Summary:
D6057 introduced images in the typeahead results, but not all
projects return a valid result. This silently broke /owners/new because
the exception "Call to a member function loadProfileImageURI() on a non-object"
is swallowed somewhere in the handler.
Test Plan: go to /owners/new and type something in the primary owner field
Reviewers: epriestley, nh, Afaque_Hussain
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6245
Summary: Added in color variables in most used places. Tweaked green to be a bit more serious.
Test Plan: Tested Tags, Error View, Timeline, Object Views, and Color Palette.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6244
Summary:
Ref T1536. Currently, when you install Phabricator you're dumped on the login screen and have to consult the documentation to learn about `bin/accountadmin`.
Instead, detect that an install is running first-time setup:
- It has no configured providers; and
- it has no user accounts.
We can safely deduce that such an install isn't configured yet, and let the user create an admin account from the web UI.
After they login, we raise a setup issue and lead them to configure authentication.
(This could probably use some UI and copy tweaks.)
Test Plan:
{F46738}
{F46739}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6228
Summary: Remade auth and policy icon.
Test Plan: look at the images.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6241
Summary: This adds an 83% Light set of colors for highlights, warnings, etc.
Test Plan: Tested Notifications, Error View, and Color Palette page. Test is out, not quite sure on notifications.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6239
Summary: See inlines.
Test Plan: Loaded timeline UIExample.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6238
Summary: Ref T1536. Make this UI a bit more human-friendly.
Test Plan: {F46873}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6237
Summary: Moves old auth icon to 'policy' and new icon is keys.
Test Plan: photoshop
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6236
Summary: We end up with both "user.id" and "email.id". Disambiguate for ORDER.
Test Plan: Ran Conduit user.query query with "email".
Reviewers: wez, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6234
Summary:
Ref T1536. This is extremely reachable and changes the login code to the new stuff.
Notes:
- I've hard-disabled password registration since I want installs to explicitly flip it on via config if they want it. New installs will get it by default in the future, but old installs shouldn't have their auth options change.
- Google doesn't let us change the redirect URI, so keep the old one working.
- We need to keep a bit of LDAP around for now for LDAP import.
- **Facebook:** This causes substantive changes in what login code is executed.
Test Plan:
- Logged in / logged out / registered, hit new flows.
- Logged in with google.
- Verified no password registration by default.
Reviewers: btrahan, chad
Reviewed By: chad
CC: wez, nh, aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6222
Summary:
Ref T1536. Because Facebook publishes data from Phabricator to user profiles and that data is sensitive, it wants to require secure browsing to be enabled in order to login.
Respect the existing option, and support it in the UI.
The UI part isn't reachable yet.
Test Plan: {F46723}
Reviewers: chad, btrahan
Reviewed By: chad
CC: arice, wez, aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6224
Summary: Picked a set of standard colors. Based on our current Maniphest color set, but tweaked to the same hue with http://color.hailpixel.com/
Test Plan: Not intended to be end all be all, but a decent first cut. Applied to Maniphest and Tags.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6229
Summary: Ref T1536. Love me some LDAP.
Test Plan: Viewed and edited form. Looked through transactions.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6227
Summary:
Ref T1536.
- When we render a dialog on a page by itself, put it on a dust background.
- Currently, we render "Logout" in two different places. Stop doing that.
- Make sure the surviving one has workflow so we get a modal ajax dialog if possible.
Test Plan: {F46731}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6226
Summary: Ref T1536. If you only have button-based logins, the new login screen looks weird.
Test Plan:
Before
{F46725}
After
{F46726}
Reviewers: chad, jamesr
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6225
Summary:
Ref T1536. None of this code is reachable.
For the new web UI for auth edits, give providers more and better customization options for handling the form. Allow them to format transactions.
Also fix the "Auth" application icon.
Test Plan: {F46718}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6221
Summary: Touches a lot of little spacing things here and there, stuck to 4px grid when possible, checked mobile views.
Test Plan: Mobile, Logging In, Multiple Providers.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6220
Summary:
Ref T1536. Above, we load all providers, which is intentional (if a user has a link with a previously-enabled but now-disabled provider, we should enrich it with provider information).
However, before showing linking options we should drop disabled providers.
Test Plan: Disabled Disqus, reloaded, didn't see Disqus anymore.
Reviewers: mbishopim3
Reviewed By: mbishopim3
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6219
Summary:
Ref T1536.
- When linking accounts after initially failing, make the crumb say "Link Account" instead of "Login".
- When on the LDAP failure form, show a "Cancel" button returning to start (if logging in) or settings (if linking accounts).
- Allow providers to distinguish between "start", "login" and "link" rendering.
Test Plan: Linked and logged in with LDAP and other registration mechainsms.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6214
Summary:
Ref T1536.
- Add a "Cancel" button, to get back to login.
- Add a crumb showing the registering provider.
- Add an account card when registering with an external account
- Tailor some language to make it less ambiguous ("Phabricator Username", "Register Phabricator Account").
Test Plan:
{F46618}
{F46619}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6213
Summary: Ref T1536. This gets the single queries out of the View and builds a propery Query class for ExternalAccount.
Test Plan: Linked/unlinked accounts, logged out, logged in.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6212
Summary: Ref T1536. This script basically exists to restore access if/when users shoot themselves in the foot by disabling all auth providers and can no longer log in.
Test Plan: {F46411}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6205
Summary: Ref T1536. When auth providers are edited, show the edit history.
Test Plan: {F46400}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6203
Summary: Ref T1536. Many rough / broken edges, but adds the rough skeleton of the provider edit workflow.
Test Plan: {F46333}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6200
Summary: Ref T1536. Adds an initial "choose a provider type" screen for adding a new provider. This doesn't go anywhere yet.
Test Plan: {F46316}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6199
Summary:
Ref T1536. Currently, we have about 40 auth-related configuration options. This is already roughly 20% of our config, and we want to add more providers. Additionally, we want to turn some of these auth options into multi-auth options (e.g., allow multiple Phabricator OAuth installs, or, theoretically multiple LDAP servers).
I'm going to move this into a separate "Auth" tool with a minimal CLI (`bin/auth`) interface and a more full web interface. Roughly:
- Administrators will use the app to manage authentication providers.
- The `bin/auth` CLI will provide a safety hatch if you lock yourself out by disabling all usable providers somehow.
- We'll migrate existing configuration into the app and remove it.
General goals:
- Make it much easier to configure authentication by providing an interface for it.
- Make it easier to configure everything else by reducing the total number of available options.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6196
Summary: Ref T1536. These can probably use some design tweaking and there's a bit of a bug with profile images for some providers, but generally seems to be in the right ballpark.
Test Plan:
{F46604}
{F46605}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6210
Summary:
Ref T1536. Currently, we have separate panels for each link/unlink and separate controllers for OAuth vs LDAP.
Instead, provide a single "External Accounts" panel which shows all linked accounts and allows you to link/unlink more easily.
Move link/unlink over to a full externalaccount-based workflow.
Test Plan:
- Linked and unlinked OAuth accounts.
- Linked and unlinked LDAP accounts.
- Registered new accounts.
- Exercised most/all of the error cases.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6189
Summary:
Ref T1536. Facebook currently does a check which should be on-login in registration hooks, and this is generally a reasonable hook to provide.
The "will login" event allows listeners to reject or modify a login, or just log it or whatever.
NOTE: This doesn't cover non-web logins right now -- notably Conduit. That's presumably fine.
(This can't land for a while, it depends on about 10 uncommitted revisions.)
Test Plan: Logged out and in again.
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6202
Summary: Ref T1536. Error state is a bit gross but we need to sort that out in general.
Test Plan:
{F46549}
{F46550}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6208
Summary: Ref T1536. Like D6080, we don't need to store the registration key itself. This prevents a theoretical attacker who can read the database but not write to it from hijacking registrations.
Test Plan: Registered a new account.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6188
Summary:
Ref T1445. Ref T1536. Although we have separate CSRF protection and have never been vulnerable to OAuth hijacking, properly implementing the "state" parameter provides a little more certainty.
Before OAuth, we set a random value on the client, and pass its hash as the "state" parameter. Upon return, validate that (a) the user has a nonempty "phcid" cookie and (b) the OAuth endpoint passed back the correct state (the hash of that cookie).
Test Plan: Logged in with all OAuth providers, which all apparently support `state`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, arice
Maniphest Tasks: T1445, T1536
Differential Revision: https://secure.phabricator.com/D6179
Summary: Ref T1536. We can safely replace the old login validation controller with this new one, and reduce code dplication while we're at it.
Test Plan: Logged in with LDAP, logged in with OAuth, logged in with username/password, did a password reset.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6178
Summary:
Ref T1536. LDAP is very likely the worst thing in existence.
This has some rough edges (error handling isn't perfect) but is already better than the current LDAP experience! durrr
Test Plan: Registered and logged in using LDAP.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6177
Summary: Ref T1536. Support for GitHub on new flows.
Test Plan: Registered and logged in with GitHub.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6166
Summary: Ref T1536. Adds Disqus as a Provider.
Test Plan: Registered and logged in with Disqus.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6165
Summary:
Ref T1536. Ref T1930. Code is not reachable.
This provides password authentication and registration on the new provider/adapter framework.
I sort of cheated a little bit and don't really route any password logic through the adapter (instead, this provider uses an empty adapter and just sets the type/domain on it). I think the right way to do this //conceptually// is to treat username/passwords as an external black box which the adapter communicates with. However, this creates a lot of practical implementation and UX problems:
- There would basically be two steps -- in the first one, you interact with the "password black box", which behaves like an OAuth provider. This produces some ExternalAccount associated with the username/password pair, then we go into normal registration.
- In normal registration, we'd proceed normally.
This means:
- The registration flow would be split into two parts, one where you select a username/password (interacting with the black box) and one where you actually register (interacting with the generic flow). This is unusual and probably confusing for users.
- We would need to do a lot of re-hashing of passwords, since passwords currently depend on the username and user PHID, which won't exist yet during registration or the "black box" phase. This is a big mess I don't want to deal with.
- We hit a weird condition where two users complete step 1 with the same username but don't complete step 2 yet. The box knows about two different copies of the username, with two different passwords. When we arrive at step 2 the second time we have a lot of bad choices about how to reoslve it, most of which create security problems. The most stragihtforward and "pure" way to resolve the issues is to put password-auth usernames in a separate space, but this would be incredibly confusuing to users (your login name might not be the same as your username, which is bizarre).
- If we change this, we need to update all the other password-related code, which I don't want to bother with (at least for now).
Instead, let registration know about a "default" registration controller (which is always password, if enabled), and let it require a password. This gives us a much simpler (albeit slightly less pure) implementation:
- All the fields are on one form.
- Password adapter is just a shell.
- Password provider does the heavy lifting.
We might make this more pure at some point, but I'm generally pretty satisfied with this.
This doesn't implement the brute-force CAPTCHA protection, that will be coming soon.
Test Plan: Registered with password only and logged in with a password. Hit various error conditions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536, T1930
Differential Revision: https://secure.phabricator.com/D6164
Summary:
Ref T1536. None of this code is reachable.
Implements new-auth login (so you can actually login) and login validation (which checks that cookies were set correctly).
Test Plan: Manually enabled FB auth, went through the auth flow to login/logout. Manually hit most of the validation errors.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6162
Summary:
Ref T1536. Code is intentionally made unreachable (see PhabricatorAuthProviderOAuthFacebook->isEnabled()).
This adds:
- A provider-driven "start" screen (this has the list of ways you can login/register).
- Registration actually works.
- Facebook OAuth works.
@chad, do you have any design ideas on the start screen? I think we poked at it before, but the big issue was that there were a limitless number of providers. Today, we have:
- Password
- LDAP
- Facebook
- GitHub
- Phabricator
- Disqus
- Google
We plan to add:
- Asana
- An arbitrary number of additional instances of Phabricator
Users want to add:
- OpenID
- Custom providers
And I'd like to have these at some point:
- Stripe
- WePay
- Amazon
- Bitbucket
So basically any UI for this has to accommodate 300 zillion auth options. I don't think we need to solve any UX problems here (realistically, installs enable 1-2 auth options and users don't actually face an overwhelming number of choices) but making the login forms less ugly would be nice. No combination of prebuilt elements seems to look very good for this use case.
Test Plan: Registered a new acount with Facebook.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6161
Summary:
Ref T1536. None of this code is reachable.
`PhabricatorAuthLoginController` provides a completely generic login/link flow, similar to how D6155 provides a generic registration flow.
`PhabricatorAuthProvider` wraps a `PhutilAuthAdapter` and glues the generic top-level flow to a concrete authentication provider.
Test Plan: Static only, code isn't meaningfully reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6159
Summary:
Currently, registration and authentication are pretty messy. Two concrete problems:
- The `PhabricatorLDAPRegistrationController` and `PhabricatorOAuthDefaultRegistrationController` controllers are giant copy/pastes of one another. This is really bad.
- We can't practically implement OpenID because we can't reissue the authentication request.
Additionally, the OAuth registration controller can be replaced wholesale by config, which is a huge API surface area and a giant mess.
Broadly, the problem right now is that registration does too much: we hand it some set of indirect credentials (like OAuth tokens) and expect it to take those the entire way to a registered user. Instead, break registration into smaller steps:
- User authenticates with remote service.
- Phabricator pulls information (remote account ID, username, email, real name, profile picture, etc) from the remote service and saves it as `PhabricatorUserCredentials`.
- Phabricator hands the `PhabricatorUserCredentials` to the registration form, which is agnostic about where they originate from: it can process LDAP credentials, OAuth credentials, plain old email credentials, HTTP basic auth credentials, etc.
This doesn't do anything yet -- there is no way to create credentials objects (and no storage patch), but I wanted to get any initial feedback, especially about the event call for T2394. In particular, I think the implementation would look something like this:
$profile = $event->getValue('profile')
$username = $profile->getDefaultUsername();
$is_employee = is_this_a_facebook_employee($username);
if (!$is_employee) {
throw new Exception("You are not employed at Facebook.");
}
$fbid = get_fbid_for_facebook_username($username);
$profile->setDefaultEmail($fbid);
$profile->setCanEditUsername(false);
$profile->setCanEditEmail(false);
$profile->setCanEditRealName(false);
$profile->setShouldVerifyEmail(true);
Seem reasonable?
Test Plan: N/A yet, probably fatals.
Reviewers: vrana, btrahan, codeblock, chad
Reviewed By: btrahan
CC: aran, asherkin, nh, wez
Maniphest Tasks: T1536, T2394
Differential Revision: https://secure.phabricator.com/D4647
Summary: Ref T1536. This is similar to D6172 but much simpler: we don't need to retain external interfaces here and can do a straight migration.
Test Plan: TBA
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6173
Summary: make it work with z-index and make it grey.
Test Plan:
clicked the grey button and it worked! Safari and Chrome
clicked around and observed loading mask functioning correctly
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3396
Differential Revision: https://secure.phabricator.com/D6207
Summary: Ref T1536. Migrates the OAuthInfo table to ExternalAccount, and makes `PhabricatorUserOAuthInfo` a wrapper for an ExternalAccount.
Test Plan: Logged in with OAuth, registered with OAuth, linked/unlinked OAuth accounts, checked OAuth status screen, deleted an account with related OAuth.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6172
Summary:
Ref T1536. Move all access to the underlying storage to inside the class. My plan is:
- Migrate the table to ExternalAccount.
- Nuke the table.
- Make this class read from and write to ExternalAccount instead.
We can't get rid of OAuthInfo completely because Facebook still depends on it for now, via registration hooks.
Test Plan: Logged in and registered with OAuth.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6171
Summary:
Ref T1536. Currently, we store OAuth tokens along with their expiry times and status. However, all we use this for is refreshing profile pictures and showing a silly (and probably somewhat confusing) interface about token status.
I want to move this storage over to `PhabricatorExternalAccount` to make the cutover easier. Drop it for now, including all the profile image stuff (I plan to rebuild that in a more sensible way anyway).
Test Plan: Viewed screen; linked/unlinked accounts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6170
Summary:
Ref T1536. This is the schema code for `PhabricatorExternalAccount` which was previously in D4647. I'm splitting it out so I can put it earlier in the sequence and because it's simple and standalone.
Expands `PhabricatorExternalAccount` to have everything we need for the rest of registration.
Test Plan: Implemented the remainder of new registration on top of this.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6169
Summary: Ref T3377. MySQL ignores indexes if we hand it mismatched datatypes. This seems colossally dumb, but give it what it expects.
Test Plan: wat
Reviewers: wez, btrahan
Reviewed By: wez
CC: aran
Maniphest Tasks: T3377
Differential Revision: https://secure.phabricator.com/D6201
Summary: Rough pass at a PHUIButtonView Class. Keeps phutil_tag intact and adds some image features if you use the class.
Test Plan: UIExamples
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6192
Summary: Restrict the menu hovers to desktop
Test Plan: test desktop and mobile
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6190
Summary:
Ref T3354. There's no way for us to test most of the config options which actually affect this limit, so the Phabricator config is basically a canary value to indicate "the administrator hasn't configured anything yet".
Raise a setup issue if it isn't set. There's a trail to get here from Files, but we've de-emphasized the old-school upload form so it's hard to unearth.
Emphasize the warning that you need to read the documentation and configure like 30 other things to make this work.
Test Plan: Cleared my config, verified I got the issue, read it, set my config, issue went away.
Reviewers: jamesr, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3354
Differential Revision: https://secure.phabricator.com/D6185
Summary: Fixes T3242. Changes the red and orange objects to match the transactions. Also adds a highlight color to 'cards'.
Test Plan: Review my audits in my sandbox
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3242
Differential Revision: https://secure.phabricator.com/D6184
