Cleanup login/reset functionality, address security issue with 2FA pathways

DaneEveritt · DaneEveritt · commit c3e462ab2f50 · 2018-04-07T16:17:51.000-05:00
diff --git a/app/Http/Controllers/Auth/AbstractLoginController.php b/app/Http/Controllers/Auth/AbstractLoginController.php
@@ -106,6 +106,10 @@ protected function sendFailedLoginResponse(Request $request, Authenticatable $us
             $this->getField($request->input('user')) => $request->input('user'),
         ]);
 
+        if ($request->route()->named('auth.checkpoint')) {
+            throw new DisplayException(trans('auth.checkpoint_failed'));
+        }
+
         throw new DisplayException(trans('auth.failed'));
     }
 
diff --git a/app/Http/Controllers/Auth/ForgotPasswordController.php b/app/Http/Controllers/Auth/ForgotPasswordController.php
@@ -3,7 +3,7 @@
 namespace Pterodactyl\Http\Controllers\Auth;
 
 use Illuminate\Http\Request;
-use Illuminate\Http\RedirectResponse;
+use Illuminate\Http\JsonResponse;
 use Illuminate\Support\Facades\Password;
 use Pterodactyl\Http\Controllers\Controller;
 use Pterodactyl\Events\Auth\FailedPasswordReset;
@@ -18,9 +18,9 @@ class ForgotPasswordController extends Controller
      *
      * @param  \Illuminate\Http\Request
      * @param string $response
-     * @return \Illuminate\Http\RedirectResponse
+     * @return \Illuminate\Http\JsonResponse
      */
