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

[hettiger]

Fixes ClientController::store() breaking change introduced via #1745

// \Laravel\Passport\Http\Controllers\ClientController::store

- if (Passport::$hashesClientSecrets) {
-     return ['plainSecret' => $client->plainSecret] + $client->toArray();
- }
+ $client->secret = $client->plainSecret;

return $client->makeVisible('secret');

This change obviously breaks usages that previously relied on the return type array with the additional plainSecret data. E.g., the old Vue components used plainSecret to present that to the user so that he could save it, etc. Since hashing is now mandatory, I restored the previous behavior without the now obsolete Passport::$hashesClientSecrets check:

return ['plainSecret' => $client->plainSecret] + $client->toArray();

I also updated the tests. I know it looks a bit fishy but I had not much choice since it's a unit test … (didn't want to make too big of a change out of this … it's deprecated anyways …)

[hafezdivandari]

BC change!

[hettiger]

@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?

[hafezdivandari]

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!

[hettiger]

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?

[hettiger]

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.

[hafezdivandari]

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.

[hettiger]

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.

[hafezdivandari]

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

[hettiger]

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

[hafezdivandari]

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

[hafezdivandari]

Suggested change

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

[hettiger]

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.