verbb / consume

A Craft CMS plugin to create HTTP requests in your Twig templates to consume JSON, XML or CSV content.
Other
4 stars 1 forks source link

Generic Client is appending access_token as a URL parameter to the request #15

Closed jamesmacwhite closed 3 weeks ago

jamesmacwhite commented 3 weeks ago

Describe the bug

I'm using the Generic client for Microsoft Graph API. The Microsoft provider doesn't work due to needing to set a different Authorization URL and Token URL. I'm hitting an interesting scenario where when testing the fetchData with the client, the request being made appears to be appending access_token as a URL parameter, which was not requested in the fetchData call.

2024-07-03 22:18:22 [ERROR] Unable to fetch data: “Client error: `GET https://graph.microsoft.com/v1.0/sites/root/lists?access_token=[access_token]` resulted in a `404 Not Found` response:

This was how fetchData was called, the URI is relative to the configured default which is https://graph.microsoft.com/v1.0

Consume::$plugin->getService()->fetchData('microsoftSharePoint', 'GET', 'sites/root/lists');

I am not calling an options i.e. query to append anything, but it is being added. This causes the request to return a 404 and fail.

Steps to reproduce

  1. Configure Generic client with settings described
  2. Make request with fetchData and observe the access_token being appended as a URL query parameter.

Craft CMS version

5.2.1

Plugin version

2.0.0

Multi-site?

No response

Additional context

No response

jamesmacwhite commented 3 weeks ago

Looks like it's coming from here: https://github.com/verbb/auth/blob/craft-5/src/providers/Generic.php#L31-L37

Although, I don't want the access_token appended as a query param as it breaks the URI request for Microsoft Graph.

jamesmacwhite commented 3 weeks ago

I've been able to solve this by extending and adding a OAuth provider for Microsoft Graph and modifying the required areas. I need to return an empty array on getApiRequestQueryParams() to remove the query parameter and include:

protected function getAuthorizationHeaders($token = null): array
{
    return [
        'Authorization' => $token instanceof AccessToken ? sprintf('Bearer %s', $token->getToken()) : '',
    ];
}

For valid Guzzle client requests to work, along with modifying a few other areas.

I extended off the Microsoft class, but modified for Microsoft Graph API specifics. If you are interested I could potentially pull request it for inclusion in the main Auth module.

engram-design commented 2 weeks ago

Hmmm, I'm not exactly sure why I've made that a default query param, but I think it was because it's commonly used, and there wouldn't be harm in having it.

Glad you could get there in the end. I do need to cleanup some of these clients, as there's Azure, EntraID, Live, and Microsoft (Graph) which are all different, and some are outdated. I'm pretty sure the current Microsoft client is for the no-longer used Live login system.

jamesmacwhite commented 2 weeks ago

@engram-design The login.live.com URLs did seem old, I believe these days login.microsoftonline.com are the modern one's, however Microsoft Graph does still need some additional handling, mainly around apps which are defined in a single tenant and not multi-tenant, which is why I had to extend the class to resolve it and point to the authorization and token URLs to specific endpoints with our tenant ID, although #14 might be something to check, for less complicated customising.

Azure Active Directory is Entra ID now, for terminology, but Azure itself hasn't gone away.

I think the Live branding is phased out.