feat(auth): Reuse existing user profile for unknown identity #28621
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.
| "email": "test@example.com", | ||
| "email": self.email, |
There was a problem hiding this comment.
Can you elaborate? I extracted this into an instance variable to reduce repetition of self.identity["email"] below.
| @@ -309,6 +308,25 @@ def test_authenticated(self, mock_render): | |||
| assert context["existing_user"] is self.request.user | |||
| assert "login_form" not in context | |||
|
|
|||
| @mock.patch("sentry.auth.helper.render_to_response") | |||
| @mock.patch("sentry.auth.helper.create_verification_key") | |||
| def test_unauthenticated_with_existing_user(self, mock_create_key, mock_render): | |||
There was a problem hiding this comment.
it seems like either we are going to end up having duplicated test behavior between tests/sentry/web/frontend/test_auth_organization_login.py and here, or have tested behavior be split across both. i'm not sure what the right resolution is, but I think the integration tests are easier to work with since you don't need so many mocks.
There was a problem hiding this comment.
as discussed offline, probably prudent to enable this flag in the org login tests to confirm existing behavior is preserved when this flag is enabled.
There was a problem hiding this comment.
@RyanSkonnord what was your conclusion on integration tests vs unit tests? It seems like we'd still want integration tests regardless
|
@JoshFerge I think (The Setting the feature flag on |
|
@RyanSkonnord alright sounds good, will wait on your changes to have tests passing with the flag to give it another go-round. |
|
Turns out [Update: See #28686.] |
|
If we don't want to wait for #28686, it should be safe to go ahead. Having manually dummied out the feature flag check, I found no regressions when running |
There was a problem hiding this comment.
having an acting_user and existing_user is still confusing to me, but I understand wanting these changes to have a small footprint is ideal. maybe we could schedule a team programming session where we do some smaller refactors to this method. see comment about integration tests, but looks good.
292b47b to
ed1aa29
Compare
…assword Change create_user so that calling it with password="" sets up the user in a state where it does not have a usable password, as the test case intended. Previously, any falsy value would give it a default password. This exposes an incorrect assertion. The test case is currently failing.
…-automatic-migration-base
) 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.
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.
This is a basis for further feature development. It has function stubs and an incomplete page template that are not reachable unless a feature flag is set. For now, this flag should be set only in a development environment. Added a unit test (
test_unauthenticated_with_existing_user) which should verify that the stub is unreachable.