Browse Source

ref(auth): Minor flow cleanup in handle_unknown_identity (#54580)

Change the flow so that `op = None` no longer controls an early halt.
Extract `login_form` so that we don't have to initialize it so early.
Ryan Skonnord 1 year ago
parent
commit
f0c8fb2b9c
1 changed files with 26 additions and 30 deletions
  1. 26 30
      src/sentry/auth/helper.py

+ 26 - 30
src/sentry/auth/helper.py

@@ -459,6 +459,27 @@ class AuthIdentityHandler:
     def _has_usable_password(self) -> bool:
         return bool(self._app_user and self._app_user.has_usable_password())
 
+    @cached_property
+    def _login_form(self) -> AuthenticationForm:
+        return AuthenticationForm(
+            self.request,
+            self.request.POST if self.request.POST.get("op") == "login" else None,
+            initial={"username": self._app_user and self._app_user.username},
+        )
+
+    def _build_confirmation_response(self, is_new_account):
+        existing_user, template = self._dispatch_to_confirmation(is_new_account)
+        context = {
+            "identity": self.identity,
+            "provider": self.provider_name,
+            "identity_display_name": self.identity.get("name") or self.identity.get("email"),
+            "identity_identifier": self.identity.get("email") or self.identity.get("id"),
+            "existing_user": existing_user,
+        }
+        if not self._logged_in_user:
+            context["login_form"] = self._login_form
+        return self._respond(f"sentry/{template}.html", context)
+
     def handle_unknown_identity(
         self,
         state: AuthHelperSessionStore,
@@ -479,15 +500,7 @@ class AuthIdentityHandler:
         - Should I create a new user based on this identity?
         """
         op = self.request.POST.get("op")
-        login_form = (
-            None
-            if self._logged_in_user
-            else AuthenticationForm(
-                self.request,
-                self.request.POST if self.request.POST.get("op") == "login" else None,
-                initial={"username": self._app_user and self._app_user.username},
-            )
-        )
+
         # we don't trust all IDP email verification, so users can also confirm via one time email link
         is_account_verified = False
         if self.request.session.get("confirm_account_verification_key"):
@@ -527,16 +540,13 @@ class AuthIdentityHandler:
         elif not self._has_usable_password():
             is_new_account = True
 
-        auth_identity = None
         if op == "confirm" and self.user.is_authenticated or is_account_verified:
             auth_identity = self.handle_attach_identity()
         elif op == "newuser":
             auth_identity = self.handle_new_user()
         elif op == "login" and not self._logged_in_user:
             # confirm authentication, login
-            assert login_form is not None
-            op = None
-            if login_form.is_valid():
+            if self._login_form.is_valid():
                 # This flow is special.  If we are going through a 2FA
                 # flow here (login returns False) we want to instruct the
                 # system to return upon completion of the 2fa flow to the
@@ -545,29 +555,15 @@ class AuthIdentityHandler:
                 # If there is no 2fa we don't need to do this and can just
                 # go on.
                 try:
-                    self._login(login_form.get_user())
+                    self._login(self._login_form.get_user())
                 except self._NotCompletedSecurityChecks:
                     return self._post_login_redirect()
             else:
                 auth.log_auth_failure(self.request, self.request.POST.get("username"))
+            return self._build_confirmation_response(is_new_account)
         else:
-            op = None
-
-        if not op:
-            existing_user, template = self._dispatch_to_confirmation(is_new_account)
-
-            context = {
-                "identity": self.identity,
-                "provider": self.provider_name,
-                "identity_display_name": self.identity.get("name") or self.identity.get("email"),
-                "identity_identifier": self.identity.get("email") or self.identity.get("id"),
-                "existing_user": existing_user,
-            }
-            if login_form:
-                context["login_form"] = login_form
-            return self._respond(f"sentry/{template}.html", context)
+            return self._build_confirmation_response(is_new_account)
 
-        assert auth_identity is not None
         user = auth_identity.user
         user.backend = settings.AUTHENTICATION_BACKENDS[0]