Summary:
Ref T13552. This older view mostly duplicates other code and has only two callsites:
- The "Commits" section of user profile pages.
- The "Ambiguous Hash" page when you visit a commit hash page which is an ambiguous prefix of two or more commit hashes.
Replace both with "DiffusionCommitGraphView".
Test Plan:
- Visited profile page, clicked "Commits".
- Visited an ambiguous hash page (`rPbd3c23`).
Maniphest Tasks: T13552
Differential Revision: https://secure.phabricator.com/D21412
Summary: On the "New User" web workflow, if you use an invalid email address, you get a failure with an empty message.
Test Plan:
- Before: Tried to create a new user with address "asdf". Got no specific guidance.
- After: Got specific guidance about email address formatting and length.
Differential Revision: https://secure.phabricator.com/D21264
Summary:
Ref T13515. Currently, opening a file to a particular line in an external editor relies on replacing "%l" with "%l" (which is escaped as "%25l") on the server, and then replacing "%25l" with the line number on the client. This will fail if the file path (or any other variable) contains "%l" in its unencoded form.
The parser also can't identify invalid variables.
Pull the parser out, formalize it, and make it generate an intermediate representation which can be sent to the client and reconstituted.
(This temporarily breaks Diffusion and permanently removes the weird, ancient integration in Dark Console.)
Test Plan:
- Added a bunch of tests for the actual parser.
- Used "Open in Editor" in Differential.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21143
Summary: Ref T13515. No callsites actually use this, most editors don't support it, it doesn't seem terribly useful for the ones that do, it makes template-based APIs for line-number substitution complicated, and we can probably just loop on `window.open()` anyway.
Test Plan: Grepped for affected symbols, found no more references. Loaded settings page, saw no more setting.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21142
Summary:
Depends on D21018. Ref T13493. Ref T6703. The "ExternalAccount" table has a unique key on `<accountType, accountDomain, accountID>` but this no longer matches our model of reality and changes in this sequence end writes to `accountID`.
Remove this key.
Then, remove all readers of `accountType` and `accountDomain` (and all nontrivial writers) because none of these callsites are well-aligned with plans in T6703.
This change has no user-facing impact today: all the rules about linking/unlinking/etc remain unchanged, because other rules currently prevent creation of more than one provider with a given "accountType".
Test Plan:
- Linked an OAuth1 account (JIRA).
- Linked an OAuth2 account (Asana).
- Used `bin/auth refresh` to cycle OAuth tokens.
- Grepped for affected symbols.
- Published an Asana update.
- Published a JIRA link.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493, T6703
Differential Revision: https://secure.phabricator.com/D21019
Summary:
Depends on D21015. When we sync an external account and get a list of account identifiers, write them to the database.
Nothing reads them yet and we still write "accountId", this just prepares us for reads.
Test Plan: Linked, refreshed, unlinked, and re-linked an external account. Peeked at the database and saw a sensible-looking row.
Differential Revision: https://secure.phabricator.com/D21016
Summary: Depends on D21014. Ref T13493. Make these objects all use destructible interfaces and destroy sub-objects appropriately.
Test Plan:
- Used `bin/remove destroy --trace ...` to destroy a provider, a user, and an external account.
- Observed destruction of sub-objects, including external account identifiers.
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21015
Summary:
Depends on D21010. Ref T13493. External accounts may have multiple different unique identifiers, most often when v1 of the API makes a questionable choice (and provies a mutable, non-unique, or PII identifier) and v2 of the API uses an immutable, unique, random identifier.
Allow Phabricator to store multiple identifiers per external account.
Test Plan: Storage only, see followup changes.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13493
Differential Revision: https://secure.phabricator.com/D21011
Summary:
See PHI1558. Ref T11860. Ref T13444. I partly implemented PHIDs for "UserEmail" objects, but they can't load on their own so you can't directly `bin/remove destroy` them yet.
Allow them to actually load by implementing "PolicyInterface".
Addresses are viewable and editable by the associated user, unless they are a bot/list address, in which case they are viewable and editable by administrators (in preparation for T11860). This has no real impact on anything today.
Test Plan:
- Used `bin/remove destroy <phid>` to destroy an individual email address.
- Before: error while loading the object by PHID in the query policy layer.
- After: clean load and destroy.
Maniphest Tasks: T13444, T11860
Differential Revision: https://secure.phabricator.com/D20927
Summary:
Ref T13444. Currently, many mutations to users and email addresses (particularly: user creation; and user and address destruction) do not propagate properly to repository identities.
Add hooks to all mutation workflows so repository identities get rebuilt properly when users are created, email addresses are removed, users or email addresses are destroyed, or email addresses are reassigned.
Test Plan:
- Added random email address to account, removed it.
- Added unassociated email address to account, saw identity update (and associate).
- Removed it, saw identity update (and disassociate).
- Registered an account with an unassociated email address, saw identity update (and associate).
- Destroyed the account, saw identity update (and disassociate).
- Added address X to account A, unverified.
- Invited address X.
- Clicked invite link as account B.
- Confirmed desire to steal address.
- Saw identity update and reassociate.
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20914
Summary:
Ref T13444. To interact meaningfully with "DestructionEngine", objects need a PHID. The "UserEmail" object currently does not have one (or a real "Query").
Provide basic PHID support so "DestructionEngine" can interact with the object more powerfully.
Test Plan:
- Ran migrations, checked data in database, saw sensible PHIDs assigned.
- Added a new email address to my account, saw it get a PHID.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20913
Summary: Ref T13444. Prepare to hook identity updates when user email addreses are destroyed.
Test Plan:
- Destroyed a user with `bin/remove destroy ... --trace`, saw email deleted.
- Destroyed an email from the web UI, saw email deleted.
Maniphest Tasks: T13444
Differential Revision: https://secure.phabricator.com/D20912
Summary:
Fixes T13446. Currently, the validation logic here rejects a rename like "alice" to "ALICE" (which changes only letter case) but this is a permissible rename.
Allow collisions that collide with the same user to permit this rename.
Also, fix an issue where an empty rename was treated improperly.
Test Plan:
- Renamed "alice" to "ALICE".
- Before: username collision error.
- After: clean rename.
- Renamed "alice" to "orange" (an existing user). Got an error.
- Renamed "alice" to "", "!@#$", etc (invalid usernames). Got sensible errors.
Maniphest Tasks: T13446
Differential Revision: https://secure.phabricator.com/D20890
Summary: Fixes T13420. Allow installs to provide username change instructions if there's someone you should contact to get this done.
Test Plan: {F6885027}
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20828
Summary: Ref T13420. This workflow currently dead-ends for non-administrators. Instead, provide explanatory text.
Test Plan:
- Clicked "Change Username" as an administrator, same workflow as always.
- Clicked "Change Username" as a non-administrator, got explanatory text.
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20827
Summary:
Ref T13420. These warnings are currently more severe than they need to be; weaken them.
Among other cases, the upstream supports and encourages changing usernames when users change human names.
The login/password instructions are also out of date since sessions were decoupled from usernames about a year ago.
Test Plan: Hit dialog as an administrator
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20826
Summary:
Ref T13393. See some previous discussion in T13366.
Caching is hard and all approaches here have downsides, but the request cache likely has fewer practical downsides for this kind of policy check than other approaches. In particular, the grant approach (at least, as previously used in Phortune) has a major downside that "Query" classes can no longer fully enforce policies.
Since Phortune no longer depends on grants and they've now been removed from instances, drop the mechanism completely.
Test Plan: Grepped for callsites, found none.
Maniphest Tasks: T13393
Differential Revision: https://secure.phabricator.com/D20754
Summary: Ref T13382. Currently, the "Make Administrator" action in the web UI does state-based MFA. Convert it to one-shot MFA.
Test Plan: Empowered and unempowered a user from the web UI, got one-shot MFA'd. Empowered a user from the CLI, no MFA issues.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20729
Summary:
Ref T13382.
- Remove "bin/people profileimage" which previously generated profile image caches but now feels obsolete.
- Replace it with "bin/user", with "enable" and "empower" flows. This command is now focused on regaining access to an install after you lock your keys inside.
- Document the various ways to unlock objects and accounts from the CLI.
Test Plan:
- Ran `bin/user enable` and `bin/user empower` with various flags.
- Grepped for `people profileimage` and found no references.
- Grepped for `bin/people` and found no references.
- Read documentation.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20724
Summary:
Fixes T13370. We currently show an "Award Badge" button conditionally, based on whether the viewer can award any badges or not.
The query to test this may overheat and this pattern isn't consistent with other UI anyway. Stop doing this test.
Test Plan:
- Created 12 badges.
- As a user who could not edit any of the badges, viewed the "Badges" section of a user profile.
Maniphest Tasks: T13370
Differential Revision: https://secure.phabricator.com/D20702
Summary:
Fixes T8830. Fixes T13364.
- The inability to destroy objects from the web UI is intentional. Make this clear in the messaging, which is somewhat out of date and partly reflects an earlier era when things could be destroyed.
- `bin/remove destroy` can't rewind time. Document expectations around the "put the cat back in the bag" use case.
Test Plan: Read documentation, clicked through both workflows.
Maniphest Tasks: T13364, T8830
Differential Revision: https://secure.phabricator.com/D20694
Summary:
Depends on D20673. Ref T13343. Since we're now putting log IDs in email, make the UI a little better for working with log IDs.
Some day, this page might have actions like "report this as suspicious" or whatever, but I'm not planning to do any of that for now.
Test Plan: {F6608631}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20674
Summary:
Depends on D20672. Ref T13343. When a user requests an account access link via email:
- log it in the activity log; and
- reference the log in the mail.
This makes it easier to ban users misusing the feature, provided they're coming from a single remote address, and takes a few steps down the pathway toward a button in the mail that users can click to report the action, suspend account recovery for their account, etc.
Test Plan:
- Requested an email recovery link.
- Saw request appear in the user activity log.
- Saw a reference to the log entry in the mail footer.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20673
Summary: Depends on D20671. Ref T13343. Now that log types are modular, provide a datasource/tokenizer for selecting them since we already have a lot (even after I purged a few in D20670) and I'm planning to add at least one more ("Request password reset").
Test Plan: {F6608534}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20672
Summary:
Depends on D20670. Ref T13343. The user activity message log types are currently hard-coded, so only upstream code can really use the log construct.
Under the theory that we're going to keep this log around going forward (just focus it a little bit), modularize things so the log is extensible.
Test Plan:
Grepped for `UserLog::`, viewed activity logs in People and Settings.
(If I missed something here -- say, misspelled a constant -- the effect should just be that older logs don't get a human-readable label, so stakes are very low.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20671
Summary: Fixes T13349. If the user profile page feed query overheats, it currently takes the whole page with it. Contain the blast to a smaller radius.
Test Plan: {F6633322}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13349
Differential Revision: https://secure.phabricator.com/D20678
Summary:
Depends on D20669. Ref T13343. Currently, the user activity log includes a number of explicit administrative actions which some administrator (not a normal user or a suspicious remote address) takes. In most/all cases, these changes are present in the user profile transaction log too, and that's //generally// a better place for them (for example, it doesn't get GC'd after a couple months).
Some of these are so old that they have no writers (like DELETE and EDIT). I'd generally like to modernize this a bit so we can reference it in email (see T13343) and I'd like to modularize the event types as part of that -- partly, cleaning this up makes that modularization easier.
There's maybe some hand-wavey argument that administrative vs non-administrative events could be related and might be useful to see in a single log, but I can't recall a time when that was actually true, and we could always build that kind of view later by just merging the two log sources, or by restoring double-writes for some subset of events. In practice, I've used this log mostly to look for obvious red flags when users report authentication difficulty (e.g., many unauthorized login attempts), and removing administrative actions from the log is only helpful in that use case.
Test Plan: Grepped for all the affected constants, no more hits in the codebase.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20670
Summary:
Depends on D20665. Ref T13343. We support CAPTCHAs on the "Forgot password?" flow, but not everyone configures them (or necessarily should, since ReCAPTCHA is a huge external dependency run by Google that requires you allow Google to execute JS on your domain) and the rate at which any reasonable user needs to take this action is very low.
Put a limit on the rate at which account recovery links may be generated for a particular account, so the worst case is a trickle of annoyance rather than a flood of nonsense.
Test Plan: {F6607794}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20666
Summary:
Depends on D20664. Ref T13343. There's a reasonable value for the default "Email Login" auth message (generic "you reset your password" text) that installs may reasonably want to replace. Add support for a default value.
Also, since it isn't completely obvious where this message shows up, add support for an extended description and explain what's going on in more detail.
Test Plan:
- Viewed message detail page, saw more detailed information.
- Sent mail (got default), overrode message and sent mail (got custom message), deleted message (got default again).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20665
Summary:
Depends on D20662. Ref T13343. Installs may reasonably want to change the guidance users receive in "Email Login"/"Forgot Password" email.
(In an upcoming change I plan to supply a piece of default guidance, but Auth Messages need a few tweaks for this.)
There's probably little reason to provide guidance on the "Set Password" flow, but any guidance one might issue on the "Email Login" flow probably doesn't make sense on the "Set Password" flow, so I've included it mostly to make it clear that this is a different flow from a user perspective.
Test Plan:
- Set custom "Email Login" and "Set Password" messages.
- Generated "Email Login" mail by using the "Login via email" link on the login screen.
- Generated "Set Password" email by trying to set a password on an account with no password yet.
- Saw my custom messages in the resulting mail bodies.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20663
Summary:
Ref T13343. This makes "Password Reset" email a little more consistent with other modern types of email. My expectation is that this patch has no functional changes, just organizes code a little more consistently.
The new `setRecipientAddress()` mechanism deals with the case where the user types a secondary (but still verified) address.
Test Plan:
- Sent a normal "login with email" email.
- Sent a "login with email to set password" email by trying to set a password on an account with no password yet.
- Tried to email reset a bot account (no dice: they can't do web logins so this operation isn't valid).
- Tested existing "PeopleMailEngine" subclasses:
- Created a new user and sent a "welcome" email.
- Renamed a user and sent a "username changed" email.
- Reviewed all generated mail with `bin/mail list-outbound`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20662
Summary: Depends on D20384. Ref T13275. A bunch of this code got converted but I missed some callsites that aren't reached directly from the menu.
Test Plan:
- Visited each controller, saw actual pages instead of menu construction fatals.
- Grepped for `getProfileMenu()`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20385
Summary:
Depends on D20357. Ref T13275. Now that there's a stronger layer between "stuff in the database" and "stuff on the screen", these subclasses all need to emit intermediate objects instead of raw, HTML-producing view objects.
This update is mostly mechanical.
Test Plan:
- Viewed Home, Favorites, Portals, User Profiles, Project Profiles.
- Clicked each item on each menu/profile type.
- Added every (I think?) type of item to a menu and clicked them all.
- Grepped for obsolete symbols (`newNavigationMenuItems`, `willBuildNavigationItems`).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275
Differential Revision: https://secure.phabricator.com/D20358
Summary:
Depends on D20355. Ref T13275. Ref T13247. Currently, "Hamburger" menus are not automatically built from navigation menus. However, this is (I'm almost completely sure?) a reasonable and appropriate default behavior, and saves us some code around profile menus.
With this rule in place, we can remove `setApplicationMenu()` and `getApplicationMenu()` from `StandardPageView`, since they have no callers.
This also updates a lot of profile menu callsites to a new API which is added in the next change.
Test Plan: See the next two changes.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13275, T13247
Differential Revision: https://secure.phabricator.com/D20356
Summary:
Fixes T13273. This element is a bit weird, but I think I fixed it without breaking anything.
The CSS is used by project hovercards and user hovercards, but they each have a class which builds mostly-shared-but-not-really-identical CSS, instead of having a single `View` class with modes. So I'm not 100% sure I didn't break something obscure, but I couldn't find anything this breaks.
The major issue is that all the text content has "position: absolute". Instead, make the image "absolute" and the text actual positioned content. Then fix all the margins/padding/spacing/layout and add overflow. Seems to work?
Plus: hide availability for disabled users, for consistency with D20342.
Test Plan:
Before:
{F6320155}
After:
{F6320156}
I think this is pixel-exact except for the overflow behavior.
Also:
- Viewed some other user hovercards, including a disabled user. They all looked unchanged.
- Viewed some project hovercards. They all looked good, too.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13273
Differential Revision: https://secure.phabricator.com/D20344
Summary:
See downstream <https://phabricator.wikimedia.org/T138723>. That suggestion is a little light on details, but I basically agree that showing "Availability: Available" on disabled user profiles is kind of questionable/misleading.
Just hide event information on disabled profiles, since this doesn't seem worth building a special "Availability: Who Knows, They Are Disabled, Good Luck" disabled state for.
Test Plan: Looked at disabled and non-disabled user profiles, saw Calendar stuff only on the former.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20342
Summary:
Depends on D20291. Ref T13259. Move all the simple cases (where paging depends only on the partial object and does not depend on keys) to a simple wrapper.
This leaves a smaller set of more complex cases where we care about external data or which keys were requested that I'll convert in followups.
Test Plan: Poked at things, but a lot of stuff is still broken until everything is converted.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13259
Differential Revision: https://secure.phabricator.com/D20292
Summary:
Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers).
They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places.
(The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.)
Test Plan:
{F6212044}
{F6212045}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20173
Summary:
Depends on D20113. Ref T6703. Continue moving toward a future where multiple copies of a given type of provider may exist.
Switch MFA from session-MFA at the start to one-shot MFA at the actual link action.
Add one-shot MFA to the unlink action. This theoretically prevents an attacker from unlinking an account while you're getting coffee, registering `alIce` which they control, adding a copy of your profile picture, and then trying to trick you into writing a private note with your personal secrets or something.
Test Plan: Linked and unlinked accounts. Refreshed account. Unlinked, then registered a new account. Unlinked, then relinked to my old account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20117
Summary:
Depends on D20111. Ref T6703. Currently, each ExternalAccount row is tied to a provider by `providerType` + `providerDomain`. This effectively prevents multiple providers of the same type, since, e.g., two LDAP providers may be on different ports on the same domain. The `domain` also isn't really a useful idea anyway because you can move which hostname an LDAP server is on, and LDAP actually uses the value `self` in all cases. Yeah, yikes.
Instead, just bind each account to a particular provider. Then we can have an LDAP "alice" on seven different servers on different ports on the same machine and they can all move around and we'll still have a consistent, cohesive view of the world.
(On its own, this creates some issues with the link/unlink/refresh flows. Those will be updated in followups, and doing this change in a way with no intermediate breaks would require fixing them to use IDs to reference providerType/providerDomain, then fixing this, then undoing the first fix most of the way.)
Test Plan: Ran migrations, sanity-checked database. See followup changes for more comprehensive testing.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20112
Summary:
Ref T6703. Currently, when you create an account on a new install, we prompt you to select a password.
You can't actually use that password unless you set up a password provider, and that password can't be associated with a provider since a password provider won't exist yet.
Instead, just don't ask for a password: create an account with a username and an email address only. Setup guidance points you toward Auth.
If you lose the session, you can send yourself an email link (if email works yet) or `bin/auth recover` it. This isn't really much different than the pre-change behavior, since you can't use the password you set anyway until you configure password auth.
This also makes fixing T9512 more important, which I'll do in a followup. I also plan to add slightly better guideposts toward Auth.
Test Plan: Hit first-time setup, created an account.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: revi
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20111
Summary: Depends on D20109. Ref T6703. This flow was contributed in 2012 and I'm not sure it ever worked, or at least ever worked nondestructively. For now, get rid of it. We'll do importing and external sync properly at some point (T3980, T13190).
Test Plan: Grepped for `ldap/`, grepped for controller.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20110
Summary:
Depends on D20122. Fixes T8029. Adds an "Approve User" action to the "Manage" page.
Users are normally approved from the "Approval Queue", but if you click into a user's profile to check them out in more detail it kind of dead ends you right now. I've occasionally hit this myself, and think this workflow is generally reasonable enough to support upstream.
Test Plan: {F6193742}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T8029
Differential Revision: https://secure.phabricator.com/D20123
Summary: Depends on D20108. Ref T6703. Update this outdated callsite to a more modern appraoch.
Test Plan: Destroyed a user with `bin/remove destroy`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20109
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary:
Depends on D20100. Ref T7732. Ref T13244. This is a bit of an adventure.
Long ago, passwords were digested with usernames as part of the salt. This was a mistake: it meant that your password becomes invalid if your username is changed.
(I think very very long ago, some other hashing may also have used usernames -- perhaps session hashing or CSRF hashing?)
To work around this, the "username change" email included a one-time login link and some language about resetting your password.
This flaw was fixed when passwords were moved to shared infrastructure (they're now salted more cleanly on a per-digest basis), and since D18908 (about a year ago) we've transparently upgraded password digests on use.
Although it's still technically possible that a username change could invalidate your password, it requires:
- You set the password on a version of Phabricator earlier than ~2018 Week 5 (about a year ago).
- You haven't logged into a version of Phabricator newer than that using your password since then.
- Your username is changed.
This probably affects more than zero users, but I suspect not //many// more than zero. These users can always use "Forgot password?" to recover account access.
Since the value of this is almost certainly very near zero now and declining over time, just get rid of it. Also move the actual mail out of `PhabricatorUser`, ala the similar recent change to welcome mail in D19989.
Test Plan: Changed a user's username, reviewed resulting mail with `bin/mail show-outbound`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244, T7732
Differential Revision: https://secure.phabricator.com/D20102
Summary:
Ref T13244. This story publishes to the feed (and I think that's reasonable and desirable) but doesn't render as nicely as it could.
Improve the rendering.
(See T9233 for some context on why we render stories like this one in this way.)
Test Plan: {F6184490}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13244
Differential Revision: https://secure.phabricator.com/D20097
Summary:
Depends on D20035. Ref T13222.
- Allow individual transactions to request one-shot MFA if available.
- Make "change username" request MFA.
Test Plan:
- Renamed a user, got prompted for MFA, provided it.
- Saw that I no longer remain in high-security after performing the edit.
- Grepped for other uses of `PhabricatorUserUsernameTransaction`, found none.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20036
