Adding ability to authenticate rest calls with bearer token #96
Conversation
change: load metadata using tenant or common, depending on presence of org_domain_hint
…. Added new settings resourceId and issuer. Added resourceId in Settings page
There was a problem hiding this comment.
First of all, apologies for the (huge) delay in this review. Second, thanks! I really like the idea of adding logic here to accept an access token issued by Azure AD (for the app in question).
I'm a bit confused by the implementation with regards to token validation. I noticed that you've added code to parse the JWT token. This is unnecessary, the JWT library already does this, and it already verifies the nbf and iat. So, a lot of this code is actually unnecessary.
I haven't quite looked at the code related to the REST request itself, I need to go learn a bit more about the filters you've added--if I have any comments on that, I'll update this.
This code could use some style clean-up, to ensure we're sticking to the WordPress coding standards: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/
| * Validates claims of bearer_token | ||
| * | ||
| * @param array $authentication_request_body The body to use in the Authentication Request. | ||
| * @param \AADSSO_Settings $settings The settings to use. |
There was a problem hiding this comment.
When this is used, Firebase\JWT::decode() has already decoded the JWT, validated the nbf and exp claims, and verified the signature. There is no need to do your own splitting, decoding or expiry validation.
AuthorizationHelper.php
Outdated
| * | ||
| * @param array $authentication_request_body The body to use in the Authentication Request. | ||
| * @param \AADSSO_Settings $settings The settings to use. | ||
| * |
There was a problem hiding this comment.
As mentioned above, this is all handled by Firebase\JWT, there's no need to do this yourself.
| return $retVal; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The @param doesn't match the method signature.
| return $jwt; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
@param doesn't match the method.
Settings.php
Outdated
| * @var string The OpenID Connect configuration discovery endpoint. | ||
| */ | ||
| public $openid_configuration_endpoint = 'https://login.microsoftonline.com/common/.well-known/openid-configuration'; | ||
| public $openid_configuration_endpoint_prefix = 'https://login.microsoftonline.com/'; |
There was a problem hiding this comment.
In daa9697, we made openid_configuration_endpoint a configurable setting. This allows the site to be either multi-tenant without external users (using common), or single-tenant with external users (using the tenanted endpoint). So, this is unnecessary.
AuthorizationHelper.php
Outdated
| * @return mixed The decoded authorization result. | ||
| */ | ||
| public static function validate_id_token( $id_token, $settings, $antiforgery_id ) { | ||
| private static function validate_token_signature( $id_token, $settings) { |
There was a problem hiding this comment.
This method does more than validate the token signature. (See comments below for more details.) The change to the method documentation isn't quite clear.
|
|
||
| /** | ||
| * @var string The resource ID obtained after registering an application in AAD. | ||
| */ |
There was a problem hiding this comment.
In which situation would the "resource ID" be different from the "client ID"?
Settings.php
Outdated
| */ | ||
| $openid_configuration = get_transient( 'aadsso_openid_configuration' ); | ||
| if( false === $openid_configuration || isset( $_GET['aadsso_reload_openid_config'] ) ) { | ||
| $tenant = !empty($instance->org_domain_hint) ? $instance->org_domain_hint : 'common'; |
There was a problem hiding this comment.
As mentioned on L120, this is unnecessary, since the OpenID Connect configuration endpoint is configurable.
| // on-the-fly). All that's left is to set the roles based on group membership. | ||
| if ( true === $this->settings->enable_aad_group_to_wp_role ) { | ||
| $user = $this->update_wp_user_roles( $user, $jwt->upn, $jwt->tid ); | ||
| $user = $this->update_wp_user_roles( $user, $jwt->oid, $jwt->tid ); |
There was a problem hiding this comment.
Indeed, good catch! (This was only working before because Azure AD Graph API lets you use the UPN as the object ID for most requests, but this wouldn't hold up for external users.)
|
Hi, regarding the resource id and client id they are different. When an application requests azureAD a token to authenticate with a second application (in our case our application request a token to authenticate with wordpress) then it uses the resource id to identify the resource. This information is stored in AUD field of the token and the receiving application has to verify that the token it receives is for itself. |
|
Is this so that AD bearer tokens can be used through the WP REST API? |
|
yes |
psignoret
force-pushed
the
master
branch
2 times, most recently
from
0320183 to
37c8428
Compare
3 participants
No description provided.