🐛 fix: migrate Microsoft OAuth to v2.0 endpoints and Graph API#4551
🐛 fix: migrate Microsoft OAuth to v2.0 endpoints and Graph API#4551mauretto78 wants to merge 1 commit intodevelopfrom
Conversation
🧪 Test-Guard Report❌ FAIL — Some changed source files lack adequate test coverage. Coverage Analysis: ❌ FAILNo changed source files found in coverage report (threshold: 80%)
Test File Matching: ❌ FAILFile matching: 1 fail
Per-File Evaluation: ❌ FAILAll files resolved by deterministic shortcuts.
Result: ❌ FAIL Why this FAIL?
To resolve: add or update tests that cover lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php and ensure coverage configuration includes this file. |
There was a problem hiding this comment.
Pull request overview
Migrates Microsoft OAuth login away from deprecated Live endpoints to Microsoft Identity Platform v2.0 endpoints and Microsoft Graph /me, aiming to restore Microsoft sign-in.
Changes:
- Updated Microsoft OAuth authorization, token exchange, and user info retrieval to use v2.0 + Graph endpoints (with direct Guzzle calls).
- Cleaned up PHPStan baseline entries related to the OAuth provider implementation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.php | Replaces old Live/SDK-based flow with v2.0 authorization URL building, direct token exchange, and Graph /me profile fetch. |
| phpstan-baseline.neon | Removes baseline suppressions previously associated with MicrosoftProvider (and also LinkedInProvider in this hunk). |
| $user->name = $data['givenName'] ?? null; | ||
| $user->lastName = $data['surname'] ?? null; |
There was a problem hiding this comment.
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).
| $user->name = $data['givenName'] ?? null; | |
| $user->lastName = $data['surname'] ?? null; | |
| $user->name = $data['givenName'] ?? ''; | |
| $user->lastName = $data['surname'] ?? ''; |
| $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; | ||
| $user->name = $data['givenName'] ?? null; | ||
| $user->lastName = $data['surname'] ?? null; |
There was a problem hiding this comment.
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.
| $httpClient = new Client(); | ||
|
|
||
| /** @var AccessToken $token */ | ||
| $token = $microsoftClient->getAccessToken('authorization_code', [ | ||
| 'code' => $code | ||
| $response = $httpClient->post(self::TOKEN_URL, [ | ||
| 'form_params' => [ |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
| $data = json_decode($response->getBody()->getContents(), true); | ||
|
|
There was a problem hiding this comment.
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.
| $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.'); | |
| } |
| $user->name = $fetched->getFirstname(); | ||
| $user->lastName = $fetched->getLastname(); | ||
| $user->picture = null; // profile picture is not publicly accessible | ||
| $user->email = $data['mail'] ?? $data['userPrincipalName'] ?? null; |
There was a problem hiding this comment.
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).
| $user->email = $data['mail'] ?? $data['userPrincipalName'] ?? null; | |
| $user->email = $data['mail'] ?? $data['userPrincipalName'] ?? ''; |
Summary
Fixes Microsoft OAuth login by migrating from the deprecated Live API endpoints to Microsoft Identity Platform v2.0 and Microsoft Graph API.
Type
feat— new user-facing featurefix— bug fixrefactor— restructure without behavior changechore— build, deps, config, docsperf— performance improvementtest— test coverageChanges
lib/Model/ConnectedServices/Oauth/Microsoft/MicrosoftProvider.phplogin.live.com/apis.live.netendpoints with Microsoft v2.0 (login.microsoftonline.com) and Graph API (graph.microsoft.com/v1.0/me). Bypassed the brokenstevenmaguire/oauth2-microsoftlibrary by using direct Guzzle HTTP calls for token exchange and user info retrieval.phpstan-baseline.neonMicrosoftProviderthat no longer apply after the rewrite.Migration Notes
No migrations needed.
Testing
vendor/bin/phpunit --exclude-group=ExternalServices --no-coveragepasses./vendor/bin/phpstanpasses (0 errors, with baseline)Manual testing: requires a valid (non-expired) Microsoft OAuth client secret configured in
inc/oauth_config.ini.AI Disclosure
GitHub Copilot (Claude)
Notes
stevenmaguire/oauth2-microsoftlibrary is outdated and uses dead endpoints (apis.live.net/v5.0/me). This fix bypasses it with direct HTTP calls rather than replacing the dependency, to minimize risk.User.Readpermission granted.inc/oauth_config.ini.