Skip to content

Fix ClientController::store() breaking change introduced via #1745 #1860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix ClientController::store() breaking change introduced via #1745 #1860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/Http/Controllers/ClientController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public function forUser(Request $request): Collection
/**
* Store a new client.
*/
public function store(Request $request): Client
public function store(Request $request): array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BC change!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hafezdivandari it has been array|Client before:

passport/src/Http/Controllers/ClientController.php

Lines 76 to 78 in 89d2594

* @return \Laravel\Passport\Client|array
*/
public function store(Request $request)

You updated it to just Client via ef122ad#diff-6656ea7db3c71aee64d3cec3512c61d176630ed9f86a2b7359565fd3bfd57aa3 ignoring that people may already rely on the array etc.

In my book thats a breaking change introduced by you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR was on a major release 12 to 13 where breaking changes are expected to happen, but this PR is on a patch release!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes of course, but was this breaking change intended? Looked like a mistake to me and upgrade.md does not mention such a BC, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Want me to add a note in the https://github.com/laravel/passport/blob/13.x/UPGRADE.md#json-api-deprecation section + update the Client model to move towards your suggested solution in #1860 (comment) instead? It would still break old upgraded apps but at least the plain_secret is available so it would be an easy fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I see no good reason to change anything here, because secret will now contain the plain secret value that’s only visible right after creating a new client. The change on array|Client to stricter return type Client was intended, and still makes sense (We have already explained the return type changes on the upgrade guide). However if you insist to include plain_secret too, my suggestion is to append it. That's just my suggestion and the final decision is for maintainers to make.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @hafezdivandari

because secret will now contain the plain secret value that’s only visible right after creating a new client.

Unfortunately that's not true. My first attempt was to update the old Vue components to use secret directly, expecting it to return the plain secret in that case. However, it didn't.

// \Laravel\Passport\Client::secret

    protected function secret(): Attribute
    {
        return Attribute::make(
            set: function (?string $value): ?string {
                $this->plainSecret = $value;

                return $this->castAttributeAsHashedString('secret', $value);
            },
        );
    }

As you can see, it is getting hashed immediately in the setter. I don't think, we can leave it as is tbh.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, let me write a test for it and think for a solution, in the meantime, please mark this PR as draft.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marked as draft. Thank you for jumping in and taking care. 🚀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hettiger I just sent #1861, please let me know if it resolves the issue, thanks!

{
$this->validation->make($request->all(), [
'name' => ['required', 'string', 'max:255'],
Expand All @@ -53,9 +53,7 @@ public function store(Request $request): Client
$request->user(),
);

$client->secret = $client->plainSecret;

return $client->makeVisible('secret');
return ['plainSecret' => $client->plainSecret] + $client->toArray();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ['plainSecret' => $client->plainSecret] + $client->toArray();
$client->secret = $client->plainSecret;
return $client->makeVisible('secret')->mergeAppends(['plain_secret']);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this as well. However, it's again breaking existing code bases like mine with no good reason. This is deprecated code. Why change the behavior now just because you want to have the Client return type over array?

By the way: I think this will throw BadMethodCallException Call to undefined method Laravel\Passport\Client::getPlainSecretAttribute(); \Laravel\Passport\Client::$plainSecret is a property. It's not a getter.

}

/**
Expand Down
14 changes: 10 additions & 4 deletions tests/Unit/ClientControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function test_clients_can_be_stored()
$clients->shouldReceive('createAuthorizationCodeGrantClient')
->once()
->with('client name', ['http://localhost'], true, $user)
->andReturn($client = new Client);
->andReturn($client = new Client(['name' => 'client']));

$redirectRule = m::mock(RedirectRule::class);

Expand All @@ -70,7 +70,10 @@ public function test_clients_can_be_stored()
$clients, $validator, $redirectRule
);

$this->assertEquals($client, $controller->store($request));
$this->assertEquals([
'name' => $client->name,
'plainSecret' => $client->plainSecret,
], $controller->store($request));
}

public function test_public_clients_can_be_stored()
Expand All @@ -89,7 +92,7 @@ public function test_public_clients_can_be_stored()
$clients->shouldReceive('createAuthorizationCodeGrantClient')
->once()
->with('client name', ['http://localhost'], false, $user)
->andReturn($client = new Client);
->andReturn($client = new Client(['name' => 'client']));

$redirectRule = m::mock(RedirectRule::class);

Expand All @@ -109,7 +112,10 @@ public function test_public_clients_can_be_stored()
$clients, $validator, $redirectRule
);

$this->assertEquals($client, $controller->store($request));
$this->assertEquals([
'name' => $client->name,
'plainSecret' => $client->plainSecret,
], $controller->store($request));
}

public function test_clients_can_be_updated()
Expand Down