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

Refactor authentication to prevent conflicts with plugins that determine the current user too early. #97

Merged
merged 3 commits into from
Mar 27, 2019

Conversation

bradyvercher
Copy link
Member

This seeks to work around conflicts with plugins that determine the current user during plugins_loaded by wiring up the determine_current_user filter immediately, so that authentication can be handled whenever wp_get_current_user() is called. See #94

Authentication servers have been refactored to simplify the interface and move state management and type gymnastics into the Authentication provider.

…rly. See #94

This seeks to work around conflicts with plugins that determine the current user
during plugins_loaded by wiring up the determine_current_user filter immediately,
so that authentication can be handled whenever wp_get_current_user() is called.

Authentication servers have been refactored to simplify the interface and move
state management and type gymnastics into the Authentication provider.
header( 'WWW-Authenticate: Basic realm="SatisPress"' );
}

$status_code = empty( $error_data['status'] ) ? HTTP::INTERNAL_SERVER_ERROR : $error_data['status'];
wp_die( wp_kses_data( $error->get_error_message() ), absint( $status_code ) );
wp_die( wp_kses_data( $e->getMessage() ), absint( $e->getStatusCode() ) );
Copy link
Collaborator

@GaryJones GaryJones Mar 26, 2019

Choose a reason for hiding this comment

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

Is there a way to let another class handle this sanitization and dieing, so that there is no need to link this class to WP code?
(There's an is_wp_error() further down as well).

I guess what I'm thinking of, is some sort of ErrorHandler interface, of which there could be a WPErrorHandler implementation that would actually do the wp_die(), that multiple classes could use.

Or moving this into an Exception class.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been thinking on this and can't really come up with a solution that isn't convoluted. The authentication handling was designed to be compatible with the REST API in case REST routes are registered in the future. The current wp_die() approach isn't really compatible with that, but neither is throwing an exception since authentication errors are retrieved through the rest_authentication_errors filter, which expects WP_Error, null, or true. It's also not possible to send headers through WP_Error.

Hmm, maybe the headers can be added when the HttpException is created in the authenticate() method. Then the handle_error() method can be dropped. Authentication::get_authentication_errors() can then convert the exception into an error object for REST requests, or rethrow the exception for the request handler to convert it into a response.

Copy link
Member Author

Choose a reason for hiding this comment

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

@GaryJones Let me know what you think of those changes. I'm not sure if the authentication exceptions are doing too much or not. This does simplify the logic in the servers quite a bit, though.

This creates a new AuthenticationException exception to transport authentication
errors, including headers and status code, from the authenticate() method
instead of using a handle_error() method.

If the current request is a REST request, the Authentication provider will
convert the exception to an error instead of allowing the exception to bubble
up.
@bradyvercher bradyvercher merged commit 870d8ab into develop Mar 27, 2019
@bradyvercher bradyvercher deleted the refactor-auth branch March 27, 2019 16:12
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.

2 participants