Skip to content
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.

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

Add extensibility point for custom storage managers #145

Closed
wants to merge 2 commits into from

Conversation

Nattfarinn
Copy link
Contributor

@Nattfarinn Nattfarinn commented May 29, 2023

Bundle provides abstraction for custom mangers but it is actually not possible to plug in custom, non-Doctrine storage due to rigid Configuration (it only allows to define either in_memory or doctrine as persistence methods).

This simple workaround provides third option custom and exposes persistence method as a parameter. Intention is to be able to detect which persistence method is currently set in CompilerPass and declare proper service aliases.

Usage example:

league_oauth2_server:
    # ...
    persistence:
        custom: ibexa
$container->getParameter('league.oauth2_server.persistence.method'); // ibexa

Without extension point we would have to brute-force custom services regardless of actual configuration.

@chalasr
Copy link
Member

chalasr commented Jul 18, 2023

Thanks for the PR and apologies for the late feedback @Nattfarinn.
I support the intent for this, but I feel like we could provide a more structured extension point through the semantic config.

Without extension point we would have to brute-force custom services regardless of actual configuration.

Would you mind sharing some example to showcase how is this supposed to be used so that I can have the full picture? Such sample code could help documenting the feature also, which is needed to move forward anyway

@Nattfarinn
Copy link
Contributor Author

Nattfarinn commented Jul 31, 2023

Thanks for the PR and apologies for the late feedback @Nattfarinn. I support the intent for this, but I feel like we could provide a more structured extension point through the semantic config.

Yes, it would be best. I avoided any bigger changes to the configuration structure and implementation as I was worried about backward compatibility. Hence simple and crude (yet working) solution.

Without extension point we would have to brute-force custom services regardless of actual configuration.

Would you mind sharing some example to showcase how is this supposed to be used so that I can have the full picture? Such sample code could help documenting the feature also, which is needed to move forward anyway

On your side, selected persistence method does not match any switch/case so it does not load any related services definitions. On our side, we provide Symfony services for each Manager but we do not alias interfaces unless persistence method matches our identifier. We plug them in on-demand with CompilerPass instead:

final class PersistenceManagerPass implements CompilerPassInterface
{
    private const PERSISTENCE_METHOD = 'ibexa';

    public function process(ContainerBuilder $container): void
    {
        $persistenceMethod = $container->getParameter('league.oauth2_server.persistence.method');

        if ($persistenceMethod !== self::PERSISTENCE_METHOD) {
            return;
        }

        $container->setAlias(AccessTokenManagerInterface::class, AccessTokenManager::class);
        $container->setAlias(AuthorizationCodeManagerInterface::class, AuthorizationCodeManager::class);
        $container->setAlias(ClientManagerInterface::class, ClientManager::class);
        $container->setAlias(RefreshTokenManagerInterface::class, RefreshTokenManager::class);
        $container->setAlias(CredentialsRevokerInterface::class, TokenRevoker::class);
    }
}

@Nattfarinn
Copy link
Contributor Author

Nattfarinn commented Nov 30, 2023

@mtarld, do you require more information from our side? I like #171 approach. If you wish to proceed with it feel free to close this one. :)

@chalasr
Copy link
Member

chalasr commented Mar 10, 2024

Closing in favor of #171 as it's more flexible.
Thanks a lot for pushing this @Nattfarinn.

@chalasr chalasr closed this Mar 10, 2024
chalasr added a commit that referenced this pull request Mar 10, 2024
This PR was merged into the 0.4-dev branch.

Discussion
----------

Allow custom persistence managers

Added support for configuring custom persistence manager.
As improvement implementation on #145

**Configuration:**
```yaml
league_oauth2_server:
    persistence:
        custom:
            access_token_manager: App\Manager\MyAccessTokenManager
            authorization_code_manager: App\Manager\MyAuthorizationCodeManager
            client_manager: App\Manager\MyClientManager
            refresh_token_manager: App\Manager\MyRefreshTokenManager
            credentials_revoker: App\Service\MyCredentialsRevoker
```

**Code changes:**

- Extended the Configuration class.
- Added support in the Extension class.
- Skipped the doctrine extension check if `in_memory` or `custom` persistence is chosen.
- Added acceptance test, testing the different auth flows.
- Updated the `readme.md` to include instructions how to set the custom persistence managers.

**Implementation notes**
I would've really liked to make `doctrine/orm` _not_ a mandatory dependency but move it to the suggests section in composer.json. We have a project where we can't use Doctrine, and need to implement our own persistence manager, but now 6-7 doctrine packages will be included in the project. Could we move `doctrine/orm` to suggests section in a future version?

Commits
-------

81a32a0 Add configuration support for custom persistence
@Nattfarinn Nattfarinn deleted the custom-managers branch June 17, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants