-
Notifications
You must be signed in to change notification settings - Fork 145
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 certificate support for Azure provider #379
Conversation
…etworg/oauth2-azure#170 for the respective PR on the provider).
README.md
Outdated
client_secret: '%env(OAUTH_AZURE_CLIENT_SECRET)%' | ||
# Using a client certificate requires thenetworg/oauth2-azure > 2.1.1: | ||
# client_certificate_private_key: '%env(OAUTH_AZURE_CLIENT_CERTIFICATE_PRIVATE_KEY)%' |
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.
for custom parameters I would suggest implement some client_extra_params
, because every provider could have some custom implementation.
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.
Or maybe some option to pass a client configured!
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.
Why is this different from all the other provider specific parameters like apple.key_file_id
etc.?
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.
IMO we we are just growing and growing this config!
for example
provider A, B, C, D ...: need the config
- X1
- X2
- X3
provider W: need the config
- X4
- X5
- X6
provider Y: need the config
- X7
- X8
- X9
most of the providers needs only the configs X1,2,3 But because few providers I need to add more and more config,
I don't see it as a good approach!
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.
Yes, but what exactly is the problem with that? The list of all the configs are only globally visible in the README.md, which simply lists all the providers with all their configs.
If you don't want to list all providers there anymore, then you could move the documentation to provider specific markdown files.
But the actual config parameters in the code were added in the provider specific configurator class. I don't see where this approach could be called growing out of hand.
The base configuration already only contains 4-5 options and leaves the rest to the provider specific configuration. If you want to change that, maybe by moving these 4-5 options to each provider individually and have no base config anymore, then that would be fine with me, but that would be a different PR then, setting a new standard for implementing providers.
This looks good to me - except two little code styling things to run: A) B) Cheers! |
Thanks @uncaught! |
This PR was squashed before being merged into the master branch. Discussion ---------- Add certificate support for Azure provider Adds certificate support to the **Azure provider**: - Makes configuration option `client_secret` optional - Adds configuration options `client_certificate_private_key` and `client_certificate_thumbprint` See TheNetworg/oauth2-azure#170 (merged) for the respective PR on the provider. That PR is not released, yet, but there are no compatibility issues with this PR here. They can be installed independently without drawbacks. Only to actually use certificates you had to have both PRs merged/released/installed. Commits ------- c7b5d12 Add certificate support for Azure provider
All green :) Is there something else missing? |
Thanks for the poke - this DID get merged in with a rebase... and for some reason my system didn't close the PR. It's all good and in! |
Hello @weaverryan , I am really interested in this feature. Thanks 🙏 |
Bare in mind, that TheNetworg/oauth2-azure#170 hasn't been released, yet, either. So you could install that repository from master or had to wait for their release, too. |
Adds certificate support to the Azure provider:
client_secret
optionalclient_certificate_private_key
andclient_certificate_thumbprint
See TheNetworg/oauth2-azure#170 (merged) for the respective PR on the provider.
That PR is not released, yet, but there are no compatibility issues with this PR here. They can be installed independently without drawbacks.
Only to actually use certificates you had to have both PRs merged/released/installed.