feat: implement stricter password rules

2024-11-10 06:34:14 +00:00 · 2021-05-21 19:14:00 +02:00 · 2021-05-21 19:14:00 +02:00 · a5389c41f7
commit a5389c41f7
parent 27b97fa317
14 changed files with 111 additions and 111 deletions
--- a/app/Http/Controllers/API/AlbumThumbnailController.php
+++ b/app/Http/Controllers/API/AlbumThumbnailController.php
@ -15,7 +15,7 @@ class AlbumThumbnailController extends Controller
        $this->mediaMetadataService = $mediaMetadataService;
    }

-    public function get(Album $album): JsonResponse
+    public function show(Album $album): JsonResponse
    {
        return response()->json(['thumbnailUrl' => $this->mediaMetadataService->getAlbumThumbnailUrl($album)]);
    }
--- a/app/Http/Controllers/API/ProfileController.php
+++ b/app/Http/Controllers/API/ProfileController.php
@ -7,6 +7,7 @@ use App\Models\User;
 use App\Services\TokenManager;
 use Illuminate\Contracts\Auth\Authenticatable;
 use Illuminate\Contracts\Hashing\Hasher as Hash;
+use Illuminate\Validation\ValidationException;

 class ProfileController extends Controller
 {
@ -34,15 +35,20 @@ class ProfileController extends Controller
            return response()->json();
        }

+        throw_unless(
+            $this->hash->check($request->current_password, $this->currentUser->password),
+            ValidationException::withMessages(['current_password' => 'Invalid current password'])
+        );
+
        $data = $request->only('name', 'email');

-        if ($request->password) {
-            $data['password'] = $this->hash->make($request->password);
+        if ($request->new_password) {
+            $data['password'] = $this->hash->make($request->new_password);
        }

        $this->currentUser->update($data);

-        $responseData = $request->password
+        $responseData = $request->new_password
            ? ['token' => $this->tokenManager->refreshToken($this->currentUser)->plainTextToken]
            : [];

--- a/app/Http/Requests/API/ProfileUpdateRequest.php
+++ b/app/Http/Requests/API/ProfileUpdateRequest.php
@ -2,8 +2,11 @@

 namespace App\Http\Requests\API;

+use Illuminate\Validation\Rules\Password;
+
 /**
- * @property string $password
+ * @property-read string|null $current_password
+ * @property-read string|null $new_password
 */
 class ProfileUpdateRequest extends Request
 {
@ -13,6 +16,8 @@ class ProfileUpdateRequest extends Request
        return [
            'name' => 'required',
            'email' => 'required|email|unique:users,email,' . auth()->user()->id,
+            'current_password' => 'required',
+            'new_password' => ['sometimes', Password::defaults()],
        ];
    }
 }
--- a/app/Http/Requests/API/UserStoreRequest.php
+++ b/app/Http/Requests/API/UserStoreRequest.php
@ -2,6 +2,8 @@

 namespace App\Http\Requests\API;

+use Illuminate\Validation\Rules\Password;
+
 /**
 * @property string $password
 * @property string $name
@ -21,7 +23,7 @@ class UserStoreRequest extends Request
        return [
            'name' => 'required',
            'email' => 'required|email|unique:users',
-            'password' => 'required',
+            'password' => ['required', Password::defaults()],
            'is_admin' => 'required',
        ];
    }
--- a/app/Http/Requests/API/UserUpdateRequest.php
+++ b/app/Http/Requests/API/UserUpdateRequest.php
@ -3,6 +3,7 @@
 namespace App\Http\Requests\API;

 use App\Models\User;
+use Illuminate\Validation\Rules\Password;

 /**
 * @property string $password
@ -26,6 +27,7 @@ class UserUpdateRequest extends Request
        return [
            'name' => 'required',
            'email' => 'required|email|unique:users,email,' . $user->id,
+            'password' => ['sometimes', Password::defaults()],
        ];
    }
 }
--- a/app/Models/User.php
+++ b/app/Models/User.php
@ -14,6 +14,7 @@ use Laravel\Sanctum\HasApiTokens;
 * @property int    $id
 * @property bool   $is_admin
 * @property string $lastfm_session_key
+ * @property string $name
 * @property string $email
 * @property string $password
 *
--- a/app/Providers/AuthServiceProvider.php
+++ b/app/Providers/AuthServiceProvider.php
@ -10,6 +10,7 @@ use App\Services\TokenManager;
 use Illuminate\Foundation\Support\Providers\AuthServiceProvider as ServiceProvider;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Auth;
+use Illuminate\Validation\Rules\Password;

 class AuthServiceProvider extends ServiceProvider
 {
@ -36,5 +37,16 @@ class AuthServiceProvider extends ServiceProvider

            return $tokenManager->getUserFromPlainTextToken($request->api_token ?: '');
        });
+
+        $this->setPasswordDefaultRules();
+    }
+
+    private function setPasswordDefaultRules(): void
+    {
+        Password::defaults(function (): Password {
+            return $this->app->isProduction()
+                ? Password::min(10)->letters()->numbers()->symbols()->uncompromised()
+                : Password::min(6);
+        });
    }
 }
--- a/composer.json
+++ b/composer.json
@ -10,7 +10,7 @@
    "type": "project",
    "require": {
        "php": ">=7.3",
-        "laravel/framework": "^8.0",
+        "laravel/framework": "^8.42",
        "james-heinrich/getid3": "^1.9",
        "guzzlehttp/guzzle": "^7.0.1",
        "aws/aws-sdk-php-laravel": "^3.1",
--- a/composer.lock
+++ b/composer.lock
@ -4,7 +4,7 @@
        "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
        "This file is @generated automatically"
    ],
-    "content-hash": "ea50e5446b57c942522ca6a1cfdebd5f",
+    "content-hash": "5f54c3d8584004c6454de204fd4c4509",
    "packages": [
        {
            "name": "algolia/algoliasearch-client-php",
@ -1453,16 +1453,16 @@
        },
        {
            "name": "laravel/framework",
-            "version": "v8.40.0",
+            "version": "v8.42.1",
            "source": {
                "type": "git",
                "url": "https://github.com/laravel/framework.git",
-                "reference": "a654897ad7f97aea9d7ef292803939798c4a02a4"
+                "reference": "41ec4897a70eb8729cf0ff34a8354413c54e42a6"
            },
            "dist": {
                "type": "zip",
-                "url": "https://api.github.com/repos/laravel/framework/zipball/a654897ad7f97aea9d7ef292803939798c4a02a4",
-                "reference": "a654897ad7f97aea9d7ef292803939798c4a02a4",
+                "url": "https://api.github.com/repos/laravel/framework/zipball/41ec4897a70eb8729cf0ff34a8354413c54e42a6",
+                "reference": "41ec4897a70eb8729cf0ff34a8354413c54e42a6",
                "shasum": ""
            },
            "require": {
@ -1570,7 +1570,7 @@
                "phpunit/phpunit": "Required to use assertions and run tests (^8.5.8|^9.3.3).",
                "predis/predis": "Required to use the predis connector (^1.1.2).",
                "psr/http-message": "Required to allow Storage::put to accept a StreamInterface (^1.0).",
-                "pusher/pusher-php-server": "Required to use the Pusher broadcast driver (^4.0|^5.0).",
+                "pusher/pusher-php-server": "Required to use the Pusher broadcast driver (^4.0|^5.0|^6.0).",
                "symfony/cache": "Required to PSR-6 cache bridge (^5.1.4).",
                "symfony/filesystem": "Required to enable support for relative symbolic links (^5.1.4).",
                "symfony/psr-http-message-bridge": "Required to use PSR-7 bridging features (^2.0).",
@ -1617,7 +1617,7 @@
                "issues": "https://github.com/laravel/framework/issues",
                "source": "https://github.com/laravel/framework"
            },
-            "time": "2021-04-28T14:38:56+00:00"
+            "time": "2021-05-19T13:03:18+00:00"
        },
        {
            "name": "laravel/helpers",
--- a/cypress/integration/profile.spec.ts
+++ b/cypress/integration/profile.spec.ts
@ -9,10 +9,10 @@ context('Profiles & Preferences', () => {
      cy.findByTestId('update-profile-form').should('be.visible')

      ;[
+        'current_password',
        'name',
        'email',
-        'password',
-        'confirm_password',
+        'new_password',
        'notify',
        'show_album_art_overlay',
        'confirm_closing'
@ -45,6 +45,7 @@ context('Profiles & Preferences', () => {
    cy.findByTestId('view-profile-link').click()

    cy.get('#profileWrapper').within(() => {
+      cy.get('[name=current_password]').clear().type('current-secrEt')
      cy.get('[name=name]').clear().type('Admin No. 2')
      cy.get('[name=email]').clear().type('admin.2@koel.test')
      cy.get('[type=submit]').click()
@ -60,10 +61,10 @@ context('Profiles & Preferences', () => {
    cy.findByTestId('view-profile-link').click()

    cy.get('#profileWrapper').within(() => {
+      cy.get('[name=current_password]').clear().type('current-secrEt')
      cy.get('[name=name]').clear().type('Admin No. 2')
      cy.get('[name=email]').clear().type('admin.2@koel.test')
-      cy.get('[name=password]').type('new-password')
-      cy.get('[name=confirm_password]').type('new-password')
+      cy.get('[name=new_password]').type('new-password')
      cy.get('[type=submit]').click()
    })

@ -71,22 +72,6 @@ context('Profiles & Preferences', () => {
    cy.findByTestId('view-profile-link').should('contain.text', 'Admin No. 2')
  })

-  it('does not update the profile if password does not match', () => {
-    cy.$login()
-    cy.findByTestId('view-profile-link').click()
-
-    cy.get('#profileWrapper').within(() => {
-      cy.get('[name=name]').clear().type('Admin No. 2')
-      cy.get('[name=email]').clear().type('admin.2@koel.test')
-      cy.get('[name=password]').as('password').type('new-password')
-      cy.get('[name=confirm_password]').as('confirmPassword').type('not-matching-password')
-      cy.get('[type=submit]').click()
-      cy.get('@password').should('have.class', 'error')
-      cy.get('@confirmPassword').should('have.class', 'error')
-      cy.findByText('Profile updated.').should('not.exist')
-    })
-  })
-
  it('has an option to show/hide album art overlay', () => {
    cy.$login()
    cy.$mockPlayback()
--- a/resources/assets
+++ b/resources/assets
@ -1 +1 @@
-Subproject commit 47861a5a3aa480f14de91af9010914d1f3b56619
+Subproject commit e644aae58d3a4edbcb2b6e2109a3300268767ed6
--- a/routes/api.php
+++ b/routes/api.php
@ -74,7 +74,7 @@ Route::group(['namespace' => 'API'], static function (): void {
        Route::put('album/{album}/cover', 'AlbumCoverController@update');
        Route::put('artist/{artist}/image', 'ArtistImageController@update');

-        Route::get('album/{album}/thumbnail', 'AlbumThumbnailController@get');
+        Route::get('album/{album}/thumbnail', 'AlbumThumbnailController@show');

        Route::group(['namespace' => 'Search', 'prefix' => 'search'], static function (): void {
            Route::get('/', 'ExcerptSearchController@index');
--- a/tests/Feature/ProfileTest.php
+++ b/tests/Feature/ProfileTest.php
@ -3,53 +3,59 @@
 namespace Tests\Feature;

 use App\Models\User;
-use Illuminate\Contracts\Hashing\Hasher;
+use Illuminate\Http\Response;
+use Illuminate\Support\Facades\Hash;

 class ProfileTest extends TestCase
 {
-    private $hash;
+    /** @var User */
+    private $user;

    public function setUp(): void
    {
        parent::setUp();

-        $this->hash = self::mock(Hasher::class);
+        $this->user = User::factory()->create(['password' => Hash::make('secret')]);
    }

