Use configured OAuth2 provider class from config#10900
Conversation
📝 WalkthroughWalkthroughThe pull request refactors OAuth2 provider class resolution from hard-coded namespace patterns to configuration-based lookup. In Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Areas requiring attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/account.php (1)
889-907: Avoid treating non‑OAuth sessions as unsupported providers inupdateSessionHere
$providercomes from the session and can be values likeanonymous,phone, etc., which are not present inConfig::getParam('oAuthProviders'). The new code will:
- Index
$oAuthProviders[$provider]['class']for these non‑OAuth providers (risking notices/warnings), and- Throw
PROJECT_PROVIDER_UNSUPPORTED, breakingPATCH /v1/account/sessions/:sessionIdfor non‑OAuth sessions.You only want to resolve/config‑check a class for providers that are actually registered in
oAuthProviders. A minimal fix:- $provider = $session->getAttribute('provider', ''); - $refreshToken = $session->getAttribute('providerRefreshToken', ''); - $oAuthProviders = Config::getParam('oAuthProviders'); - $className = $oAuthProviders[$provider]['class']; - if (!\class_exists($className)) { - throw new Exception(Exception::PROJECT_PROVIDER_UNSUPPORTED); - } - - if (!empty($provider) && \class_exists($className)) { + $provider = $session->getAttribute('provider', ''); + $refreshToken = $session->getAttribute('providerRefreshToken', ''); + $oAuthProviders = Config::getParam('oAuthProviders'); + $className = null; + + // Only treat providers registered in config as OAuth2 providers + if (!empty($provider) && isset($oAuthProviders[$provider])) { + $className = $oAuthProviders[$provider]['class'] ?? null; + + if (empty($className) || !\class_exists($className)) { + throw new Exception(Exception::PROJECT_PROVIDER_UNSUPPORTED); + } + } + + if (!empty($provider) && !empty($className)) { $appId = $project->getAttribute('oAuthProviders', [])[$provider . 'Appid'] ?? ''; $appSecret = $project->getAttribute('oAuthProviders', [])[$provider . 'Secret'] ?? '{}'; $oauth2 = new $className($appId, $appSecret, '', [], []); $oauth2->refreshTokens($refreshToken);This keeps the new config‑driven behavior for OAuth2 providers, while leaving email/phone/anonymous sessions unaffected.
🧹 Nitpick comments (1)
app/controllers/api/avatars.php (1)
91-107: Guard against missing/misconfigured OAuth provider config entryIndexing
$oAuthProviders[$provider]['class']assumes both the provider key andclassfield exist; if they don’t, PHP will emit notices before you throwPROJECT_PROVIDER_UNSUPPORTED. You can avoid that and keep the same behavior by coalescing and checking for an empty class name:- $oAuthProviders = Config::getParam('oAuthProviders'); - $className = $oAuthProviders[$provider]['class']; - if (!\class_exists($className)) { - throw new Exception(Exception::PROJECT_PROVIDER_UNSUPPORTED); - } + $oAuthProviders = Config::getParam('oAuthProviders'); + $className = $oAuthProviders[$provider]['class'] ?? null; + if (empty($className) || !\class_exists($className)) { + throw new Exception(Exception::PROJECT_PROVIDER_UNSUPPORTED); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(3 hunks)app/controllers/api/avatars.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/account.php (1)
src/Appwrite/Extend/Exception.php (1)
Exception(7-465)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: scan
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
🔇 Additional comments (1)
app/controllers/api/account.php (1)
1455-1482: Config‑driven OAuth2 class resolution in OAuth flows looks correctBoth the OAuth2 redirect handler and the OAuth2 token creation endpoint now:
- Resolve the provider class via
Config::getParam('oAuthProviders')[$provider]['class'], and- Throw
PROJECT_PROVIDER_UNSUPPORTEDwhen the configured class is missing.Because
provideris already constrained bynew WhiteList(array_keys(Config::getParam('oAuthProviders')), true)on both routes, the lookup is safe, and this aligns behavior across all OAuth entry points. No further changes needed here.Also applies to: 1955-2015
✨ Benchmark results
⚡ Benchmark Comparison
|
What does this PR do?
Use configured OAuth2 provider class from config like it is done here:
https://github.com/appwrite/appwrite/blob/1.8.x/app/controllers/api/account.php#L1317-L1321
Checklist