Page MenuHomePhabricator

Actually enforce auth.lock-config
ClosedPublic

Authored by amckinley on Jul 10 2019, 3:05 PM.

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:

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

amckinley created this revision.Jul 10 2019, 3:05 PM
amckinley edited the summary of this revision. (Show Details)Jul 10 2019, 3:06 PM
amckinley requested review of this revision.Jul 10 2019, 3:07 PM

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.

amckinley planned changes to this revision.Jul 10 2019, 4:29 PM
amckinley updated this revision to Diff 49256.Jul 11 2019, 5:14 PM
  • 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:
  • When trying to save a provider when the config gets locked during an edit, show reasonable error:
amckinley added inline comments.Jul 11 2019, 5:15 PM
src/applications/auth/controller/config/PhabricatorAuthListController.php
91–93

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

amckinley edited the summary of this revision. (Show Details)Jul 11 2019, 6:31 PM
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 accepted this revision.Jul 11 2019, 7:55 PM
epriestley added inline comments.
src/applications/auth/controller/config/PhabricatorAuthListController.php
91–93

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
amckinley added inline comments.Jul 15 2019, 6:37 PM
src/applications/auth/controller/config/PhabricatorAuthListController.php
91–93

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/?

epriestley added inline comments.Jul 15 2019, 6:41 PM
src/applications/auth/controller/config/PhabricatorAuthListController.php
91–93

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.

amckinley added inline comments.Jul 15 2019, 6:50 PM
src/applications/auth/controller/config/PhabricatorAuthListController.php
91–93

Ahhhh, that's what I was looking for.

amckinley updated this revision to Diff 49263.Jul 15 2019, 6:51 PM
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.