Adds "verified" and "secretKey" to Legalpad document signatures. For logged in users using an email address they own, things are verified right away. Otherwise, the email is sent a verification letter. When the user clicks the link the signature is marked verified.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T4283: legalpad can't public to no login user visible
- Commits
- Restricted Diffusion Commit
rP41d2a0953604: Legalpad - make it work for not logged in users
signed the document with a bogus email address not logged in. verified the email that would be sent looked good from command line. followed link and successfully verified bogus email address
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
| src/applications/auth/controller/PhabricatorExternalEmailVerificationController.php | ||
|---|---|---|
| 46 ↗ | (On Diff #17948) | oh, and "Continue to Phabricator" could be "View Legal Document" or something instead |
Comment Actions
Well, I like the inclusion of the PHID. I might make some sort of lookup table to external accounts though so there's just a single clean <code>. I'll call this something like PhabricatorExternalEmailAccount and it will just had the real external account phid and a code column.
...I also forgot to check the secret in the URI matches the secret in the database. That's a must fix.
Lemme know if you have any thoughts.
Comment Actions
I'm not sure it makes sense to verify the external address itself (that is, verify the email), see inline #1. Also, I don't think that property/method exists? But in the more general case, it probably doesn't make sense to "verify" a Twitter or Google account. We could verify that we know an email account actually exists, maybe, but verifying it as authentic seems to make less sense.
Particularly, I think the verification should actually be on the operation itself? That is, you're verifying the signature, not the account. If we verify the account, anyone else can later come and sign anything else as that account, presumably, since it's already "verified", but this is the wrong result.
That is, for now, I think the workflow should work something like this:
- Signature has a new verified flag on it. This is set for users, but not set for emails / public users.
- Signature has a new secretKey field on it which is populated with random stuff.
- Unverified signatures send an email with a link to /legalpad/verify/xxxxxx/.
- When clicked, that page loads the Signature with secret key = xxxxxx, verifies it, and redirects to the legalpad.
If we anticipate needing other similar verifications, we could do this:
- Remove secretKey from Signature.
- Add a new ExternalAccountVerificationRequest table to the user database, with columns like (externalAccountPHID, secretKey, verifyObjectPHID) instead.
- When you hit /auth/verify/xxxx/, we load the ExternalAccountVerificationRequest row with secretKey = xxxx. From that, we can load the user and object.
- A new VerifiableThing sort of interface lets that controller (a) verify the Signature and (b) redirect to the right place afterward.
- The VerifiableThing interface also helps write the email to the user when they make an unverified signature.
But maybe this is overkill and we don't need the VerifiableThing or ExternalAccountVerificationRequest abstractions.
Also, at some point farther in the future, I think we'll want to start issuing external user sessions. I'm not exactly sure how verification should work under that workflow. This workflow doesn't seem bad even if we have sessions, though.
| src/applications/auth/controller/PhabricatorExternalEmailVerificationController.php | ||
|---|---|---|
| 48 ↗ | (On Diff #17948) | (First bit is about this line.) |
| src/applications/legalpad/controller/LegalpadDocumentSignController.php | ||
| 44–64 | This could probably be some kind of load-or-create method somewhere. I think the distinction between verified_email and unverified_email here isn't correct, though. You can get an entry in that table by just emailing bugs@.... | |
Comment Actions
some changes
- make it about verifying the signature as opposed to some external account
- pertinent schema changes
- blank out signature data (rather than help logged in users) to be more compliant with e-signature stuff
Re-tested form failure, sign -> verification for a logged out user, sign -> verification for a logged in user using some other email account, sign + auto verification for logged in user using an email address they own.
Did not move the external account load or create someplace central 'cuz I found that to be a "hard naming problem." I think I'll maybe need it again later for Nuance and can deal with that then.
| resources/sql/autopatches/20140113.legalpadsig.sql | ||
|---|---|---|
| 1–2 ↗ | (On Diff #18030) | Theoretically we should migrate the existing documents to give them keys. |
| src/applications/legalpad/controller/LegalpadDocumentSignController.php | ||
| 38–54 | The gist of this logic is duplicated in PhabricatorMailReceiver (which also goes through the does-it-exist, create-it-if-not dance); we should probably move it somewhere shared in the future. | |
| 45–46 | We should probably reject the account if it is tied to an existing user (has a nonempty userPHID). This will prevent someone from signing as btrahan@phacility.com anonymously -- they can't verify the address anyway, but there's no legitimate reason to let them do this. This lets attackers fish for valid addresses, but they can already do this on the registration form. I think we should mitigate this attack in a general way with high-level roadblocking after too many requests via T3923. | |
| 139–144 | This seems a touch funky. Generally, I don't think we should require logged-in users to type in their email address. | |
Comment Actions
Changes
- added loadOneOrCreate method to PhabricatorExternalAccountQuery
- use that in LegalpadSign controller and PhabricatorMailReceiver
- reject signatures if there is a PhabricatorUserEmail object -or- and external account associated with a user phid
- prompt folks to login with the right account in these scenarios
NOTE: You were using domain "self" for these email external accounts in PhabricatorMailReceiver. I changed the domain to be the domain of the email address, and made it so the "self" thing would be backwards compatible in the PhabricatorMailReceiver lookup code.
Question - does the NOTE sound good? =D
NOTE: I did not migrate any of the document signatures because (instead) they are all considered auto-verified and the secret is not needed unless you need to verify an account.
Question - do you want me to add secrets despite the last NOTE ?
Comment Actions
(pushing to your queue since I have a few questions I think its best to get right before checkin)
Comment Actions
NOTE: You were using domain "self" for these email external accounts in PhabricatorMailReceiver. I changed the domain to be the domain of the email address, and made it so the "self" thing would be backwards compatible in the PhabricatorMailReceiver lookup code.
I think your approach is better, and it's fine to drop backward compatibility. This feature required configuration to enable and doesn't do much meaningful stuff so very few installs are likely to have any relevant rows.
NOTE: I did not migrate any of the document signatures because (instead) they are all considered auto-verified and the secret is not needed unless you need to verify an account.
I think either approach is fine. I have a very slight preference toward migrating in case we use the secret for something else in the future and forget that we didn't migrate old ones, but I think it's pretty moot in this case since there's likely not much real data in this application anyway.
Comment Actions
added a patch to add secret keys. tested by dropping columns then re-running both migrations and verifying data populated