Skip to content

Add OAuth providers migration#191

Open
premtsd-code wants to merge 11 commits into
add-email-templates-migrationfrom
add-oauth-providers-migration
Open

Add OAuth providers migration#191
premtsd-code wants to merge 11 commits into
add-email-templates-migrationfrom
add-oauth-providers-migration

Conversation

@premtsd-code
Copy link
Copy Markdown
Contributor

Summary

  • Adds TYPE_OAUTH_PROVIDERS to GROUP_AUTH_RESOURCES for migrating the project's OAuth2 provider configuration map.
  • Source (Sources/Appwrite) reads $project->oAuthProviders and emits one OAuthProviders singleton carrying key/enabled/appId for each provider.
  • Destination (Destinations/Appwrite) merges the entries into the project doc's oAuthProviders map as flat {key}Enabled / {key}Appid keys via dbForPlatform (mirrors the destination path used by auth-methods / policies).
  • {key}Secret is intentionally not migrated — the source API never exposes secrets and the destination user must re-enter them post-migration. Same caveat as the SMTP password handling.
  • Grouped under GROUP_AUTH (not GROUP_INTEGRATIONS) — OAuth providers are auth methods that happen to use external identity providers; same group as TYPE_AUTH_METHODS and TYPE_POLICIES.

Test plan

  • CI lints + tests green
  • E2E testAppwriteMigrationOAuthProviders (in appwrite/appwrite) passes

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile Summary

This PR adds full OAuth2 provider migration support between Appwrite instances, covering all 40 providers. It introduces a clean three-tier class hierarchy (OAuth2ProviderStandardProviderWithEndpointProvider) with provider-specific subclasses for Apple, Google, and Microsoft, routing destination writes through instanceof dispatch to a single createOAuth2Provider method.

  • The source reads the provider list from listOAuth2Providers, maps each entry to a typed resource class via a static key→class lookup, and emits them under the single TYPE_OAUTH2_PROVIDER constant (shared to keep statusCounters compact).
  • The destination merges migrated fields into the project's oAuthProviders map using a read-then-merge pattern that preserves existing destination keys; clientSecret and p8 are never written since the source API does not expose them.
  • The Apple branch in createOAuth2Provider calls mergeAppleSecret unconditionally even when both getKeyId() and getTeamId() return '', changing a destination appleSecret of '' to '[]' — inconsistent with the non-empty guards used by all other extra-field providers.

Confidence Score: 3/5

The core migration logic is sound but the Apple branch has a concrete data mutation flaw that should be fixed before merging.

The Apple branch in createOAuth2Provider unconditionally calls mergeAppleSecret even when the source provides no key metadata, writing '[]' to a destination appleSecret field that was previously ''. This is inconsistent with how every other provider with optional extra fields guards its Secret write, and can cause the platform to treat an unconfigured Apple provider as partially configured.

src/Migration/Destinations/Appwrite.php — specifically the Apple branch in createOAuth2Provider around the unconditional mergeAppleSecret call.

Important Files Changed

Filename Overview
src/Migration/Destinations/Appwrite.php Adds createOAuth2Provider with per-shape branches. The Apple branch calls mergeAppleSecret unconditionally even when both keyId and teamId are empty, writing '[]' to an empty destination secret — inconsistent with how all other providers guard their extra-field writes.
src/Migration/Sources/Appwrite.php Adds exportOAuth2Providers with a static provider-class registry and lazy key→class map. $response->providers is accessed without a ?? [] guard unlike calculateReportCounts.
src/Migration/Resources/Auth/OAuth2/OAuth2Provider.php New abstract base class. Clean design: getName() returns the shared type constant, getProviderKey() is abstract.
src/Migration/Resources/Auth/OAuth2/Apple.php Correctly models Apple's bespoke shape (serviceId/keyId/teamId readable, p8File intentionally omitted).
src/Migration/Resource.php Adds TYPE_OAUTH2_PROVIDER constant with a clear rationale comment, inserted consistently into GROUP_AUTH_RESOURCES.
src/Migration/Transfer.php Adds TYPE_OAUTH2_PROVIDER to GROUP_AUTH_RESOURCES and ALL_PUBLIC_RESOURCES. Both additions are correct.
tests/Migration/Unit/Destinations/AppwriteOAuth2SecretTest.php Good unit coverage of mergeAppleSecret and mergeJsonSecret. Does not exercise the zero-metadata Apple case that triggers '' → '[]'.
tests/Migration/Unit/General/OAuth2ProviderTransferTest.php Transfer-level tests cover Google, Apple, Microsoft, and a plain StandardProvider with all provider-specific fields verified post-transfer.

Reviews (10): Last reviewed commit: "OAuth2: DRY secret merge, surface unmapp..." | Re-trigger Greptile

Comment thread src/Migration/Sources/Appwrite.php
Comment thread src/Migration/Sources/Appwrite.php Outdated
Comment thread src/Migration/Destinations/Appwrite.php Outdated
- Destination: dispatch via explicit case Resource::TYPE_OAUTH2_PROVIDER instead of default + instanceof
- Source: count in report() directly like sibling resources (drop try/catch), and move the in_array guard inside the export try
- Harden mergeAppleSecret/mergeJsonSecret against non-array decoded JSON
- Fix stale OAuth2Provider docblock (single shared TYPE, not per-subclass)
- mergeAppleSecret now delegates to mergeJsonSecret (one merge implementation)
- exportOAuth2Providers surfaces providers with no Resource class as non-fatal
  errors instead of dropping them silently
- report() counts only migratable providers; fix the misleading enabled comment
- use elseif for the mutually-exclusive provider-shape branches
- add AppwriteOAuth2SecretTest (secret-merge) and OAuth2ProviderTransferTest
  (transfer round-trip via MockSource/MockDestination)
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.

1 participant