Skip to content

🐛 fix: migrate Microsoft OAuth to v2.0 endpoints and Graph API #4551

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.

Sign up for GitHub

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

Open
mauretto78 wants to merge 1 commit into
base: develop
Choose a base branch
from
+47 −64
Open

🐛 fix: migrate Microsoft OAuth to v2.0 endpoints and Graph API #4551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 47 additions & 22 deletions lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,25 @@
<?php

namespace Model\ConnectedServices\Oauth\Microsoft;

use GuzzleHttp\Client;
use GuzzleHttp\Exception\GuzzleException;
use League\OAuth2\Client\Provider\Exception\IdentityProviderException;
use League\OAuth2\Client\Token\AccessToken;
use Model\ConnectedServices\Oauth\AbstractProvider;
use Model\ConnectedServices\Oauth\ProviderUser;
use Stevenmaguire\OAuth2\Client\Provider\Microsoft;
use Stevenmaguire\OAuth2\Client\Provider\MicrosoftResourceOwner;
use Utils\Registry\AppConfig;

class MicrosoftProvider extends AbstractProvider
{

const string PROVIDER_NAME = 'microsoft';

private const string AUTHORIZE_URL = 'https://login.microsoftonline.com/common/oauth2/v2.0/authorize';
private const string TOKEN_URL = 'https://login.microsoftonline.com/common/oauth2/v2.0/token';
private const string RESOURCE_OWNER_URL = 'https://graph.microsoft.com/v1.0/me';
private const string SCOPES = 'openid email profile User.Read';

/**
* @param string|null $redirectUrl
*
Expand All @@ -27,6 +31,9 @@
'clientId' => AppConfig::$MICROSOFT_OAUTH_CLIENT_ID,
'clientSecret' => AppConfig::$MICROSOFT_OAUTH_CLIENT_SECRET,
'redirectUri' => $redirectUrl ?? AppConfig::$MICROSOFT_OAUTH_REDIRECT_URL,
'urlAuthorize' => self::AUTHORIZE_URL,
'urlAccessToken' => self::TOKEN_URL,
'urlResourceOwnerDetails' => self::RESOURCE_OWNER_URL,
]);
}

Expand All @@ -37,53 +44,71 @@
*/
public function getAuthorizationUrl(string $csrfTokenState): string
{
$options = [
$params = [
'client_id' => AppConfig::$MICROSOFT_OAUTH_CLIENT_ID,
'redirect_uri' => $this->redirectUrl ?? AppConfig::$MICROSOFT_OAUTH_REDIRECT_URL,
'response_type' => 'code',
'scope' => self::SCOPES,
'state' => $csrfTokenState,
'prompt' => 'select_account'
'prompt' => 'select_account',
];

$microsoftClient = static::getClient($this->redirectUrl);

return $microsoftClient->getAuthorizationUrl($options);
return self::AUTHORIZE_URL . '?' . http_build_query($params);
}

/**
* @param string $code
*
* @return AccessToken
* @throws IdentityProviderException
* @throws GuzzleException
* @throws \RuntimeException
* @throws \InvalidArgumentException
*/
public function getAccessTokenFromAuthCode(string $code): AccessToken
{
$microsoftClient = static::getClient($this->redirectUrl);
$httpClient = new Client();

/** @var AccessToken $token */
$token = $microsoftClient->getAccessToken('authorization_code', [
'code' => $code
$response = $httpClient->post(self::TOKEN_URL, [
'form_params' => [
Comment on lines +69 to +72
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

Both methods instantiate a new GuzzleHttp\Client with default options. Without explicit timeout/connect_timeout, a slow or stalled external request can hang the PHP worker. Consider configuring sensible timeouts (and optionally reusing a single client instance) to reduce operational risk.

Copilot uses AI. Check for mistakes.
'client_id' => AppConfig::$MICROSOFT_OAUTH_CLIENT_ID,
'client_secret' => AppConfig::$MICROSOFT_OAUTH_CLIENT_SECRET,
'code' => $code,
'redirect_uri' => $this->redirectUrl ?? AppConfig::$MICROSOFT_OAUTH_REDIRECT_URL,
'grant_type' => 'authorization_code',
'scope' => self::SCOPES,
],
]);

return $token;
$data = json_decode($response->getBody()->getContents(), true);

Comment on lines +82 to +83
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

json_decode(..., true) can return null (invalid JSON) or a non-array value; passing that into new AccessToken($data) will cause a runtime failure (TypeError) or an AccessToken missing required fields. Consider decoding with JSON_THROW_ON_ERROR and explicitly validating that the response contains an access_token (and optionally expires_in) before constructing the AccessToken, throwing a clear exception otherwise.

Suggested change
$data = json_decode($response->getBody()->getContents(), true);
try {
$data = json_decode($response->getBody()->getContents(), true, 512, JSON_THROW_ON_ERROR);
} catch (\JsonException $e) {
throw new \RuntimeException('Invalid JSON received from Microsoft token endpoint.', 0, $e);
}
if (!is_array($data) || !isset($data['access_token']) || !is_string($data['access_token']) || $data['access_token'] === '') {
throw new \RuntimeException('Microsoft token endpoint returned an invalid access token payload.');
}

Copilot uses AI. Check for mistakes.
return new AccessToken($data);
}

/**
* @param AccessToken $token
*
* @return mixed
* @return ProviderUser
* @throws GuzzleException
* @throws IdentityProviderException
* @throws \RuntimeException
* @throws \TypeError
*/
public function getResourceOwner(AccessToken $token): ProviderUser
{
$microsoftClient = static::getClient($this->redirectUrl);
/** @var MicrosoftResourceOwner $fetched */
$fetched = $microsoftClient->getResourceOwner($token);
$httpClient = new Client();

$response = $httpClient->get(self::RESOURCE_OWNER_URL, [
'headers' => [
'Authorization' => 'Bearer ' . $token->getToken(),
],
]);

$data = json_decode($response->getBody()->getContents(), true);

$user = new ProviderUser();
$user->email = $fetched->getEmail();
$user->name = $fetched->getFirstname();
$user->lastName = $fetched->getLastname();
$user->picture = null; // profile picture is not publicly accessible
$user->email = $data['mail'] ?? $data['userPrincipalName'] ?? null;
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

ProviderUser::$email is declared as string, but this assignment can produce null (e.g., when both mail and userPrincipalName are missing). That will trigger a TypeError at runtime. Ensure a non-null string is assigned (or change the model to allow ?string and handle it consistently across providers).

Suggested change
$user->email = $data['mail'] ?? $data['userPrincipalName'] ?? null;
$user->email = $data['mail'] ?? $data['userPrincipalName'] ?? '';

Copilot uses AI. Check for mistakes.
$user->name = $data['givenName'] ?? null;
$user->lastName = $data['surname'] ?? null;
Comment on lines +109 to +110
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

ProviderUser::$name is declared as string, but $data['givenName'] ?? null can evaluate to null, which will cause a TypeError when assigned. Please provide a non-null fallback (or update ProviderUser typing and downstream consumers accordingly).

Suggested change
$user->name = $data['givenName'] ?? null;
$user->lastName = $data['surname'] ?? null;
$user->name = $data['givenName'] ?? '';
$user->lastName = $data['surname'] ?? '';

Copilot uses AI. Check for mistakes.
Comment on lines +105 to +110
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

Similar to the token exchange, the Graph /me response is decoded with json_decode(..., true) without checking for decode errors or expected shape. If the response body isn't valid JSON or isn't an array, the following field reads will yield null and can cascade into TypeErrors when populating ProviderUser. Consider using JSON_THROW_ON_ERROR and validating the decoded payload before mapping fields.

Copilot uses AI. Check for mistakes.
$user->picture = null;
$user->authToken = $token;
$user->provider = self::PROVIDER_NAME;

Expand Down
42 changes: 0 additions & 42 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8712,48 +8712,6 @@ parameters:
count: 1
path: lib/Model/ConnectedServices/Oauth/LinkedIn/LinkedInProvider.php

-
message: '#^Method Model\\ConnectedServices\\Oauth\\LinkedIn\\LinkedInProvider\:\:getResourceOwner\(\) throws checked exception TypeError but it''s missing from the PHPDoc @throws tag\.$#'
identifier: missingType.checkedException
count: 5
path: lib/Model/ConnectedServices/Oauth/LinkedIn/LinkedInProvider.php

-
message: '#^Method Model\\ConnectedServices\\Oauth\\Microsoft\\MicrosoftProvider\:\:getAccessTokenFromAuthCode\(\) throws checked exception UnexpectedValueException but it''s missing from the PHPDoc @throws tag\.$#'
identifier: missingType.checkedException
count: 1
path: lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php

-
message: '#^Method Model\\ConnectedServices\\Oauth\\Microsoft\\MicrosoftProvider\:\:getAuthorizationUrl\(\) throws checked exception InvalidArgumentException but it''s missing from the PHPDoc @throws tag\.$#'
identifier: missingType.checkedException
count: 1
path: lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php

-
message: '#^Method Model\\ConnectedServices\\Oauth\\Microsoft\\MicrosoftProvider\:\:getResourceOwner\(\) throws checked exception TypeError but it''s missing from the PHPDoc @throws tag\.$#'
identifier: missingType.checkedException
count: 3
path: lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php

-
message: '#^Method Model\\ConnectedServices\\Oauth\\Microsoft\\MicrosoftProvider\:\:getResourceOwner\(\) throws checked exception UnexpectedValueException but it''s missing from the PHPDoc @throws tag\.$#'
identifier: missingType.checkedException
count: 1
path: lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php

-
message: '#^PHPDoc tag @return with type mixed is not subtype of native type Model\\ConnectedServices\\Oauth\\ProviderUser\.$#'
identifier: return.phpDocType
count: 1
path: lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php

-
message: '#^Property Model\\ConnectedServices\\Oauth\\ProviderUser\:\:\$email \(string\) does not accept string\|null\.$#'
identifier: assign.propertyType
count: 1
path: lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php

-
message: '#^Property Model\\ConnectedServices\\Oauth\\ProviderUser\:\:\$name \(string\) does not accept string\|null\.$#'
identifier: assign.propertyType
Comment on lines 8712 to 8717
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

This hunk removes PHPStan baseline entries for both MicrosoftProvider and LinkedInProvider, but the PR description only mentions removing obsolete entries for MicrosoftProvider. If the LinkedIn baseline removal is intentional, please update the PR description accordingly; if not, the baseline change should be narrowed to Microsoft-related entries to avoid accidentally reintroducing LinkedIn findings.

Copilot uses AI. Check for mistakes.
Expand Down
Loading