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

feat: add a configuration option to support clients that don't require consent #118

Merged

Conversation

lart2150
Copy link
Contributor

@lart2150 lart2150 commented Nov 4, 2024

for internal applications you might want to skip prompting for consent. With this change you can set your clients array like the array below to not prompt users for consent.

return array(
	'client_id_random_string' => array(
		'name' => 'The name of the Client',
		'secret' => 'a secret string',
		'redirect_uri' => 'https://example.com/redirect.uri',
		'grant_types' => array( 'authorization_code' ),
		'requires_consent' => false,
	),

@lart2150 lart2150 force-pushed the feature/support-no-consent-clients branch from 5550419 to bd9c700 Compare November 4, 2024 20:47
…e consent

for internal applications you might want to skip prompting for consent. With this change you can set your clients array like the array below to not prompt users for consent.

	return array(
		'client_id_random_string' => array(
			'name' => 'The name of the Client',
			'secret' => 'a secret string',
			'redirect_uri' => 'https://example.com/redirect.uri',
			'grant_types' => array( 'authorization_code' ),
			'requires_consent' => false,
		),
Copy link
Member

@ashfame ashfame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lart2150 for the PR! Looks good except a minor suggestion for a stricter check in place.

I can also just push that change myself, but waiting for a bit since I need to consult my colleagues on whether they also think this PR warrants a major bump in version since it introduces a breaking change.

src/Storage/ClientCredentialsStorage.php Outdated Show resolved Hide resolved
@ashfame
Copy link
Member

ashfame commented Nov 26, 2024

@akirk @psrpinto Looking for your input - this PR changes the constructor signature in AuthenticateHandler (from array $clients to ClientCredentialsStorage $clients) and AuthorizeHandler (added argument of ClientCredentialsStorage $clients).

Even though these classes might not be commonly instantiated directly, do you agree this warrants a major version bump given the breaking API changes?

@psrpinto
Copy link
Member

psrpinto commented Dec 2, 2024

At first sight this looks like a good change to me, but indeed, either we make it backwards-compatible (by accepting either an array or an instance of ClientCredentialsStorage), or we should bump the major version.

@lart2150
Copy link
Contributor Author

lart2150 commented Dec 2, 2024

At first sight this looks like a good change to me, but indeed, either we make it backwards-compatible (by accepting either an array or an instance of ClientCredentialsStorage), or we should bump the major version.

Are you targeting php 8 so I could type hint AuthorizeHandler/AuthenticateHandler
array|ClientCredentialsStorage $clients
I don't see a php version in the composer.json

@ashfame
Copy link
Member

ashfame commented Dec 4, 2024

@lart2150 That would be PHP 7.4, same as WordPress.

@lart2150
Copy link
Contributor Author

lart2150 commented Dec 4, 2024

Any preference how to handle the change? If we want to keep api compatibility I think we could do
AuthenticateHandler
public function __construct( ConsentStorage $consent_storage, mixed $clients ) {

AuthorizeHandler
public function __construct( OAuth2Server $server, ConsentStorage $consent_storage, ClientCredentialsStorage $clients = null) {

I'm not a big fan of using mixed. I can also revert the change to the AuthenticateHandler contractor and convert the array to ClientCredentialsStorage inside the constructor.

I think the last option is to go with the api change and bump the version accordingly.

@ashfame
Copy link
Member

ashfame commented Jan 6, 2025

Sorry for the long wait. Let's just bump the major version. Merging PR.

@ashfame ashfame self-requested a review January 6, 2025 08:23
@ashfame ashfame merged commit 7c0ddc4 into Automattic:main Jan 6, 2025
1 check passed
@ashfame ashfame mentioned this pull request Jan 6, 2025
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