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 certificate support for Azure provider #379

Closed
wants to merge 3 commits into from
Closed

Add certificate support for Azure provider #379

wants to merge 3 commits into from

Conversation

uncaught
Copy link
Contributor

@uncaught uncaught commented Oct 1, 2022

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.

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)%'
Copy link

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.

Copy link

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!

Copy link
Contributor Author

@uncaught uncaught Oct 5, 2022

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

Copy link

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!

Copy link
Contributor Author

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.

@weaverryan weaverryan added Status: Waiting Feedback Needs feedback from the author Feature New Feature labels Oct 5, 2022
@weaverryan
Copy link
Member

This looks good to me - except two little code styling things to run:

A) php-cs-fixer fix --verbose --diff (once you have php-cs-fixer installed - you could even temporarily composer require it locally into this repo then just not commit the composer changes)

B) bin/update_readme - to regenerate part of the README

Cheers!

@weaverryan
Copy link
Member

Thanks @uncaught!

weaverryan added a commit that referenced this pull request Oct 10, 2022
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
@uncaught
Copy link
Contributor Author

All green :) Is there something else missing?

@weaverryan
Copy link
Member

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!

@weaverryan weaverryan closed this Oct 13, 2022
@quentinRogeret34
Copy link

Hello @weaverryan , I am really interested in this feature.
Do you know when you are going to release?

Thanks 🙏

@uncaught
Copy link
Contributor Author

Hello @weaverryan , I am really interested in this feature. Do you know when you are going to release?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Waiting Feedback Needs feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants