It's useless without SMS support and only exposed to the user themselves.
Closes T15486
Details
Details
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
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- T15486contactNr (branched from master)
- Lint
Lint Warnings Severity Location Code Message Warning src/applications/auth/editor/PhabricatorAuthContactNumberEditEngine.php:78 XHP9 Naming Conventions Warning src/applications/auth/editor/PhabricatorAuthContactNumberEditEngine.php:79 XHP9 Naming Conventions Warning src/applications/settings/panel/PhabricatorContactNumbersSettingsPanel.php:27 XHP9 Naming Conventions Warning src/applications/settings/panel/PhabricatorContactNumbersSettingsPanel.php:28 XHP9 Naming Conventions - Unit
Tests Passed - Build Status
Buildable 870 Build 870: arc lint + arc unit
Event Timeline
Comment Actions
Changing policy based on sms being configured seems a little off to me. Having the setting only conditionally show based on it being configured seems fine, however what happens in this scenario:
- Turn on sms
- Add number
- Turn off sms
I’m guessing the user would be unable to remove their contact number (or even see it), unless turning off sms deletes their contact number. In this case I think the user would still want to know it’s saved somewhere and could remove it.
It might be better for now to include some explanation text on those pages saying it’s only used for MFA if enabled, and leave the display of these as-is
Comment Actions
@speck: Currently (without my patch), users can only disable a contact number of them; users could not and cannot remove or delete a contact number which they entered into Phorge. Thus my patch does not make the situation worse: users still will not be able to remove the contact number themselves (SMS enabled or not) but at least users cannot enter more contact numbers while SMS is not enabled in their Phorge installation.
Comment Actions
Thanks for clarifying this behavior, it sounds like contact numbers in general need fleshed out quite a bit.
What do you think about not modifying policy based on the sms MFA turned on? Would it be possible to have that endpoint return 404 or similar without dynamically modifying policy? Or is there another example in the codebase for similar dynamic policy definition that can be referenced?
Comment Actions
I am sorry, thanks for your last comment, I now understand your point. Yes, I guess we'd better return new Aphront404Response() instead of some permission based message. Like we'd do trying to access the URL of an uninstalled application. Now just need to find out how to do that :)