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
Summary: Ref T13222. Providers can now be deprecated (existing factors still work, but users can't add new factors for the provider) or disabled (factors stop working, also can't add new ones).
Test Plan:
- Enabled, deprecated, and disabled some providers.
- Viewed provider detail, provider list.
- Viewed MFA settings list.
- Verified that I'm prompted for enabled + deprecated only at gates.
- Tried to disable final provider, got an error.
- Hit the MFA setup gate by enabling "Require MFA" with no providers, got a more useful message.
- Immediately forced a user to the "MFA Setup Gate" by disabling their only active provider with another provider enabled ("We no longer support TOTP, you HAVE to finish Duo enrollment to continue starting Monday.").
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20031
Summary: Fixes T13239. See that task for discussion.
Test Plan: Tried to send welcome mail with no "Welcome" message.
Maniphest Tasks: T13239
Differential Revision: https://secure.phabricator.com/D20001
Summary:
Depends on D19994. See PHI1027. If an install has customized the "Welcome Mail" message, include it in welcome mail. A special custom message from the profile screen overrides it, if provided.
(I fiddled with putting the custom message as "placeholder" text in the remarkup area as a hint, but newlines in "placeholder" text appear to have issues in Safari and Firefox. I think this is probably reasonably clear as-is.)
Make both render remarkup-into-text so things like links work properly, as it's reasonably likely that installs will want to link to things.
Test Plan:
- With custom "Welcome Mail" text, sent mail with no custom override (got custom text) and a custom override (got overridden text).
- Linked to some stuff, got sensible links in the mail (`bin/mail show-outbound`).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19995
Summary:
See PHI1027. Currently, we allow you to customize invite email, but not most other types of email (approve, welcome). As a step forward, also allow welcome email to be customized with a message.
I considered separating the custom text from the main text with something heavyhanded ("alice added this custom message:") or a beautiful ASCII art divider like one of these:
https://www.asciiart.eu/art-and-design/dividers
...but nothing truly sung to me.
This only works on the profile flow for now. I'm planning to let you set a default message. I may or may not let you customize from "Create New User", seems like the default message probably covers most of that. Probably won't touch `scripts/user/add_user.php` since that's not really exactly super supported.
Test Plan:
Sent mail with and without custom messages, reviewed it with `bin/mail show-outbound`.
{F6137410}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19991
Summary:
See PHI1027. Currently, the "Welcome" mail always tells users to set a password. This definitely isn't helpful if an install doesn't have password auth enabled.
We can't necessarily guess what they're supposed to do, so just give them generic instructions ("set up your account"). Upcoming changes will give administrators more control over the mail content.
Test Plan: Sent both versions of the mail, used `bin/mail show-outbound` to inspect them for correctness.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19990
Summary:
Ref PHI1027. Currently, `PhabricatorUser` has a couple of mail-related methods which shouldn't really be there in the long term. Immediately, I want to make some adjusments to the welcome email.
Move "Welcome" mail generation to a separate class and consolidate all the error handling. (Eventually, "invite" and "verify address" email should move to similar subclasses, too.) Previously, a bunch of errors/conditions got checked in multiple places.
The only functional change is that we no longer allow you to send welcome mail to disabled users.
Test Plan:
- Used "Send Welcome Mail" from profile pages to send mail.
- Hit "not admin", "disabled user", "bot/mailing list" errors.
- Used `scripts/user/add_user.php` to send welcome mail.
- Used "Create New User" to send welcome mail.
- Verified mail with `bin/mail show-outbound`. (Cleaned up a couple of minor display issues here.)
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19989
Summary:
Ref T12509.
- Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
- Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
- Extract the CSRF logic from `PhabricatorUser` to a standalone engine.
I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.
For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.
It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).
This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.
Test Plan:
- As a logged-in user, submitted forms normally (worked).
- As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
- As a logged-out user, hit the success and error cases.
- Visually inspected tokens for correct format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D19946
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