-    protected function sendResetLinkFailedResponse(Request $request, $response): RedirectResponse
+    protected function sendResetLinkFailedResponse(Request $request, $response): JsonResponse
     {
         // As noted in #358 we will return success even if it failed
         // to avoid pointing out that an account does or does not
@@ -34,9 +34,9 @@ protected function sendResetLinkFailedResponse(Request $request, $response): Red
      * Get the response for a successful password reset link.
      *
      * @param string $response
-     * @return \Illuminate\Http\RedirectResponse|\Illuminate\Http\JsonResponse
+     * @return \Illuminate\Http\JsonResponse
      */
-    protected function sendResetLinkResponse($response)
+    protected function sendResetLinkResponse($response): JsonResponse
     {
         return response()->json([
             'status' => trans($response),
diff --git a/app/Http/Controllers/Auth/LoginCheckpointController.php b/app/Http/Controllers/Auth/LoginCheckpointController.php
@@ -10,9 +10,8 @@ class LoginCheckpointController extends AbstractLoginController
 {
     /**
      * Handle a login where the user is required to provide a TOTP authentication
-     * token. In order to add additional layers of security, users are not
-     * informed of an incorrect password until this stage, forcing them to
-     * provide a token on each login attempt.
+     * token. Once a user has reached this stage it is assumed that they have already
+     * provided a valid username and password.
      *
      * @param \Pterodactyl\Http\Requests\Auth\LoginCheckpointRequest $request
      * @return \Illuminate\Http\JsonResponse
@@ -28,7 +27,7 @@ public function index(LoginCheckpointRequest $request): JsonResponse
             return $this->sendFailedLoginResponse($request);
         }
 
-        if (! array_get($cache, 'valid_credentials') || array_get($cache, 'request_ip') !== $request->ip()) {
+        if (array_get($cache, 'request_ip') !== $request->ip()) {
             return $this->sendFailedLoginResponse($request, $user);
         }
 
@@ -40,7 +39,7 @@ public function index(LoginCheckpointRequest $request): JsonResponse
             return $this->sendFailedLoginResponse($request, $user);
         }
 
-        $this->authManager->guard()->login($user, true);
+        $this->auth->guard()->login($user, true);
 
         return $this->sendLoginResponse($request);
     }
diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
@@ -33,22 +33,26 @@ public function login(Request $request): JsonResponse
             return $this->sendFailedLoginResponse($request);
         }
 
-        $validCredentials = password_verify($request->input('password'), $user->password);
+        // Ensure that the account is using a valid username and password before trying to
+        // continue. Previously this was handled in the 2FA checkpoint, however that has
+        // a flaw in which you can discover if an account exists simply by seeing if you
+        // can proceede to the next step in the login process.
+        if (! password_verify($request->input('password'), $user->password)) {
+            return $this->sendFailedLoginResponse($request, $user);
+        }
+
+        // If the user is using 2FA we do not actually log them in at this step, we return
+        // a one-time token to link the 2FA credentials to this account via the UI.
         if ($user->use_totp) {
             $token = str_random(128);
             $this->cache->put($token, [
                 'user_id' => $user->id,
-                'valid_credentials' => $validCredentials,
                 'request_ip' => $request->ip(),
             ], 5);
 
             return response()->json(['complete' => false, 'token' => $token]);
         }
 
-        if (! $validCredentials) {
-            return $this->sendFailedLoginResponse($request, $user);
-        }
-
         $this->auth->guard()->login($user, true);
 
         return response()->json(['complete' => true]);
diff --git a/resources/assets/pterodactyl/scripts/components/auth/ForgotPassword.vue b/resources/assets/pterodactyl/scripts/components/auth/ForgotPassword.vue
@@ -1,9 +1,19 @@
 <template>
     <div>
-        <form class="bg-white shadow-lg rounded-lg pt-10 px-8 pb-6 mb-4 animate fadein" method="post">
+        <div class="pb-4" v-for="error in errors">
+            <div class="p-2 bg-red-dark border-red-darker border items-center text-red-lightest leading-normal rounded flex lg:inline-flex w-full text-sm"
+                 role="alert">
+                <span class="flex rounded-full bg-red uppercase px-2 py-1 text-xs font-bold mr-3 leading-none">Error</span>
+                <span class="mr-2 text-left flex-auto">{{ error }}</span>
+            </div>
+        </div>
+        <form class="bg-white shadow-lg rounded-lg pt-10 px-8 pb-6 mb-4 animate fadein" method="post"
+              v-on:submit.prevent="submitForm"
+        >
             <div class="flex flex-wrap -mx-3 mb-6">
                 <div class="input-open">
                     <input class="input" id="grid-email" type="email" aria-labelledby="grid-email" ref="email" required
+                           v-bind:readonly="showSpinner"
                            v-bind:value="email"
                            v-on:input="updateEmail($event)"
                     />
@@ -12,9 +22,11 @@
                 </div>
             </div>
             <div>
-                <csrf/>
-                <button class="btn btn-blue btn-jumbo" type="submit">
-                    {{ $t('auth.recover_account') }}
+                <button class="btn btn-blue btn-jumbo" type="submit" v-bind:disabled="submitDisabled">
+                    <span class="spinner white" v-bind:class="{ hidden: ! showSpinner }">&nbsp;</span>
+                    <span v-bind:class="{ hidden: showSpinner }">
+                        {{ $t('auth.recover_account') }}
+                    </span>
                 </button>
             </div>
             <div class="pt-6 text-center">
@@ -27,10 +39,7 @@
 </template>
 
 <script>
-    import Csrf from "../forms/CSRF";
-
     export default {
-        components: {Csrf},
         name: 'forgot-password',
         props: {
             email: {type: String, required: true},
@@ -41,11 +50,43 @@
         data: function () {
             return {
                 X_CSRF_TOKEN: window.X_CSRF_TOKEN,
+                errors: [],
+                submitDisabled: false,
+                showSpinner: false,
             };
         },
         methods: {
             updateEmail: function (event) {
+                this.$data.submitDisabled = false;
                 this.$emit('update-email', event.target.value);
+            },
+
+            submitForm: function () {
+                const self = this;
+                this.$data.submitDisabled = true;
+                this.$data.showSpinner = true;
+                this.$data.errors = [];
+
+                window.axios.post(this.route('auth.forgot-password.send-link'), {
+                    email: this.$props.email,
+                })
+                    .then(function (response) {
+                        self.$data.submitDisabled = false;
+                        self.$data.showSpinner = false;
+                        self.flash({message: response.data.status, variant: 'success'});
+                        self.$router.push({name: 'login'});
+                    })
+                    .catch(function (err) {
+                        self.$data.showSpinner = false;
+                        if (!err.response) {
+                            return console.error(err);
+                        }
+
+                        const response = err.response;
+                        if (response.data && _.isObject(response.data.errors)) {
+                            self.$data.errors.push(response.data.errors[0].detail);
+                        }
+                    });
             }
         }
     }
diff --git a/resources/assets/pterodactyl/scripts/components/auth/LoginForm.vue b/resources/assets/pterodactyl/scripts/components/auth/LoginForm.vue
@@ -2,23 +2,20 @@
     <div>
         <flash-message variant="danger" />
         <flash-message variant="success" />
-        <flash-message variant="warning" />
-        <flash-message variant="info" />
-        <div class="py-4" v-if="errors && errors.length === 1">
+        <div class="pb-4" v-if="errors && errors.length === 1">
             <div class="p-2 bg-red-dark border-red-darker border items-center text-red-lightest leading-normal rounded flex lg:inline-flex w-full text-sm"
                  role="alert">
                 <span class="flex rounded-full bg-red uppercase px-2 py-1 text-xs font-bold mr-3 leading-none">Error</span>
                 <span class="mr-2 text-left flex-auto">{{ errors[0] }}</span>
             </div>
         </div>
         <form class="bg-white shadow-lg rounded-lg pt-10 px-8 pb-6 mb-4 animate fadein" method="post"
-              v-on:submit.prevent="handleLogin"
+              v-on:submit.prevent="submitForm"
         >
             <div class="flex flex-wrap -mx-3 mb-6">
                 <div class="input-open">
-                    <input class="input" id="grid-username" type="text" name="user" aria-labelledby="grid-username"
+                    <input class="input" id="grid-username" type="text" name="user" aria-labelledby="grid-username" required
                            ref="email"
-                           required
                            v-bind:value="user.email"
                            v-on:input="updateEmail($event)"
                     />
@@ -28,15 +25,19 @@
             <div class="flex flex-wrap -mx-3 mb-6">
                 <div class="input-open">
                     <input class="input" id="grid-password" type="password" name="password"
+                           ref="password"
                            aria-labelledby="grid-password" required
                            v-model="user.password"
                     />
                     <label for="grid-password">{{ $t('strings.password') }}</label>
                 </div>
             </div>
             <div>
-                <button class="btn btn-blue btn-jumbo" type="submit">
-                    {{ $t('auth.sign_in') }}
+                <button class="btn btn-blue btn-jumbo" type="submit" v-bind:disabled="showSpinner">
+                    <span class="spinner white" v-bind:class="{ hidden: ! showSpinner }">&nbsp;</span>
+                    <span v-bind:class="{ hidden: showSpinner }">
+                        {{ $t('auth.sign_in') }}
+                    </span>
                 </button>
             </div>
             <div class="pt-6 text-center">
@@ -67,6 +68,7 @@
         data: function () {
             return {
                 errors: [],
+                showSpinner: false,
             }
         },
         mounted: function () {
@@ -75,8 +77,9 @@
         methods: {
             // Handle a login request eminating from the form. If 2FA is required the
             // user will be presented with the 2FA modal window.
-            handleLogin: function () {
+            submitForm: function () {
                 const self = this;
+                this.$data.showSpinner = true;
 
                 axios.post(this.route('auth.login'), {
                     user: this.$props.user.email,
@@ -88,17 +91,20 @@
                         }
 
                         self.$props.user.password = '';
+                        self.$data.showSpinner = false;
                         self.$router.push({name: 'checkpoint', query: {token: response.data.token}});
                     })
                     .catch(function (err) {
                         self.$props.user.password = '';
+                        self.$data.showSpinner = false;
                         if (!err.response) {
                             return console.error(err);
                         }
 
                         const response = err.response;
                         if (response.data && _.isObject(response.data.errors)) {
-                            self.$data.errors.push(response.data.errors[0].detail);
+                            self.$data.errors = [response.data.errors[0].detail];
+                            self.$refs.password.focus();
                         }
                     });
             },
diff --git a/resources/assets/pterodactyl/styles/components/buttons.css b/resources/assets/pterodactyl/styles/components/buttons.css
@@ -7,7 +7,7 @@
   &.btn-blue {
     @apply .bg-blue .border-blue-dark .border .text-white;
 
-    &:hover {
+    &:hover:enabled {
       @apply .bg-blue-dark .border-blue-darker;
     }
   }
@@ -18,4 +18,9 @@
   &.btn-jumbo {
     @apply .p-4 .w-full .uppercase .tracking-wide .text-sm;
   }
+
+  &:disabled {
+    opacity: 0.55;
+    cursor: default;
+  }
 }
diff --git a/resources/assets/pterodactyl/styles/components/spinners.css b/resources/assets/pterodactyl/styles/components/spinners.css
@@ -0,0 +1,52 @@
+.spinner {
+  color: transparent;
+  pointer-events: none;
+  position: relative;
+
+  &:after {
+    animation: spinners--spin 500ms infinite linear;
+    border-radius: 9999px;
+    @apply .border-2 .border-grey-light;
+    border-top-color: transparent !important;
+    border-right-color: transparent !important;
+    content: '';
+    display: block;
+    width: 1em;
+    height: 1em;
+    left: calc(50% - (1em / 2));
+    top: calc(50% - (1em / 2));
+    position: absolute !important;
+  }
+
+  /**
+   * Speeds
+   */
+  &.spin-slow:after {
+    animation: spinners--spin 1200ms infinite linear;
+  }
+
+  /**
+   * Spinner Colors
+   */
+  &.blue:after {
+    @apply .border-blue;
+  }
+
+  &.white:after {
+    @apply .border-white;
+  }
+
+  &.spinner-thick:after {
+    @apply .border-4;
+  }
+}
+
+@keyframes spinners--spin {
+  from {
+    transform: rotate(0deg);
+  }
+
+  to {
+    transform: rotate(360deg);
+  }
+}
diff --git a/resources/assets/pterodactyl/styles/main.css b/resources/assets/pterodactyl/styles/main.css
@@ -10,6 +10,7 @@
 @import "components/animations.css";
 @import "components/authentication.css";
 @import "components/buttons.css";
+@import "components/spinners.css";
 
 /**
  * Tailwind Utilities
diff --git a/resources/lang/en/auth.php b/resources/lang/en/auth.php
@@ -19,7 +19,8 @@
     'reset_password_text' => 'Reset your account password.',
     'reset_password' => 'Reset Account Password',
     'email_sent' => 'An email has been sent to you with further instructions for resetting your password.',
-    'failed' => 'The credentials provided do not match those we have on record, or the 2FA token provided was invalid.',
+    'failed' => 'No account matching those credentials could be found.',
+    'checkpoint_failed' => 'The two-factor authentication token was invalid.',
     'throttle' => 'Too many login attempts. Please try again in :seconds seconds.',
     'password_requirements' => 'Passwords must contain at least one uppercase, lowercase, and numeric character and must be at least 8 characters in length.',
     'request_reset' => 'Locate Account',
diff --git a/routes/auth.php b/routes/auth.php

Original file line number	Diff line number	Diff line change
`@@ -106,6 +106,10 @@ protected function sendFailedLoginResponse(Request $request, Authenticatable $us`
`106`	`106`	`$this->getField($request->input('user')) => $request->input('user'),`
`107`	`107`	`]);`
`108`	`108`
	`109`	`+ if ($request->route()->named('auth.checkpoint')) {`
	`110`	`+ throw new DisplayException(trans('auth.checkpoint_failed'));`
	`111`	`+ }`
	`112`	`+`
`109`	`113`	`throw new DisplayException(trans('auth.failed'));`
`110`	`114`	`}`
`111`	`115`
Original file line number	Diff line number	Diff line change
`@@ -10,9 +10,8 @@ class LoginCheckpointController extends AbstractLoginController`
`10`	`10`	`{`
`11`	`11`	`/**`
`12`	`12`	`* Handle a login where the user is required to provide a TOTP authentication`
`13`		`- * token. In order to add additional layers of security, users are not`
`14`		`- * informed of an incorrect password until this stage, forcing them to`
`15`		`- * provide a token on each login attempt.`
	`13`	`+ * token. Once a user has reached this stage it is assumed that they have already`
	`14`	`+ * provided a valid username and password.`
`16`	`15`	`*`
`17`	`16`	`* @param \Pterodactyl\Http\Requests\Auth\LoginCheckpointRequest $request`
`18`	`17`	`* @return \Illuminate\Http\JsonResponse`
`@@ -28,7 +27,7 @@ public function index(LoginCheckpointRequest $request): JsonResponse`
`28`	`27`	`return $this->sendFailedLoginResponse($request);`
`29`	`28`	`}`
`30`	`29`
`31`		`- if (! array_get($cache, 'valid_credentials') \|\| array_get($cache, 'request_ip') !== $request->ip()) {`
	`30`	`+ if (array_get($cache, 'request_ip') !== $request->ip()) {`
`32`	`31`	`return $this->sendFailedLoginResponse($request, $user);`
`33`	`32`	`}`
`34`	`33`
`@@ -40,7 +39,7 @@ public function index(LoginCheckpointRequest $request): JsonResponse`
`40`	`39`	`return $this->sendFailedLoginResponse($request, $user);`
`41`	`40`	`}`
`42`	`41`
`43`		`- $this->authManager->guard()->login($user, true);`
	`42`	`+ $this->auth->guard()->login($user, true);`
`44`	`43`
`45`	`44`	`return $this->sendLoginResponse($request);`
`46`	`45`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`&.btn-blue {`
`8`	`8`	`@apply .bg-blue .border-blue-dark .border .text-white;`
`9`	`9`
`10`		`- &:hover {`
	`10`	`+ &:hover:enabled {`
`11`	`11`	`@apply .bg-blue-dark .border-blue-darker;`
`12`	`12`	`}`
`13`	`13`	`}`
`@@ -18,4 +18,9 @@`
`18`	`18`	`&.btn-jumbo {`
`19`	`19`	`@apply .p-4 .w-full .uppercase .tracking-wide .text-sm;`
`20`	`20`	`}`
	`21`	`+`
	`22`	`+ &:disabled {`
	`23`	`+ opacity: 0.55;`
	`24`	`+ cursor: default;`
	`25`	`+ }`
`21`	`26`	`}`