feat(idp-migrations): one-time account validation keys #28732
Conversation
When a user supplies unrecognized SSO credentials associated with an email address for an existing user profile, offer them a chance to link their identity to that profile instead of automatically provisioning a new profile. Introduce a feature flag. The feature is incomplete and will hit unimplemented code if the flag is set.
src/sentry/auth/idpmigration.py
Outdated
| raise NotImplementedError # TODO | ||
| redis_key = "verificationKeyStorage" | ||
| TTL = 60 * 10 | ||
| cluster = redis.clusters.get("default").get_local_client_for_key(redis_key) |
There was a problem hiding this comment.
sorry, could u clarify? im not too familiar with what those mean
| {% block main %} | ||
| <h3>Confirm Email</h3> | ||
| <p>Please confirm your email ({{ confirm_email }}) by submitting the following code on the verification page: ({{ verification_key }}). This code will expire in 10 minutes.</p> | ||
| <!-- below is if we want to add a clickable button to verify --> |
There was a problem hiding this comment.
we'll definitely want a clickable button
There was a problem hiding this comment.
(will be done in a follow up PR)
…d auth:one-time-key prefix from email
src/sentry/auth/idpmigration.py
Outdated
| verification_value = {"user_id": user.id, "email": email, "member_id": member_id} | ||
|
|
||
| cluster.hmset(verification_key, verification_value) | ||
| cluster.expire(verification_key, TTL) |
There was a problem hiding this comment.
Optional style advice: I'm always a fan of using objects over primitives where it adds meaning. In my opinion, it would improve the declaration to change it from TTL = 60 * 10 to TTL = timedelta(minutes=10). Then, you would just have to unpack it back to a primitive here, by
cluster.expire(verification_key, TTL.total_seconds())There was a problem hiding this comment.
Redis doesn't seem to like TTL.total_seconds() since it will return 600.0, so i used TTL.seconds instead
There was a problem hiding this comment.
TTL.seconds works in this case but I think int(TTL.total_seconds()) is better style. The seconds attribute is just the seconds part of the timedelta being divided into days, seconds, and microseconds. (Observe that timedelta(days=1).seconds is 0.) Not much risk of a bug unless the TTL is adjusted to more than 24 hours, but better safe than sorry. 😉
src/sentry/auth/idpmigration.py
Outdated
| redis_key = "verificationKeyStorage" | ||
| TTL = 60 * 10 | ||
| cluster = redis.clusters.get("default").get_local_client_for_key(redis_key) | ||
| TTL = timedelta(minutes=10) |
There was a problem hiding this comment.
TTL can also go next to _REDIS_KEY (as _TTL).
src/sentry/auth/idpmigration.py
Outdated
| type="user.confirm_email", | ||
| context=context, | ||
| ) | ||
| msg.send_async([email]) | ||
|
|
||
|
|
||
| def create_verification_key(user: User, org: Organization, email: str) -> None: |
There was a problem hiding this comment.
i'm wondering if we could rename this method to be more descriptive about sending an email to the user: create_verification_key -> send_one_time_account_confirm_link?
src/sentry/auth/idpmigration.py
Outdated
|
|
||
| verification_code = get_random_string(32, string.ascii_letters + string.digits) | ||
| verification_key = f"auth:one-time-key:{verification_code}" | ||
| verification_value = {"user_id": user.id, "email": email, "member_id": member_id} |
There was a problem hiding this comment.
to be extra careful, could we also store in the verification value?:
identity["id"] (have to pass this into method)
and the organization_id?
There was a problem hiding this comment.
Per discussion in meeting, 👍 for identity["id"] but org ID can be retrieved from member_id.
… & added identity[id] to verification value
| } | ||
| msg = MessageBuilder( | ||
| subject="{}Confirm Email".format(options.get("mail.subject-prefix")), | ||
| template="sentry/emails/idp_verification_email.txt", |
There was a problem hiding this comment.
should we standardize all of these names to "confirm_account"?
| @@ -1,9 +1,40 @@ | |||
| import uuid | |||
| import string | |||
There was a problem hiding this comment.
we could probably generalize the name of this file too, idpmigration.py -> confirm_account.py
There was a problem hiding this comment.
(can be done in a follow up PR if you'd like as to avoid merge hell)
Create one-time keys to validate ownership of an email address, when a user with an existing profile but no password supplies SSO credentials with a matching address. This one-time key is then stored in Redis and an email will be sent containing the key to the user for them to verify their account.