From 5c5a381d9b9974c96f57edbe176fc91e8d1c1eca Mon Sep 17 00:00:00 2001
From: Matthew Somerville <matthew@mysociety.org>
Date: Thu, 19 Sep 2024 11:26:42 +0100
Subject: [PATCH] Make SSO button use separate form.

This reduces confusion if a browser auto-fills the email form but
someone still clicks the SSO button, or similar.
---
 perllib/FixMyStreet/App/Controller/Auth.pm |  5 +-
 t/app/controller/auth_social.t             |  1 +
 templates/web/base/auth/general.html       | 53 ++++++++++------------
 templates/web/brent/errors/generic.html    |  2 +-
 templates/web/camden/auth/general.html     | 18 ++++----
 templates/web/hackney/auth/general.html    | 18 ++++----
 templates/web/tfl/errors/generic.html      |  2 +-
 7 files changed, 47 insertions(+), 52 deletions(-)

diff --git a/perllib/FixMyStreet/App/Controller/Auth.pm b/perllib/FixMyStreet/App/Controller/Auth.pm
index 79eb29aba4c..052e97630ba 100644
--- a/perllib/FixMyStreet/App/Controller/Auth.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth.pm
@@ -44,10 +44,7 @@ sub general : Path : Args(0) {
 
     # decide which action to take
     $c->detach('code_sign_in') if $clicked_sign_in_by_code || ($data_email && !$data_password);
-    if (!$data_username && !$data_password && !$data_email && $c->get_param('social_sign_in')) {
-        $c->forward('social/handle_sign_in');
-    }
-
+    $c->detach('social/handle_sign_in') if $c->get_param('social_sign_in');
     $c->forward( 'sign_in', [ $data_username ] )
         && $c->detach( 'redirect_on_signin', [ $c->get_param('r') ] );
 
diff --git a/t/app/controller/auth_social.t b/t/app/controller/auth_social.t
index 4211e560548..b40ceca29b9 100644
--- a/t/app/controller/auth_social.t
+++ b/t/app/controller/auth_social.t
@@ -143,6 +143,7 @@ for my $state ( 'refused', 'no email', 'existing UID', 'okay' ) {
                     update => 'Test update',
                 };
             }
+            $mech->form_with_fields('social_sign_in');
             $mech->submit_form(with_fields => $fields, button => 'social_sign_in');
 
             # As well as the cookie issue above, caused by this external
diff --git a/templates/web/base/auth/general.html b/templates/web/base/auth/general.html
index 89d2c3d427c..c32b386e091 100644
--- a/templates/web/base/auth/general.html
+++ b/templates/web/base/auth/general.html
@@ -18,14 +18,12 @@ <h1>
     <p class="form-error">[% loc('Sorry, we could not log you in. Please fill in the form below.') %]</p>
 [% END %]
 
-<form action="/auth" method="post" name="general_auth" class="validate">
-
-    <input type="hidden" name="r" value="[% c.req.params.r | html %]">
-
 [% IF NOT oauth_need_email AND c.cobrand.social_auth_enabled %]
+<form action="/auth" method="post" name="sso_auth" class="validate">
+    <input type="hidden" name="r" value="[% c.req.params.r | html %]">
     [% IF c.config.FACEBOOK_APP_ID %]
       <div class="form-box">
-        <button name="social_sign_in" id="facebook_sign_in" value="facebook" class="btn btn--block btn--social btn--facebook">
+        <button name="social_sign_in" value="facebook" class="btn btn--block btn--social btn--facebook">
             <img alt="" src="/i/facebook-icon-32.png" width="17" height="32">
             [% loc('Log in with Facebook') %]
         </button>
@@ -34,23 +32,26 @@ <h1>
     [% oidc_config = c.cobrand.call_hook('oidc_config') OR c.cobrand.feature('oidc_login') %]
     [% IF oidc_config %]
       <div class="form-box">
-        <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="btn btn--block btn--social btn--oidc">
+        <button name="social_sign_in" value="oidc" class="btn btn--block btn--social btn--oidc">
             [% tprintf(loc('Login with %s'), oidc_config.display_name) %]
         </button>
       </div>
     [% END %]
     [% IF c.config.TWITTER_KEY %]
       <div class="form-box">
-        <button name="social_sign_in" id="twitter_sign_in" value="twitter" class="btn btn--block btn--social btn--twitter">
+        <button name="social_sign_in" value="twitter" class="btn btn--block btn--social btn--twitter">
             <img alt="" src="/i/twitter-icon-32.png" width="17" height="32">
             [% loc('Log in with Twitter') %]
         </button>
       </div>
     [% END %]
-      <div id="js-social-email-hide">
+</form>
 [% END %]
 
-        [% loc_username_error = INCLUDE 'auth/_username_error.html' default='email' %]
+<form action="/auth" method="post" name="general_auth" class="validate">
+    <input type="hidden" name="r" value="[% c.req.params.r | html %]">
+
+    [% loc_username_error = INCLUDE 'auth/_username_error.html' default='email' %]
 
 [% IF c.cobrand.sms_authentication %]
     [% SET username_label = loc('Your email or mobile') %]
@@ -60,26 +61,22 @@ <h1>
     [% SET username_type = 'email' %]
 [% END %]
 
