Skip to content

Add OAuth providers migration#191

Merged
abnegate merged 27 commits into
mainfrom
add-oauth-providers-migration
Jun 4, 2026
Merged

Add OAuth providers migration#191
abnegate merged 27 commits into
mainfrom
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

greptile-apps Bot commented May 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces OAuth2 provider configuration migration between Appwrite projects. It adds a new OAuth2Provider resource class with a curated allow-list of safe-to-migrate fields per provider, source-side extraction via listOAuth2Providers, and destination-side direct DB writes that merge non-secret config (endpoints, prompts, tenant IDs) into the {key}Secret JSON blob while intentionally omitting client secrets.

  • OAuth2Provider uses a two-target field model (appId / secret) with per-provider rename mappings; client secrets and Apple's p8 file are never in scope, providing a clean security boundary.
  • mergeJsonSecret correctly guards the empty-fields case (returns existing string unchanged) and handles malformed JSON gracefully; the isConfigured filter on the source side ensures unconfigured providers are never emitted.
  • The destination applies one read-modify-write DB cycle per provider rather than batching all providers into a single update, which multiplies DB round trips for projects with many configured providers.

Confidence Score: 5/5

The change is safe to merge; it adds a self-contained new migration path with no modifications to existing resource types or write paths.

The core logic — allow-list field mapping, conditional secret merging, and isConfigured filtering — is correct and consistent with how auth methods and policies are handled in the same codebase. The null guard on listOAuth2Providers().providers is present, and mergeJsonSecret correctly short-circuits on empty fields.

No files require special attention; the implementation is straightforward and mirrors established patterns in the codebase.

Important Files Changed

Filename Overview
src/Migration/Resources/Auth/OAuth2/OAuth2Provider.php New resource class with a well-structured provider allow-list and safe field mapping; minor missing null-coalescing on $array['id'] in fromArray
src/Migration/Destinations/Appwrite.php Adds createOAuth2Provider and mergeJsonSecret using the established read-modify-write pattern; mergeJsonSecret is robust, but the per-provider DB read+write loop is N-fold instead of a single batch operation
src/Migration/Sources/Appwrite.php Adds exportOAuth2Providers / getOAuth2ProviderResources with correct null-guard on providers and proper isConfigured filtering; calculateResourcesReport count matches what's actually exported
src/Migration/Resource.php Adds TYPE_OAUTH2_PROVIDER constant with clear rationale (shared type avoids statusCounters column overflow); inserted into ALL_RESOURCES correctly
src/Migration/Transfer.php Adds TYPE_OAUTH2_PROVIDER to GROUP_AUTH_RESOURCES and the public resources array; no logic changes
tests/Migration/Unit/Adapters/MockDestination.php Registers TYPE_OAUTH2_PROVIDER in the mock destination's supported types list
tests/Migration/Unit/Adapters/MockSource.php Registers TYPE_OAUTH2_PROVIDER in the mock source's supported types list

Reviews (24): Last reviewed commit: "Remove OAuth provider unit tests" | 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)
…rage

Other migration resources (auth methods, policies, …) ship no per-resource
tests in this library; keep OAuth2 consistent with that baseline.
Comment thread src/Migration/Sources/Appwrite.php Outdated
Base automatically changed from add-email-templates-migration to main June 3, 2026 06:15

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we generalize and have 1 class for all? Much easier to maintain

Replace the per-provider class hierarchy (OAuth2Provider base +
StandardProvider/WithEndpointProvider + ~40 one-line subclasses) with a
single OAuth2Provider class driven by a `providerKey => non-secret fields`
map. Addresses review feedback to have one class for all providers.

- Source dispatch: drop the 42-entry class registry + oauth2ClassFor();
  build providers via OAuth2Provider::fromArray($key, $payload).
- Destination dispatch: replace the instanceof chain with a key check
  (Apple) + data-driven settings routing for endpoint/tenant/prompt.
- The field map doubles as a secret allow-list: only declared non-secret
  fields are copied off the listOAuth2Providers payload, so a future
  upstream secret field cannot leak into the migration.

Net 44 fewer files. Pint, PHPStan level 3, and the Unit suite all pass.
Comment thread src/Migration/Destinations/Appwrite.php Outdated
- Remove unused getSetting(); getDestinationAppId/SecretFields cover all callers.
- getDestinationAppId() now returns ?string (null when unset), so the
  destination drops its duplicate isEmptyOAuth2Setting() helper and isConfigured()
  reads as `enabled || appId !== null`.
- Document the PROVIDERS target/key routing so adding a provider is self-explanatory.
@abnegate abnegate merged commit 50526e1 into main Jun 4, 2026
4 checks passed
@abnegate abnegate deleted the add-oauth-providers-migration branch June 4, 2026 05:09
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.

2 participants