spiral / framework

High-Performance PHP Framework
https://spiral.dev
MIT License
1.82k stars 89 forks source link

Fix configuration replacement for auth in HttpAuthBootloader #1165

Open laxity7 opened 2 days ago

laxity7 commented 2 days ago

This pull request addresses an issue in the HttpAuthBootloader where configuration settings for authentication transports and storages defined in the auth.php config file are overridden by hardcoded defaults during the booting process.

According to the documentation, it should be possible to configure where the authentication token is stored (e.g., in a cookie or header) and customize related parameters, such as the token's name, through the configuration file, as shown:

return [
    'defaultStorage'   => 'session',
    'defaultTransport' => 'cookie',

    'transports' => [
        'header' => new HeaderTransport(header: 'X-Auth-Token-Custom'),
        'cookie' => new CookieTransport(cookie: 'token-custom', basePath: '/'),
    ],
];

However, these configuration settings are ignored, and the token is always stored in the default token cookie. Currently, the only way to change the cookie name is by overriding it in the AppBootloader.

Issue:

In the Spiral\Bootloader\Auth\HttpAuthBootloader::init() method, the configuration values are replaced by hardcoded defaults:

$kernel->booting(function () {
    $this->addTransport('cookie', $this->createDefaultCookieTransport());
    $this->addTransport('header', new HeaderTransport('X-Auth-Token'));
    $this->addTokenStorage('session', SessionTokenStorage::class);
});

Benefits:

Q A
Bugfix? ✔️
Breaks BC?
New feature?
msmakouz commented 2 days ago

The createDefaultCookieTransport method uses the HTTP config ($config = $this->config->getConfig(HttpConfig::CONFIG);). This change will cause it to be requested earlier, immediately upon calling setDefaults. After that, it will no longer be possible to modify it. I think this could become an issue. What do you think @butschster @roxblnfk ?

laxity7 commented 2 days ago

msmakouz I didn't change createDefaultCookieTransport, that's how it works now

msmakouz commented 2 days ago

msmakouz I didn't change createDefaultCookieTransport, that's how it works now

@laxity7 Yes, but the method’s invocation location has changed. Previously, it was called in the booting event, but now it’s called directly within the setDefaults method. As a result, it is now invoked earlier.

laxity7 commented 2 days ago

Ok, then the behaviour really changes. Do you have any ideas how to do it better?

msmakouz commented 1 day ago

@laxity7 Perhaps I'm wrong and everything is ok.

Changes are restricted once the config object has already been created: https://github.com/spiral/framework/blob/master/src/Config/src/ConfigManager.php#L63

But the getConfig method does not create it: https://github.com/spiral/framework/blob/master/src/Config/src/ConfigManager.php#L82

Sorry. I thought it would be better to fix this with a check in the event itself before adding. But it seems your approach is good too.