-        <label class="n" for="username">[% username_label %]</label>
-      [% IF loc_username_error %]
-        <div class="form-error">[% loc_username_error %]</div>
+    <label class="n" for="username">[% username_label %]</label>
+  [% IF loc_username_error %]
+    <div class="form-error">[% loc_username_error %]</div>
+  [% END %]
+    <input type="[% username_type %]" class="form-control required" id="username" name="username" value="[% username | html %]" autocomplete="username"
+    [%~ IF c.cobrand.moniker != 'borsetshire' %] autofocus[% END %]>
+
+    <div id="form_sign_in">
+      [% IF oauth_need_email %]
+        [% INCLUDE form_sign_in_no %]
+        <input type="hidden" name="oauth_need_email" value="1">
+      [% ELSE %]
+        [% INCLUDE form_sign_in_yes %]
+        [% INCLUDE form_sign_in_no %]
       [% END %]
-        <input type="[% username_type %]" class="form-control required" id="username" name="username" value="[% username | html %]" autocomplete="username"
-        [%~ IF c.cobrand.moniker != 'borsetshire' %] autofocus[% END %]>
-
-        <div id="form_sign_in">
-          [% IF oauth_need_email %]
-            [% INCLUDE form_sign_in_no %]
-            <input type="hidden" name="oauth_need_email" value="1">
-          [% ELSE %]
-            [% INCLUDE form_sign_in_yes %]
-            [% INCLUDE form_sign_in_no %]
-          [% END %]
-        </div>
-
-[% IF NOT oauth_need_email AND c.cobrand.social_auth_enabled %]
-      </div>
-[% END %]
+    </div>
 
 </form>
 
diff --git a/templates/web/brent/errors/generic.html b/templates/web/brent/errors/generic.html
index 589686514c7..766f802d487 100755
--- a/templates/web/brent/errors/generic.html
+++ b/templates/web/brent/errors/generic.html
@@ -25,7 +25,7 @@ <h1>My Account login</h1>
         <form action="/auth" method="post" name="general_auth" class="validate">
             <input type="hidden" name="r" value="[% c.req.params.r | html %]">
             <div class="form-box">
-                <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="btn btn--block btn--social btn--oidc">
+                <button name="social_sign_in" value="oidc" class="btn btn--block btn--social btn--oidc">
                     Click here to continue...
                 </button>
                 </div>
diff --git a/templates/web/camden/auth/general.html b/templates/web/camden/auth/general.html
index cf916b5e89f..96efd09521c 100644
--- a/templates/web/camden/auth/general.html
+++ b/templates/web/camden/auth/general.html
@@ -42,11 +42,19 @@ <h1>
       [% ELSE %]
         [% INCLUDE form_sign_in_yes %]
         [% INCLUDE form_sign_in_no %]
-        [% INCLUDE form_sign_in_camden_staff %]
       [% END %]
     </div>
 </form>
 
+[% IF c.cobrand.feature('oidc_login') AND NOT oauth_need_email %]
+<form action="/auth" method="post" name="sso_auth" class="validate">
+    <input type="hidden" name="r" value="[% c.req.params.r | html %]">
+    <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="fake-link sso-staff-sign-in">
+        Camden Staff Sign-in
+    </button>
+</form>
+[% END %]
+
 [% INCLUDE 'footer.html' %]
 
 [% BLOCK form_sign_in_yes %]
@@ -76,11 +84,3 @@ <h1>
       [%~ END ~%]
       "></p>
 [% END %]
-
-[% BLOCK form_sign_in_camden_staff %]
-  [% IF c.cobrand.feature('oidc_login') %]
-      <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="fake-link sso-staff-sign-in">
-          Camden Staff Sign-in
-      </button>
-  [% END %]
-[% END %]
diff --git a/templates/web/hackney/auth/general.html b/templates/web/hackney/auth/general.html
index 791bf47569e..344005a38b7 100644
--- a/templates/web/hackney/auth/general.html
+++ b/templates/web/hackney/auth/general.html
@@ -44,11 +44,19 @@ <h1>
       [% ELSE %]
         [% INCLUDE form_sign_in_yes %]
         [% INCLUDE form_sign_in_no %]
-        [% INCLUDE form_sign_in_hackney_staff %]
       [% END %]
     </div>
 </form>
 
+[% IF c.cobrand.feature('oidc_login') AND NOT oauth_need_email %]
+<form action="/auth" method="post" name="sso_auth" class="validate">
+    <input type="hidden" name="r" value="[% c.req.params.r | html %]">
+    <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="fake-link sso-staff-sign-in">
+        Hackney Staff Sign-in
+    </button>
+</form>
+[% END %]
+
 [% INCLUDE 'footer.html' %]
 
 [% BLOCK form_sign_in_yes %]
@@ -78,11 +86,3 @@ <h1>
       [%~ END ~%]
       "></p>
 [% END %]
-
-[% BLOCK form_sign_in_hackney_staff %]
-  [% IF c.cobrand.feature('oidc_login') %]
-      <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="fake-link sso-staff-sign-in">
-          Hackney Staff Sign-in
-      </button>
-  [% END %]
-[% END %]
diff --git a/templates/web/tfl/errors/generic.html b/templates/web/tfl/errors/generic.html
index f1931d05fd7..23961ebbc30 100755
--- a/templates/web/tfl/errors/generic.html
+++ b/templates/web/tfl/errors/generic.html
@@ -25,7 +25,7 @@ <h1>My Account login</h1>
         <form action="/auth" method="post" name="general_auth" class="validate">
             <input type="hidden" name="r" value="[% c.req.params.r | html %]">
             <div class="form-box">
-                <button name="social_sign_in" id="oidc_sign_in" value="oidc" class="btn btn--block btn--social btn--oidc">
+                <button name="social_sign_in" value="oidc" class="btn btn--block btn--social btn--oidc">
                     Click here to continue...
                 </button>
                 </div>