-
Notifications
You must be signed in to change notification settings - Fork 3
Add OAuth2 Support #27
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 OAuth2 Support #27
Conversation
7618077 to
ab3e88b
Compare
|
Hey @StanBarrows! My force push closed this PR. Are you able to reopen it, or should I create a new PR? |
|
Could you take a closer look at this? The new Bexio credentials for the dev setup are available in our 1Password instance. |
|
|
||
| return [ | ||
| 'auth' => [ | ||
| 'use_oauth2' => env('BEXIO_USE_OAUTH2', false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you strictly separate the token-based and OAuth2 approaches?
At the moment, we’re just passing the token like this to create a new connector:
new BexioConnector(token: $this->user->bexio_api_key);
I think it might be better to create a separate OAuth2Connector instead of combining everything into one connector. This also guarantees 100% that we won’t break any existing applications.
Also, please adjust the format of config/bexio.php to the following:
<?php
return [
'auth' => [
'token' => env('BEXIO_API_TOKEN'),
'oauth2' => [
'client_id' => env('BEXIO_OAUTH2_CLIENT_ID'),
'client_secret' => env('BEXIO_OAUTH2_CLIENT_SECRET'),
'email' => env('BEXIO_OAUTH2_EMAIL'),
'scopes' => [],
],
],
'route_prefix' => 'bexio',
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StanBarrows We could create a separate OAuth2Connector, but I would not worry about backwards compatibility, as the current BexioConnector can only use one approach or the other. So if the laravel app uses a PAT, then the OAuth2 logic will never be touched, and vice versa.
One thing that would be of a concern to me, would be if you would use OAuth2 on a per user basis, like in your example:
new BexioConnector(token: $this->user->bexio_api_key);
As the default setup of the PR assumes that the app only stores one authenticator (refresh token + access token + expiry data) globally per app in cache, and the only account that is authorised to create the authenticator, is the one defined with the email in config as BEXIO_OAUTH2_EMAIL. So if this package should also be used as a per user basis, the architecture needs to be adjusted. Key points to consider:
- Which bexio accounts should the app be able to generate an authenticator for, with the used Client ID and Client Secret?
- Should we support multiple authenticator storage methods for the consuming Laravel apps (cache, file, database), or should we default to encrypted cache storage and let the consuming app customise their storage logic by extending/override the
BexioOauthTokenStorageclass?
Regardless of the outcome, I think keeping everything to one connector, reduces the migration barrier by consuming apps, as they would only need to think about setting the .env variables and add the scopes to the bexio config that they need for the endpoints they use. Everything with connecting to bexio would then remain identical to how it was before.
Could you give me an example of a company or app, where multiple app users should have access tokens for bexio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our app, we need to connect to multiple Bexio instances per application. We have a typical SaaS application where each tenant connects to their own Bexio instance. That’s why the approach in the PR would not work for us at the moment. So, in order to get this merged, I need to ensure that the approach works for our at least our application.
We already created different packages — for example, laravel-docuware — where we handle OAuth dynamically, but refactoring this requires time. I’ve planned to work on this as soon as we have some capacity left, ideally within July.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StanBarrows Thanks for explaining the multi-tenancy requirement. I have a few ideas on how we could implement this:
-
Token Storage
- Abstract the token storage mechanism to support different storage backends
- Let consuming apps choose between cache (simple apps) or database storage (multi-tenant)
- Allow apps to implement custom storage solutions if needed
-
Multi-tenant Support
- Add tenant context to token operations
- Make the OAuth flow tenant-aware
- Support tenant-specific configurations
- Keep the current single-tenant approach as default for simpler apps
-
Configuration Flexibility
- Make email verification optional/customizable
- Allow tenant-specific OAuth settings
- Support custom success/error redirect flows
This approach would maintain backward compatibility while adding support for multi tennancy.
What are your thoughts on this? I could implement this on the current PR if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your message.
As we’ve already dealt with similar tenant workflows, I’d like to keep things consistent with our existing packages, where we use tenant-aware OAuth 2.0.
I’ve assigned @RhysLees to this task. He’ll look into it as soon as possible — likely not next week, but at the latest the week after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StanBarrows Alright, I will let @RhysLees take over from here. Or if you want this packages OAuth2 approach to be consistent with laravel-docuware, then I could also look into implementing it like that, and let @RhysLees review it when he is back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StanBarrows Is there an update on this?
| Route::middleware(['web'])->prefix(config('bexio.route_prefix', 'bexio'))->group(function () { | ||
| Route::get('/oauth/redirect', [BexioOAuthController::class, 'redirect'])->name('bexio.oauth.redirect'); | ||
| Route::get('/oauth/callback', [BexioOAuthController::class, 'callback'])->name('bexio.oauth.callback'); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Route::middleware(['web'])->prefix(config('bexio.route_prefix', 'bexio'))->group(function () {
Route::get('oauth/redirect', [BexioOAuthController::class, 'redirect'])->name('bexio.oauth.redirect');
Route::get('oauth/callback', [BexioOAuthController::class, 'callback'])->name('bexio.oauth.callback');
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed in the latest commit.
src/BexioServiceProvider.php
Outdated
| __DIR__.'/Http/Controllers/BexioOAuthController.php' => app_path('Http/Controllers/BexioOAuthController.php'), | ||
| ], 'bexio-controller'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Issue Temporary Fix:
// https://github.com/spatie/laravel-data/pull/699#issuecomment-1995546874
// https://github.com/spatie/laravel-data/issues/731
|
Hi @StanBarrows, @RhysLees, I’ve pushed some changes that implement multi-tenancy support, and I tried to make this PR aligned more with the conventions of laravel-docuware. I prioritised backward compatibility as well, so I kept the ability to pass I had to introduce a config resolver pattern to enable multi-tenancy for the OAuth2 redirect/callback flow, which allows for setting the configuration dynamically in any service provider in the consuming apps. I wanted to get these changes in before the next review cycle so the PR is as close as possible to your SaaS requirements. |
|
Closing this for. |
Overview
This PR introduces full OAuth2 Authorization Code Grant and OpenID Connect support to the Laravel Bexio package, along with secure encrypted token storage, centralized OAuth handling, and updated documentation.
🔑 Key Features
✅ OAuth2 Support
userinfoverification for enhanced identity validation.BexioOAuthController,BexioOAuthService,BexioOAuthTokenStoreFetchUserinfoRequest,UserinfoDTO,UserinfoVerificationException🧱 Integration with Existing Package
BexioConnectornow supports both PAT and OAuth2 modesAuthorizationCodeGrantandAlwaysThrowOnErrorstraitsconfig/bexio.phpBexioServiceProviderregisters views and routes for the new OAuth2 flow🔐 Secure Token Storage
CryptfacadeBexioTokenStorewithBexioOAuthTokenStore📚 Documentation
README.mdwith:🧪 Tests
BexioOAuthTokenStore)BexioOAuthController)userinfohandlingBexioOAuthService)Covers:
🛠 Migration Notes
README.mdfor full instructions.Closes #25