Summary:
The expression `$all_tags[$tag]['count']` does not do anything. Reading the surrounding code, intention is to display a "Common" section on `/settings/panel/emailpreferences/` listing MailTags defined by at least two different applications. (This is currently not the case anyway as Phorge prefixes all MailTags with their corresponding application but might be a future use case.)
This change fixes the logic accordingly.
Closes T15874
Test Plan: Apply the patch; replace the value of a random `const MAILTAG_*` line in the codebase with the value of another random `const MAILTAG_*` line in the codebase to make it "common", now see that `/settings/panel/emailpreferences/` displays this MailTag in a "Common" section on top of the options.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15874
Differential Revision: https://we.phorge.it/D25727
Summary:
`::class` is available since PHP 5.5 (5.5 is a minimal requirement by Phorge): https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basic.class.class
It makes finding code using IDEs easier; see discussion in D25500.
Thus replace all string return values with returning the `::class` constant instead, with one exception: 'PhabricatorSettingApplication' in `PhabricatorUserPreferencesSearchEngine.php` does not exist and makes arc lint fail so this string remained unchanged.
Also note that two occurrences were wrapped in `pht()` for reasons I do not know.
List of functions whose return value get updated in this code change:
* getApplicationClassName()
* getAdapterApplicationClass()
* getDatasourceApplicationClass()
* getEditorApplicationClass()
* getEngineApplicationClass()
* getPHIDTypeApplicationClass()
* getQueryApplicationClass()
cf. T15158
Test Plan: Too broad - click around, basically.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15158
Differential Revision: https://we.phorge.it/D25524
Summary:
Add `getQueryApplicationClass()` to all `*TransactionQuery.php` classes similar to other `*Query.php` classes having the same function, and make the parent function in `PhabricatorApplicationTransactionQuery.php` abstract.
In the future, this will enable excluding transaction query results based on their underlying application (for example if an application has been uninstalled) to mitigate the problem of overheated search results. See https://we.phorge.it/T15642 for context.
The only callers of `getQueryApplicationClass()` are in `src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php` and `src/applications/policy/__tests__/PhabricatorPolicyTestCase.php`.
See T15642
Test Plan:
Patch changes only one existing code place, thus check if related pages still work as expected:
* Go to http://phorge.localhost/feed/
* Go to http://phorge.localhost/feed/transactions/
* On http://phorge.localhost/feed/transactions/ , click `Edit Query` and set `Object Types` to `Application` etc.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15642
Differential Revision: https://we.phorge.it/D25500
Summary:
It's useless without SMS support and only exposed to the user themselves.
Closes T15486
Test Plan:
Before and after applying this patch,
* Try to access the list of your contact numbers at `/settings/panel/contact/`
* Try to access an existing, previously created contact number at `/auth/contact/1/`
* Try to add a contact number at `/auth/contact/edit/`
* Go to e.g. `/settings/panel/datetime` and check the "Authentication" section in the left sidebar for {nav icon=hashtag, name=Contact Numbers}
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15486
Differential Revision: https://we.phorge.it/D25452
Summary:
This applies a suggestion initially proposed in
https://we.phorge.it/D25420#12264
Test Plan:
- Change your browser/system timezone to differ from your Phorge profile timezone
- Click the notice that Phorge shows at the bottom left about the timezone mismatch
- Confirm that the form text has been changed as per the diff in this revision
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: aklapper, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25458
Summary:
When there is a new timezone conflict, you will be able to ignore it with a checkbox.
Fix T15349
Preview:
{F343198}
Test Plan: Having a conflicting timezone, click the notification so the usual popup appears. There is a checkbox, leave it checked to ignore the current conflict, uncheck to manually resolve the conflict by selecting one of the available timezones.
Reviewers: O1 Blessed Committers, valerio.bozzolan, avivey
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: speck, waldyrious, avivey, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15349
Differential Revision: https://we.phorge.it/D25420
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15388
Test Plan:
Applied these two changes; afterwards managed to add a 2FA factor and `/settings/user/username/page/multifactor/?id=1` correctly listed mys Authentication Factors.
Additional tests:
- Unset any eventual personal MFA
- Setup a personal MFA
- Login/Logout using the MFA
- Remove a personal MFA
- Setup a personal enroll message from /auth/mfa/1/
- Setup a personal MFA
- Login/Logout using the MFA
- (then cleanup removing your test MFA)
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15388
Differential Revision: https://we.phorge.it/D25219
Summary:
Since PHP 8.1, passing a null string to `ltrim(string $string)` is deprecated.
Thus we make sure that `$request->getStr('email')` does not return null as default.
Closes T15376
Test Plan: Applied this change, afterwards repeated the steps to add a new email address on `/settings/panel/email/`. This time, it's possible to close the "Verification Email Sent" and the page `/settings/panel/email/` renders and lists the new email address.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15376
Differential Revision: https://we.phorge.it/D25210
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15377
Test Plan: Applied this change, afterwards repeated the steps to add a new email address on `/settings/panel/email/` and left the "Email" field empty. This time, after selecting the "Save" button, the error message "Email is required." is displayed in the "New Address" overlay dialog.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15377
Differential Revision: https://we.phorge.it/D25211
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15310
Test Plan:
Applied this change (on top of D25144, D25145, D25146, D25147, D25151,
D25152, D25153) and `/settings/panel/editor/` correctly rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15310
Differential Revision: https://we.phorge.it/D25160
Summary:
Ref T13679. In D16983, global settings objects were given an exception to let logged-out users see them, even on installs with no "public" user role.
This exception is too broad and grants everyone all capabilities, not just "CAN_VIEW". In particular, it incorrectly grants "CAN_EDIT", so any user can edit global settings defaults.
Restrict this grant to "CAN_VIEW".
Test Plan:
- As a non-administrator, tried to edit global settings.
- Before: could.
- After: could not.
Maniphest Tasks: T13679
Differential Revision: https://secure.phabricator.com/D21811
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Viewed "remarkup.process" Conduit method API page.
- Viewed URIs in a Diffusion repository.
- Viewed editor protocol configuration in Settings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21772
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Ran `bin/mail`.
- Uninstalled and reinstalled an application.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21770
Summary: This supports the IntelliJ IDEA editor.
Test Plan:
- Looked at the editor settings panel, saw "idea://".
- Set my editor pattern to "idea://a?b".
- (Did not actually install IntelliJ IDEA.)
Differential Revision: https://secure.phabricator.com/D21206
Summary:
Ref T13516. Deletes all old filetree / flex / active / collapse nav code in favor of the new code.
Restores the inline tips in the path tree.
Test Plan: {F7374175}
Maniphest Tasks: T13516
Differential Revision: https://secure.phabricator.com/D21154
Summary: Ref T13515. External editor URIs currently depend on repositories having callsigns, but callsigns are no longer required. Add some variables to support configuring this feature for repositories that do not have callsigns.
Test Plan: Changed settings to use new variables, saw links generate appropriately.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21147
Summary:
Ref T13515.
- Previously valid editor URIs may become invalid without being changed (if an administrator removes a protocol from the list, for example), but this isn't explained very well. Show an error on the settings page if the current value isn't usable.
- Generate a list of functions from an authority in the parser.
- Generate a list of protocols from configuration.
Test Plan: {F7370872}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21146
Summary:
Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI.
This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI.
Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance:
- Set a cookie on the redirect which identifies the form we just saved.
- On page startup: if this cookie exists, save the value and clear it.
- If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page.
This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner?
Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly.
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21144
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:
Ref T13515. It's not intuitive that these settings are "Display Preferences", even thought they're intenrally related to some of the other display preferences.
Give them a separate group.
Test Plan: {F7370500}
Maniphest Tasks: T13515
Differential Revision: https://secure.phabricator.com/D21140
Summary: Fixes T13405. We currently offer non-global custom saved queries here, but this doesn't make sense as a global default setting.
Test Plan: Saved a global search query, edited global search settings, no longer saw the non-global query as an option.
Maniphest Tasks: T13405
Differential Revision: https://secure.phabricator.com/D20793
Summary: Fixes T13405. The default behavior of the global search bar isn't currently configurable, but can be made configurable fairly easily.
Test Plan: Changed setting as an administrator, saw setting reflected as a user with no previous preference. As a user with an existing preference, saw preference retained.
Maniphest Tasks: T13405
Differential Revision: https://secure.phabricator.com/D20787
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: Depends on D20668. Ref T13343. Just an easy cleanup/simplification while I'm here.
Test Plan: `grep` for `getActionConstant()`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20669
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-when-logging-in-with-mfa/2828>. The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes.
These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`.
Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20587
Summary:
See downstream <https://phabricator.wikimedia.org/T902>. Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores.
Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation).
There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary.
Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20559
Summary:
Ref T13263. Two minor issues:
- The "reconcile" dialog shows the wrong sign because JS signs differ from normal signs (for example, PST or PDT or whatever we're in right now is shown as "UTC+7", but should be "UTC-7").
- The big dropdown of possible timezones lumps "UTC+X:30" timezones into "UTC+X".
Test Plan:
- Reconciled "America/Nome", saw negative UTC offsets for "America/Nome" and "America/Los_Angeles" (previously: improperly positive).
- Viewed the big timzone list, saw ":30" and ":45" timezones grouped/labeled more accurately.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13263
Differential Revision: https://secure.phabricator.com/D20314
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
Depends on D20119. Fixes T9512. When you don't have a password on your account, the "Password" panel in Settings is non-obviously useless: you can't provide an old password, so you can't change your password.
The correct remedy is to "Forgot password?" and go through the password reset flow. However, we don't guide you to this and it isn't really self-evident.
Instead:
- Guide users to the password reset flow.
- Make it work when you're already logged in.
- Skin it as a "set password" flow.
We're still requiring you to prove you own the email associated with your account. This is a pretty weak requirement, but maybe stops attackers who use the computer at the library after you do in some bizarre emergency and forget to log out? It would probably be fine to just let users "set password", this mostly just keeps us from having two different pieces of code responsible for setting passwords.
Test Plan:
- Set password as a logged-in user.
- Reset password on the normal flow as a logged-out user.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: revi
Maniphest Tasks: T9512
Differential Revision: https://secure.phabricator.com/D20120
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 D20112. Ref T6703. When you go to unlink an account, unlink it by ID. Crazy!
Test Plan: Unlinked and relinked Google accounts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20113
Summary:
Ref T13250. A handful of callsites are doing `getRequestURI()` + `setQueryParams(array())` to get a bare request path.
They can just use `getPath()` instead.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20150
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 D20105. Fixes T7732. T7732 describes a case where a user had their Google credentials swapped and had trouble regaining access to their account.
Since we now allow email login even if password auth is disabled, it's okay to let users unlink their final account, and it's even reasonable for users to unlink their final account if it is mis-linked.
Just give them a warning that what they're doing is a little sketchy, rather than preventing the workflow.
Test Plan: Unlinked my only login account, got a stern warning instead of a dead end.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7732
Differential Revision: https://secure.phabricator.com/D20106
Summary:
Ref T6703. Replaces the small "link" icon with a more obvious "Link External Account" button.
Moves us toward operating against `$config` objects instead of against `$provider` objects, which is more modern and will some day allow us to resolve T6703.
Test Plan: Viewed page, saw a more obvious button. Linked an external account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T6703
Differential Revision: https://secure.phabricator.com/D20105
Summary: Depends on D20039. Ref T13242. If installs want users to install a specific application, reference particular help, etc., let them customize the MFA enrollment message so they can make it say "if you have issues, see this walkthrough on the corporate wiki" or whatever.
Test Plan:
{F6164340}
{F6164341}
{F6164342}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13242
Differential Revision: https://secure.phabricator.com/D20043
Summary:
Depends on D20033. Ref T13222. Flesh this UI out a bit, and provide bit-strength information for TOTP.
Also, stop users from adding multiple SMS factors since this is pointless (they all always text your primary contact number).
Test Plan:
{F6156245}
{F6156246}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20034
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:
Depends on D20020. Ref T13222. This puts another step in the MFA enrollment flow: pick a provider; read text and click "Continue"; actually enroll.
This is primarily to stop CSRF attacks, since otherwise an attacker can put `<img src="phabricator.com/auth/settings/enroll/?providerPHID=xyz" />` on `cute-cat-pix.com` and get you to send yourself some SMS enrollment text messages, which would be mildly annoying.
We could skip this step if we already have a valid CSRF token (and we often will), but I think there's some value in doing it anyway. In particular:
- For SMS/Duo, it seems nice to have an explicit "we're about to hit your phone" button.
- We could let installs customize this text and give users a smoother onboard.
- It allows the relatively wordy enroll form to be a little less wordy.
- For tokens which can expire (SMS, Duo) it might save you from answering too slowly if you have to go dig your phone out of your bag downstairs or something.
Test Plan: Added factors, read text. Tried to CSRF the endpoint, got a dialog instead of a live challenge generation.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20021
Summary:
Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret).
However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class.
Also, slightly simplify some of the HTTP form gymnastics.
Test Plan:
- Hit the TOTP enroll screen.
- Reloaded it, got new secrets.
- Reloaded it more than 10 times, got told to stop generating new challenges.
- Answered a challenge properly, got a new TOTP factor.
- Grepped for removed class name.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20020
Summary:
Depends on D20018. Ref T13222. When you add a new MFA configuration, you can technically (?) guess your way through it with brute force. It's not clear why this would ever really be useful (if an attacker can get here and wants to add TOTP, they can just add TOTP!) but it's probably bad, so don't let users do it.
This limit is fairly generous because I don't think this actually part of any real attack, at least today with factors we're considering.
Test Plan:
- Added TOTP, guessed wrong a ton of times, got rate limited.
- Added TOTP, guessed right, got a TOTP factor configuration added to my account.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20019
Summary:
Depends on D20016. Ref T920. This does nothing interesting on its own since the TOTP provider has no guidance/warnings, but landing it separately helps to simplify an upcoming SMS diff.
SMS will have these guidance messages:
- "Administrator: you haven't configured any mailer which can send SMS, like Twilio."
- "Administrator: SMS is weak."
- "User: you haven't configured a contact number."
Test Plan: {F6151283} {F6151284}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20017
Summary:
Depends on D20013. Recently, I renamed the "Account" panel to "Language".
When you land on "Settings" and the first panel is an "EditEngine" panel ("Account/Langauge", "Date and Time", and "Conpherence" are all "EditEngine" panels), the engine shows the controls for the first panel.
However, the "first panel" according to EditEngine and the "first panel" in the menu are currently different: the menu groups panels into topics.
When I renamed "Account" to "Language", it went from conicidentally being the first panel in both lists to being the second panel in the grouped menu list and the, uh, like 12th panel in the ungrouped raw list.
This made landing on "Settings" show you the right chrome, but show you a different panel's controls ("Conpherence", now alphabetically first).
Instead, use the same order in both places.
(This was also a pre-existing bug if you use a language which translates the panel names such that "Account" is not alphabetically first.)
Test Plan: Visited "Settings", saw "Date & Time" form controls instead of "Conpherence" form controls on the default screen with "Date & Time" selected in the menu.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20016
Summary:
Depends on D20010. Ref T920. Allow users to designate which contact number is "primary": the number we'll actually send stuff to.
Since this interacts in weird ways with "disable", just do a "when any number is touched, put all of the user's rows into the right state" sort of thing.
Test Plan:
- Added numbers, made numbers primary, disabled a primary number, un-disabled a number with no primaries. Got sensible behavior in all cases.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T920
Differential Revision: https://secure.phabricator.com/D20011
Summary:
Depends on D20006. Ref T13222. Currently, the "MFA Is Required" gate doesn't let you do anything else, but you'll need to be able to access "Contact Numbers" if an install provides SMS MFA.
Tweak this UI to give users limited access to settings, so they can set up contact numbers and change their language.
(This is a little bit fiddly, and I'm doing it early on partly so it can get more testing as these changes move forward.)
Test Plan: {F6146136}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13222
Differential Revision: https://secure.phabricator.com/D20008