-    public function testUpdateProfileWithoutPassword(): void
+    public function testUpdateProfileRequiresCurrentPassword(): void
    {
-        $user = User::factory()->create();
-
-        $this->hash->shouldReceive('make')->never();
-
-        $this->putAsUser('api/me', ['name' => 'Foo', 'email' => 'bar@baz.com'], $user);
-
-        self::assertDatabaseHas('users', ['name' => 'Foo', 'email' => 'bar@baz.com']);
-    }
-
-    public function testUpdateProfileWithPassword(): void
-    {
-        /** @var User $user */
-        $user = User::factory()->create();
-
-        $this->hash
-            ->shouldReceive('make')
-            ->once()
-            ->with('qux')
-            ->andReturn('hashed');
-
        $this->putAsUser('api/me', [
            'name' => 'Foo',
            'email' => 'bar@baz.com',
-            'password' => 'qux',
-        ], $user)
-            ->assertJsonStructure(['token']);
+        ], $this->user)
+            ->assertStatus(Response::HTTP_UNPROCESSABLE_ENTITY);
+    }

-        self::assertDatabaseHas('users', [
-            'id' => $user->id,
+    public function testUpdateProfileWithoutNewPassword(): void
+    {
+        $this->putAsUser('api/me', [
            'name' => 'Foo',
            'email' => 'bar@baz.com',
-            'password' => 'hashed',
-        ]);
+            'current_password' => 'secret',
+        ], $this->user);
+
+        $this->user->refresh();
+
+        self::assertSame('Foo', $this->user->name);
+        self::assertSame('bar@baz.com', $this->user->email);
+        self::assertTrue(Hash::check('secret', $this->user->password));
+    }
+
+    public function testUpdateProfileWithNewPassword(): void
+    {
+        $this->putAsUser('api/me', [
+            'name' => 'Foo',
+            'email' => 'bar@baz.com',
+            'new_password' => 'new-secret',
+            'current_password' => 'secret',
+        ], $this->user)
+            ->assertJsonStructure(['token']);
+
+        $this->user->refresh();
+
+        self::assertSame('Foo', $this->user->name);
+        self::assertSame('bar@baz.com', $this->user->email);
+        self::assertTrue(Hash::check('new-secret', $this->user->password));
    }
 }
