Page MenuHomePhabricator

Modularize auth provider configuration
AcceptedPublic

Authored by amckinley on Jul 17 2019, 11:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 19, 3:17 AM
Unknown Object (File)
Fri, Apr 19, 3:17 AM
Unknown Object (File)
Fri, Apr 12, 5:33 AM
Unknown Object (File)
Thu, Apr 11, 4:26 AM
Unknown Object (File)
Tue, Apr 9, 8:04 PM
Unknown Object (File)
Tue, Apr 9, 9:21 AM
Unknown Object (File)
Mon, Apr 8, 6:31 PM
Unknown Object (File)
Sat, Mar 30, 8:26 AM
Subscribers

Details

Reviewers
epriestley
Summary

Move provider configs towards a better world, maybe even potentially a post-T6703 world.

This should be functionally identical to the previous version, except that we now enforce fiddling with shouldTrustEmails and showAllowAutoLogin to the transaction layer.

Test Plan

Made many edits to provider configs; eventually did not get any more crashes.

Diff Detail

Repository
rP Phabricator
Branch
auth-config-mod
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 23148
Build 31789: Run Core Tests
Build 31788: arc lint + arc unit

Event Timeline

src/applications/auth/controller/config/PhabricatorAuthEditController.php
130

This is also new.

src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
15

Maybe this should also be moved to PhabricatorAuthConfigPropertyTransaction?

epriestley added inline comments.
src/applications/auth/storage/PhabricatorAuthProviderConfigTransaction.php
15

Moving this makes sense to me.

src/applications/auth/xaction/PhabricatorAuthConfigTrustEmailsTransaction.php
32

Ideally, we would raise this error only if (a) the transaction actually attempts to change the value and (b) probably, the new value is "enabled". See D20311 for a somewhat-recent somewhat-relevant example.

This is mostly for API callers, but the ideas here are:

  • It's okay to write a client that shows the user a form, then submits all the transactions that "Save" implies. We don't require the client to know whether each field is editable or valid: it's okay for them to send back the full current state of the object and let us validate it.
  • A specific subcase of this is when a task has an invalid value for some <select /> field, like priority set to "zebra". It's okay to apply a transaction which changes the priority from "zebra" to "zebra", even though "zebra" is not valid, because if we don't allow this it's super tough to write a sensible form on the client that doesn't get in the user's way in the face of questionable data.
  • validateTransactions() is doing (at least) two types of checks: one type of checks is structural ("is this value in a plausible format?") and one is more permissions-ey ("is the value itself a legal value which the user has permission to select?"). We could structure the code like this instead:
validateTransactionStructure($xactions);
$xactions = filterNoOpTransactions($xactions);
validateTransactionValues($xactions);

...but I think most of the time these checks are pretty part-and-parcel and this would lead to a lot of duplicate code, even though it might technically be more clear.

An adjacent issue is that you can submit transactions (for an object with title "B") like "set title to A" and also "set title to B" in the same transaction list. These merge+filter into a no-op (since we change the title, but then change it back), but in this case we do want to raise an error.

Basically, there are a million weird cases here and I think we're structurally in reasonable territory where badness is at least relatively low, if not actually minimized, but one piece of badness is that you unintuitively should allow not-exactly-valid-but-no-op transactions to proceed through validateTransactions().

This revision is now accepted and ready to land.Jul 18 2019, 4:32 PM