Skip to content

Conversation

@MattRink
Copy link

@MattRink MattRink commented Feb 8, 2018

Some Microsoft Office/Outlook OAUTH API's use Bearer tokens. I added the ability to specify the authorization type as either Bearer or none, defaulting to none (the current functionality).

@stevenmaguire
Copy link
Owner

Thanks for the PR! What is the use case for this functionality? Have you found this functionality is required to authorize and receive an access token? Have you found this functionality is required to make authorized requests against non-auth resources?

@MattRink
Copy link
Author

Hi, Internally we were using your package to wrap a series of Microsoft OAuth API's, one of them, the Outlook Mail API required the bearer type tokens, so I added it as an option which suited our use case. I thought you may be interested in the addition too.

@stevenmaguire stevenmaguire self-assigned this Feb 23, 2018
@DonCallisto
Copy link

Umh, I think it is always needed. I can see here that this Provider is used only for OAuth2 flows, so as getAuthorizationHeaders is used only this way https://github.com/thephpleague/oauth2-client/blob/master/src/Provider/AbstractProvider.php#L837 and token will be present only when fetching user infos, I suspspect that we should only implement

protected function getAuthorizationHeaders($token = null)
{
    return ['Authorization' => 'Bearer ' . $token];
}

Moreover (I'm gonna open a PR), now the endpoint /me to retrieve user infos has to be changed (as old one is deprecated) and new one needs that Bearer Token.

@DonCallisto
Copy link

Also response parsing for User should be adapted. Look here

@DonCallisto
Copy link

Moreover I'm wondering if could be a better choice to wrap the official SDK and delegate the heavy work to it, with this library to be only a wrapper/bridge/adapter/cohordinator (call it as you prefer) between league library and microsoft SDK.
WDYT?

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