--- a/tests/Feature/UserTest.php
+++ b/tests/Feature/UserTest.php
@ -3,17 +3,13 @@
 namespace Tests\Feature;

 use App\Models\User;
-use Illuminate\Contracts\Hashing\Hasher;
+use Illuminate\Support\Facades\Hash;

 class UserTest extends TestCase
 {
-    private $hash;
-
    public function setUp(): void
    {
        parent::setUp();
-
-        $this->hash = self::mock(Hasher::class);
    }

    public function testNonAdminCannotCreateUser(): void
@ -21,64 +17,48 @@ class UserTest extends TestCase
        $this->postAsUser('api/user', [
            'name' => 'Foo',
            'email' => 'bar@baz.com',
-            'password' => 'qux',
+            'password' => 'secret',
            'is_admin' => false,
-        ])->assertStatus(403);
+        ])->assertForbidden();
    }

    public function testAdminCreatesUser(): void
    {
-        $this->hash
-            ->shouldReceive('make')
-            ->once()
-            ->with('qux')
-            ->andReturn('hashed');
-
        $this->postAsUser('api/user', [
            'name' => 'Foo',
            'email' => 'bar@baz.com',
-            'password' => 'qux',
+            'password' => 'secret',
            'is_admin' => true,
-        ], User::factory()->admin()->create());
+        ], User::factory()->admin()->create())
+            ->assertOk();

-        self::assertDatabaseHas('users', [
-            'name' => 'Foo',
-            'email' => 'bar@baz.com',
-            'password' => 'hashed',
-            'is_admin' => true,
-        ]);
+        /** @var User $user */
+        $user = User::firstWhere('email', 'bar@baz.com');
+
+        self::assertTrue(Hash::check('secret', $user->password));
+        self::assertSame('Foo', $user->name);
+        self::assertSame('bar@baz.com', $user->email);
+        self::assertTrue($user->is_admin);
    }

    public function testAdminUpdatesUser(): void
    {
        /** @var User $user */
-        $user = User::factory()->create([
-            'name' => 'John',
-            'email' => 'john@doe.com',
-            'password' => 'nope',
-            'is_admin' => true,
-        ]);
+        $user = User::factory()->admin()->create(['password' => 'secret']);

-        $this->hash
-            ->shouldReceive('make')
-            ->once()
-            ->with('qux')
-            ->andReturn('hashed');
-
-        $this->putAsUser("api/user/{$user->id}", [
+        $this->putAsUser("api/user/$user->id", [
            'name' => 'Foo',
            'email' => 'bar@baz.com',
-            'password' => 'qux',
+            'password' => 'new-secret',
            'is_admin' => false,
        ], User::factory()->admin()->create());

-        self::assertDatabaseHas('users', [
-            'id' => $user->id,
-            'name' => 'Foo',
-            'email' => 'bar@baz.com',
-            'password' => 'hashed',
-            'is_admin' => false,
-        ]);
+        $user->refresh();
+
+        self::assertTrue(Hash::check('new-secret', $user->password));
+        self::assertSame('Foo', $user->name);
+        self::assertSame('bar@baz.com', $user->email);
+        self::assertFalse($user->is_admin);
    }

    public function testAdminDeletesUser(): void
@ -87,7 +67,7 @@ class UserTest extends TestCase
        $user = User::factory()->create();
        $admin = User::factory()->admin()->create();

-        $this->deleteAsUser("api/user/{$user->id}", [], $admin);
+        $this->deleteAsUser("api/user/$user->id", [], $admin);
        self::assertDatabaseMissing('users', ['id' => $user->id]);
    }

@ -97,7 +77,7 @@ class UserTest extends TestCase
        $admin = User::factory()->admin()->create();

        // A user can't delete himself
-        $this->deleteAsUser("api/user/{$admin->id}", [], $admin)
+        $this->deleteAsUser("api/user/$admin->id", [], $admin)
            ->assertStatus(403);

        self::assertDatabaseHas('users', ['id' => $admin->id]);
@ -105,6 +85,7 @@ class UserTest extends TestCase

    public function testUpdateUserProfile(): void
    {
+        /** @var User $user */
        $user = User::factory()->create();
        self::assertNull($user->getPreference('foo'));