Page MenuHomePhabricator

Actually enforce auth.lock-config
ClosedPublic

Authored by amckinley on Jul 10 2019, 3:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 7:16 PM
Unknown Object (File)
Tue, Mar 26, 7:16 PM
Unknown Object (File)
Thu, Mar 14, 5:27 PM
Unknown Object (File)
Feb 15 2024, 1:14 AM
Unknown Object (File)
Feb 3 2024, 10:51 PM
Unknown Object (File)
Jan 25 2024, 2:07 AM
Unknown Object (File)
Jan 25 2024, 2:07 AM
Unknown Object (File)
Jan 25 2024, 2:07 AM
Subscribers

Details

Summary

Forgot to post this after D20394. Fixes T7667.

Test Plan
  • Edited some providers with the config locked and unlocked.
  • Opened the edit form with the config unlocked, locked the config, then saved, and got a sensible error:
    Screen Shot 2019-07-11 at 10.12.44 AM.png (352×1 px, 50 KB)

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 23124
Build 31751: Run Core Tests
Build 31750: arc lint + arc unit

Event Timeline

Maybe implement PhabricatorAuthProviderConfigEditor->validateTransaction()?

This should more or less be available as either:

  • ...Editor->validateAllTransactions(): easiest approach, but not ivory-tower-modular since extensions can't do anything about it. Probably the right approach here.
  • ...Editor->validateTransactionsWithExtensions(), which you actually add code to by implementing a PhabricatorEditorExtension. This is ivory-tower-modular in the sense that it can do something like this:
if ($xaction instanceof TransactionWhichCaresAboutAuthLockInterface) {
  if (is auth locked) {
    raise validation exception;
  }
}

So it would let third-party code (a) implement Auth transactions which bypass this setting and (b) implement transactions in other applications which respect this setting.

I think the extreme modularity here has no actual driving use cases and validateAllTransactions() is probably the better fit.

Let me actually read the rest of the change, now...

I think some variation of this probably remains desirable no matter what, since we want to roadblock users before they start doing edits, not only once they reach the transaction layer -- not for correctness, but just for usability, so they don't fill out a form and then learn that their changes can't be saved.

The "full correctness" change is probably:

  • optionally, modularize these transactions;
  • use Editor->validateAllTransactions() to implement this check at the transaction layer so future API callsites and such can't bypass it.
src/applications/auth/controller/config/PhabricatorAuthEditController.php
30–32

You can reduce boilerplate with return $this->newDialog()->... in a Controller, and setMethod() shouldn't be necessary (cancel buttons are just links that look like buttons visually, only "submit" is ever actually a button).

I was also looking for a way to still render the form, except with all the fields grayed out, since it isn't terribly user-friendly to make someone unlock the config just to see what it already says. This seems tricky though, since there's no AphrontFormView->getChildren() method I can iterate over to call setDisable(true) on.

I'd like to make the "view" page more useful (/auth/config/view/X/) which will probably solve that (by showing the non-secret configuration parameters before you hit the edit screen). That page is somewhat recent and just hasn't really been built out yet.

I'm also imagining possibly moving the common behaviors ("Can register / login / link / unlink") to a separate form since you rarely need to fiddle with them, they're pretty wordy, and they make the "Edit Provider" form huge even for providers that need relatively little configuration.

  • Raise "editing while locked" errors in the transaction layer.
  • Changed controllers to catch errors and display them reasonably.
  • Warn users about locked config when editing existing provider:
    Screen Shot 2019-07-11 at 10.09.18 AM.png (386×1 px, 66 KB)
  • When trying to save a provider when the config gets locked during an edit, show reasonable error:
    Screen Shot 2019-07-11 at 10.12.44 AM.png (352×1 px, 50 KB)
src/applications/auth/controller/config/PhabricatorAuthListController.php
89–91

This feels bad, but without this the button is still clickable, even if it's been setDisable'd.

amckinley edited the test plan for this revision. (Show Details)
amckinley edited the test plan for this revision. (Show Details)
amckinley edited the test plan for this revision. (Show Details)
epriestley added inline comments.
src/applications/auth/controller/config/PhabricatorAuthListController.php
89–91

In general, we let you click disabled things elsewhere and explain why the thing is disabled when you click it, so that's possibly fine?

This revision is now accepted and ready to land.Jul 11 2019, 7:55 PM
src/applications/auth/controller/config/PhabricatorAuthListController.php
89–91

Without this change, clicking the button navigates to /auth/config/new/ which shows the config warning instead of displaying the normal form. Is that ok, or should the click pop a little dialogue with the warning instead of navigating away from /auth/?

src/applications/auth/controller/config/PhabricatorAuthListController.php
89–91

Oh -- preferably, link the button in both cases and use setWorkflow($is_disabled) to make it pop a dialog if you click the disabled state.

src/applications/auth/controller/config/PhabricatorAuthListController.php
89–91

Ahhhh, that's what I was looking for.

amckinley edited the test plan for this revision. (Show Details)

Pop a dialog instead of navigating away.

This revision was automatically updated to reflect the committed changes.