can now tell phabricator you trust an auth provider's emails (useful for Google OAuth), which will mark emails as "verified" and will skip email verification.
ClosedPublic
Actions

Authored by talshiri on May 16 2014, 5:27 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

Restricted Diffusion Commit
rP43d45c49568a: can now tell phabricator you trust an auth provider's emails (useful for Google…

Summary

This is useful when you're trying to onboard an entire office and you end up using the Google OAuth anyway.

Test Plan

tested locally. Maybe I should write some tests?

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

talshiri updated this revision to Diff 21731.May 16 2014, 5:27 AM

talshiri retitled this revision from to can now tell phabricator you trust an auth provider's emails (useful for Google OAuth), which will mark emails as "verified" and will skip email verification..

talshiri updated this object.

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

talshiri added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 16 2014, 5:27 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B503: Diff 21731.May 16 2014, 5:28 AM

I think I caught a couple things -- let me know if I'm misunderstanding the verified case.

src/applications/auth/controller/PhabricatorAuthRegisterController.php
253	We should additionally require that the `$value_email === $default_email` here -- that we're really trusting the address the adapter gave us. I don't think this has any security implications, but if the address the adapter gives us is already associated with another account, we'll currently let the user enter a different address. In this case, we shouldn't trust it, since the user just typed something. Mostly, the behavior might change in the future, and this is a potentially weak link.
302–304	I think this is incorrect -- verification (click a link in an email) and approval (administrator manually says the account is okay) are different. This will stop administrators from being notified that a new user is waiting for approval.
src/applications/auth/controller/config/PhabricatorAuthEditController.php
127	This needs to be `getInt(..., 0)` for installs with MySQL strict mode.
223	For consistency, prefer title case: 'Trust Email Addresses'

This revision now requires changes to proceed.May 16 2014, 12:29 PM

incorporated review comments

Harbormaster completed remote builds in B511: Diff 21746.May 16 2014, 7:41 PM

epriestley added inline comments.May 16 2014, 7:43 PM

src/applications/auth/controller/PhabricatorAuthRegisterController.php
253	Why is the `empty(...)` case here? If the provider didn't give us an email, we're just trusting whatever the user types in?

epriestley added inline comments.May 16 2014, 7:45 PM

resources/sql/autopatches/20141505.trustEmails.sql
1 ↗	(On Diff #21746)	Oh -- this should be named: 20140515.trust-emails.sql That is, YYYYMMDD (not YYYYDDMM) so alphabetic/chronological sorts are the same, and `trust-emails` for consistency with other patches.

talshiri added inline comments.May 16 2014, 7:47 PM

src/applications/auth/controller/PhabricatorAuthRegisterController.php
253	Most providers just set the account's email before reaching this part, but the user/password provider doesn't do that (maybe worth fixing?).
302–304	Oh oops. I thought this was something else :)

talshiri added inline comments.May 16 2014, 7:54 PM

src/applications/auth/controller/PhabricatorAuthRegisterController.php
253	to clarify, in the user/password auth flow, the account has not been created at this point, so there is no default email.
253	Maybe I should soften it to check $is_default instead?

Ah. Let's add something like an isThisAdapterTrustable() method that defaults to true and returns false for username/pass, then hide the checkbox in the UI for anything untrustable? I don't think we should ever trust things users type in if require-email-verification is set. It doesn't make sense to have that option off and then trust username/password auth, so if installs end up in that state the admin is probably confused.