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
Summary: Decided to just remove the hover grey to white, seems fine with the new white icons.
Test Plan: use homepage + icons
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6181
Summary: the people widget was returning a comma-delimited list of HTML nodes so kill that noise with some hsprintf action. We also weren't consistently updating the latest transaction id so simplify those codepaths (widgets vs pontificate) a bit. Fixes T3336.
Test Plan: left some messages, added some participants. noted that the people widget looked good and only the pertinent transactions were pulled down on updates.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3336
Differential Revision: https://secure.phabricator.com/D6180
Summary: The shadow on the white icons was too harsh for their size, looked bad on timelines.
Test Plan: Check timeline example, phuilist example.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6176
Summary: Took a stab at some login icons for buttons.
Test Plan: photoshop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6174
Summary: Fixes T3330
Test Plan: Test desktop and mobile menus in chrome and ios.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3330
Differential Revision: https://secure.phabricator.com/D6157
Summary:
Ref T1703. This sets the stage for (but does not yet implement) custom UI types for config. In particular, a draggable list for custom fields.
I might make all the builtin types go through this at some point too, but don't really want to bother for the moment. It would be very slightly cleaner but woudn't get us much of anything.
Test Plan:
UI now renders via custom code, although that code does nothing (produces an unadorned text field):
{F45693}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6154
Summary:
Ref T1703.
- Adds "Title".
- Adds "Blurb".
- Adds `user.fields` config for selecting and reordering. This will get UI in the next patch.
Test Plan:
{F45689}
{F45690}
Edited the fields, too.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6153
Summary:
Adds a profile edit controller (with just one field and on links to it) that uses ApplicationTransactions and CustomField.
{F45617}
My plan is to move the other profile fields to this interface and get rid of Settings -> Profile. Basically, these will be "settings":
- Sex
- Language
- Timezone
These will be "profile":
- Real Name
- Title
- Blurb
- Profile Image (but I'm going to put this on a separate UI)
- Other custom fields
Test Plan: Edited my realname using the new interface.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6152
Summary: Used more logical icons for subscribe, auto, and delete instead of the mail icons. Fixes T3329
Test Plan: Tested subscribing and unsubscribing in Maniphest.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3329
Differential Revision: https://secure.phabricator.com/D6151
Summary:
None of this code is reachable yet. See discussion in D6147. Ref T1703.
Provide tighter integration between ApplicationTransactions and CustomField. Basically, I'm just trying to get all the shared stuff into the base implementation.
Test Plan: Code not reachable.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6149
Summary:
Ref T1703. We have currently have two custom field implementations (Maniphest, Differential) and are about to add a third (User, see D6122). I'd like to generalize custom fields before doing a third implementation, so we don't back ourselves into the ApplicationTransactions corner we have with Maniphest/Differential/Audit.
For the most part, the existing custom fields work well and can be directly generalized. There are three specific things I want to improve, though:
- Integration with ApplicationSearch: Custom fields aren't indexable. ApplicationSearch is now online and seems stable and good. D5278 provides a template for a backend which can integrate with ApplicationSearch, and ApplicationSearch solves many of the other UI problems implied by exposing custom fields into search (principally, giant pages full of query fields). Generally, I want to provide stronger builtin integration between custom fields and ApplicationSearch.
- Integration with ApplicationTransactions: Likewise, custom fields should support more native integrations with ApplicationTransactions, which are also online and seem stable and well designed.
- Selection and sorting: Selecting and sorting custom fields is a huge mess right now. I want to move this into config now that we have the UI to support it, and move away from requiring users to subclass a ton of stuff just to add a field.
For ApplicationSearch, I've adopted and generalized D5278.
For ApplicationTransactions, I haven't made any specific affordances yet.
For selection and sorting, I've partially implemented config-based selection and sorting. It will work like this:
- We add a new configuration value, like `differential.fields`. In the UI, this is a draggable list of supported fields. Fields can be reordered, and most fields can be disabled.
- We load every avialable field to populate this list. New fields will appear at the bottom.
- There are two downsides to this approach:
- If we add fields in the upstream at a later date, they will appear at the end of the list if an install has customized list order or disabled fields, even if we insert them elsewhere in the upstream.
- If we reorder fields in the upstream, the reordering will not be reflected in install which have customized the order/availability.
- I think these are both acceptable costs. We only incur them if an admin edits this config, which implies they'll know how to fix it if they want to.
- We can fix both of these problems with a straightforward configuration migration if we want to bother.
- There are numerous upsides to this approach:
- We can delete a bunch of code and replace it with simple configuration.
- In general, we don't need the "selector" classes anymore.
- Users can enable available-but-disabled fields with one click.
- Users can add fields by putting their implementations in `src/extensions/` with zero subclassing or libphutil stuff.
- Generally, it's super easy for users to understand.
This doesn't actually do anything yet and will probably see some adjustments before anything starts running it.
Test Plan: Static checks only, this code isn't reachable yet.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6147
Summary: Adds collapsing of the sidebar, also allows you to say where it goes on mobile (above or below content). ToC for example, above. General Navbar, below. Up to you.
Test Plan: Review UIExamples and Diviner.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6150
Summary: Fixes some issues with lists and tablet/mobile layouts.
Test Plan: shrink my screen
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6148
Summary: We need to make sure the symbol is always saved before the atom. I mucked this up recently and didn't catch it locally since I'd already generated the atoms.
Test Plan: Ran `bin/diviner generate` after truncating all diviner tables.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6146
Summary: Gets TOC populated for articles, at least, and fixes a few other things.
Test Plan: {F45474}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6144
Summary: Tweaks the dark, grey, and white icons to match the action-icons. Also added a home icon for navigation.
Test Plan: looked at list navs, action menus.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6142
Summary: Allow users to set a default by dragging it to the top. When they land on a page without a saved query, choose their default.
Test Plan: Hit `/paste/`, got my default results, etc.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6140
Summary:
- Use the same styles for shared operations (`drag-ghost`, `drag-dragging`).
- Move shared code into the base class.
Test Plan: Dragged around tasks and named queries.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6141
Summary: Needed to be more restrictive
Test Plan: Test maniphest and list examples.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6139
Summary:
Also you have to drop them. So drag, and then drop.
This needs some cleanup and reconciliation/generalization with the Maniphest implementation. In particular, you can't drag things to the very top right now, and they should share more CSS and more behaviors.
Test Plan:
Look I alphabetized them:
{F45286}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6136
Summary: See discussion in D6131, D6130. This turned into 35 layers of mess so throw it away and just tweak the JS to be more flexible.
Test Plan:
- Clicked "Show More Applications".
- Clicked "Show Fewer Applications".
- Edited tasks using popup dialog.
- Tried to drag tasks using pencil icon (correctly no longer works).
- Changed threads in Conpherence.
- Not sure how to actually hit the Conpherence "Load ... Threads" thing since
it seems to auto-load? But that works, at least, and the code doesn't really
care what you hit.
- Added a conpherence participant.
- Added a new calendar item.
- Poked around other menus.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6133
Summary:
See discussion in D6130. Basically, all of these should activate workflow:
<a data-sigil="workflow" href="...">...</a>
<div data-sigil="workflow">
<a href="...">...</a>
</div>
<form data-sigil="workflow" action="...">...</form>
<div data-sigil="workflow">
<form action="...">...</form>
</div>
The only case where we don't want to activate workflow is this one:
<form data-sigil="workflow">
<a href="...">...</a>
</form>
Here, the form should workflow but the `<a />` should not.
These cases aren't really covered:
// Undefined no matter where "workflow" is because it's nonsense.
<a><a>...</a></a>
// As above except like a million times more dumb.
<form><form>...</form></form>
// This one is ambiguous. The <a /> will currently workflow. We don't do
// this anywhere and probably never will. If we want a different rule we
// can cross that bridge when we come to it.
<div data-sigil="workflow">
<form action="...">
<a href="...">...</a>
</form>
</div>
Test Plan: Clicked/submitted some things with workflow.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6131
Summary: This adds examples and abstracts out CSS for common nav re-use.
Test Plan: Tested DocumentExample and ListExample
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6138
Summary: I made changes here recently to improve robustness in the presence of missing files, but accidentally caused the results to re-key. Some callers depend on the mapping, and every other query is consistent about it. Restore the original behavior.
Test Plan: `Pnnn` works again in remarkup.
Reviewers: dctrwatson, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6134
Summary:
Ref T3320. Request from Dropbox. You can currently prefill "title" and some other stuff (including most of the fields by using templates) but there's no way to prefill description and assigned to right now.
- Allow "Description" to be prefilled with a string ("description=...").
- Allow "Assigned To" to be prefilled with a user rather than a PHID ("assign=username").
Test Plan:
Hit `/maniphest/task/create/?assign=epriestley&description=derpderp&title=blarpblarp` locally and got prefills.
{F45259}
Reviewers: chad, deuresti
Reviewed By: chad
CC: aran, euresti
Maniphest Tasks: T3320
Differential Revision: https://secure.phabricator.com/D6132
Summary:
This diff covers a bit of ground.
- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView
Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.
Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6130
Summary: At the global level, truncate emails at a user-configured size.
Test Plan: Untested, as I could not get PHP to send emails on my box, but if you can this should be very easy to test. Just set the max size to something like .001 kilobytes and make sure it does the right thing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T1392
Differential Revision: https://secure.phabricator.com/D6118
Summary:
Applications come with builtin queries, but users might want to get rid of them. Allow users to disable named queries if they prefer.
This has one funky behavior, which is that the first time you disable a named query it goes to the top of your list. That will be fixed in the next diff, which will make them reorderable.
Test Plan: Added/edited/removed named queries, disabled/enabled builtin named queries.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6128
Summary:
I want to use draggable lists in at least three other interfaces:
- (Today) Reorganizing named search queries.
- (Today) Reorganizing custom fields.
- (Future) Dragging tasks around on boards.
This mostly generalizes the drag-and-drop code in Maniphest's task list. It isn't a total generalization and will need some more tweaking (for example, Maniphest's list is unusual in that the user can't drag items to the top of the list), but it substantially separates the Maniphest-specific behaviors from the general dragging behaviors.
This diff causes no functional changes.
Test Plan: Dragged and dropped tasks in Maniphest.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6124
Summary: Ref T988. Mostly backend changes, with a very rough frontend on top of them. See Conpherence discussion.
Test Plan: {F45010}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6113
