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
