[WEB-5657] feat: add synchronization configuration for multiple providers in authentication adapter

[pablohashescobar]

Description

• implement check_sync_enabled method to determine if sync is enabled for Google, GitHub, GitLab, Gitea providers
• update user data synchronization logic to utilize the new method
• add configuration variables for enabling sync for each provider in instance configuration files

Type of Change

• Improvement (change that would cause existing functionality to not work as expected)

Test Scenarios

• verify login/sign up with google, github, gitlab and gitea.

References

WEB-5657

Summary by CodeRabbit

• New Features

  • Added per-provider sync toggles in admin auth settings for Google, GitHub, GitLab and Gitea.

• Improvements

  • Optional automatic profile sync (display name and avatar) during login when enabled, with old avatar cleanup.
  • Forms now provide clearer save feedback and reliably reset to returned settings.
  • Better display-name generation for accounts missing a name.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[makeplane]

Linked to Plane Work Item(s)

• [WEB-5657] add idp sync functionality for all the authentication providers

This comment was auto-generated by Plane

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds ENABLE_*_SYNC toggles to admin provider forms and type defs, a reusable ControllerSwitch form component, backend sync and avatar handling in the auth adapter (with S3 bulk-delete support), a User.get_display_name helper, and new instance config variables for provider sync flags.

Changes

Cohort / File(s)	Summary
Admin Authentication Forms apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx, apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx, apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx, apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx	Add ENABLE_*_SYNC default values and *_FORM_SWITCH_FIELD descriptors; render ControllerSwitch; switch submit flow to async/await with try/catch; include sync flags in reset from response; adjust Save button handler to call handleSubmit correctly.
Admin Switch Component apps/admin/core/components/common/controller-switch.tsx	New generic ControllerSwitch component and TControllerSwitchFormField type; binds a labeled ToggleSwitch to react-hook-form and maps "0"/"1" string values to boolean.
Backend Auth Adapter apps/api/plane/authentication/adapter/base.py	Add check_sync_enabled(), sync_user_data(), delete_old_avatar(); refactor download_and_upload_avatar() to use S3Storage and guard missing URLs; integrate sync/avatar handling into complete_login_or_signup() and adjust save flow.
Backend Storage Utilities apps/api/plane/settings/storage.py	Add S3Storage.delete_files(object_names) to perform bulk S3 deletions via delete_objects with ClientError handling.
Backend Models & Config apps/api/plane/db/models/user.py, apps/api/plane/utils/instance_config_variables/core.py	Add User.get_display_name(cls, email) classmethod; add instance config variables ENABLE_GOOGLE_SYNC, ENABLE_GITHUB_SYNC, ENABLE_GITLAB_SYNC, ENABLE_GITEA_SYNC sourced from env (default "0", not encrypted).
Type Definitions packages/types/src/instance/auth.ts	Extend provider authentication configuration key unions to include ENABLE_GOOGLE_SYNC, ENABLE_GITHUB_SYNC, ENABLE_GITLAB_SYNC, ENABLE_GITEA_SYNC and update TInstanceAuthenticationConfigurationKeys.

Sequence Diagram

sequenceDiagram
    participant AdminUI as Admin UI
    participant Form as Admin Form
    participant API as Backend API
    participant DB as Database
    participant Adapter as Auth Adapter
    participant IDP as Identity Provider
    participant S3 as S3 Storage

    AdminUI->>Form: Toggle ENABLE_*_SYNC and submit
    Form->>API: POST provider config (includes ENABLE_*_SYNC)
    API->>DB: Persist config
    API-->>Form: Return updated config
    Form-->>AdminUI: Show toast and reset form

    Note over IDP,Adapter: Later — user authenticates via provider
    IDP->>Adapter: OAuth response (user data + avatar URL)
    Adapter->>Adapter: check_sync_enabled(provider)
    alt sync enabled
        Adapter->>Adapter: sync_user_data(user, idp_data)
        Adapter->>S3: download_and_upload_avatar(avatar_url)
        S3->>S3: delete_files(old_avatar_objects)
        S3-->>Adapter: return FileAsset or fail
        Adapter->>DB: save_user_data(updated profile, avatar ref)
    else sync disabled
        Adapter->>DB: save_user_data(minimal)
    end
    Adapter-->>API: complete login/signup response

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Inspect avatar lifecycle and sync gating in apps/api/plane/authentication/adapter/base.py (check_sync_enabled, sync_user_data, delete_old_avatar, download_and_upload_avatar).
• Verify S3Storage.delete_files correctness and error handling in apps/api/plane/settings/storage.py.
• Confirm ControllerSwitch value mapping ("0"/"1" ↔ boolean) and react-hook-form typings in apps/admin/core/components/common/controller-switch.tsx.
• Check admin forms for correct initialization, submission, and reset of ENABLE_*_SYNC fields in apps/admin/.../form.tsx.
• Validate User.get_display_name edge cases in apps/api/plane/db/models/user.py.
• Ensure TypeScript type updates in packages/types/src/instance/auth.ts align with runtime keys.

Poem

🐰 I toggled tiny switches bright,

Sync sprites hummed into the night,
Old avatars hopped out of sight,
New faces landed soft and light,
A joyful rabbit sings: byte!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding synchronization configuration for multiple providers in the authentication adapter, which aligns with the changeset scope.
Description check	✅ Passed	The description covers key aspects: implementation of check_sync_enabled, user data sync updates, and configuration variables. However, it lacks details on new components (ControllerSwitch UI), storage improvements (delete_files), and user model enhancements (get_display_name).

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch auth-sync

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc57b22 and 38f97d0.

📒 Files selected for processing (1)

• apps/api/plane/db/models/user.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• apps/api/plane/db/models/user.py

⏰ 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). (2)

• GitHub Check: Build packages
• GitHub Check: Analyze (javascript)

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

apps/api/plane/authentication/adapter/base.py (2)

186-188: Bug: Avatar content downloaded twice, using wrong variable for upload.

The code chunks the response into content (line 178) but then uses response.content (line 187) for upload. This causes the avatar to be downloaded twice and wastes bandwidth. Use the already-downloaded content variable.

             # Create file-like object
-            file_obj = BytesIO(response.content)
+            file_obj = BytesIO(content)
             file_obj.seek(0)

---

290-292: Fix inverted is_signup boolean logic.

Line 292 sets is_signup = bool(user), which is backwards. When a user is found (existing user, login flow), is_signup becomes True; when no user is found (new user, signup flow), it becomes False. This contradicts the codebase semantics where is_signup=True denotes a signup flow and is_signup=False denotes a login flow—as confirmed by usage in provider/credentials/email.py (which checks for existing users when is_signup=True and raises an error) and the email views (which pass is_signup=False for login and is_signup=True for signup).

Change line 292 to:

is_signup = user is None

🧹 Nitpick comments (10)

apps/api/plane/settings/storage.py (1)

191-201: Fix docstring and add type annotations for consistency.

The docstring says "Delete an S3 object" (singular) but the method deletes multiple objects. Also, this method lacks type annotations unlike the adjacent upload_file method.

-    def delete_files(self, object_names):
-        """Delete an S3 object"""
+    def delete_files(self, object_names: list[str]) -> bool:
+        """Delete multiple S3 objects in a single request"""

Additionally, be aware that AWS S3 delete_objects has a limit of 1000 objects per request. If callers may pass larger lists, consider batching.

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (2)

153-160: Add user feedback on error.

The error is only logged to console. Users won't know the save failed. Consider adding an error toast for better UX, matching the success toast pattern.

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save GitLab configuration. Please try again.",
+      });
     }

---

10-10: Unused import.

The cn utility is imported but not used in this file.

-import { cn } from "@plane/utils";

apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (2)

176-178: Add user feedback on error.

Same issue as other provider forms - errors are only logged to console. Add an error toast for user feedback.

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save GitHub configuration. Please try again.",
+      });
     }

---

12-12: Unused import.

The cn utility is imported but not used in this file.

-import { cn } from "@plane/utils";

apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (1)

151-165: Consider adding user feedback on configuration save failure.

The error is logged but the user receives no feedback when saving fails. For consistency with the success case, consider showing an error toast.

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save Google authentication configuration.",
+      });
     }

apps/admin/core/components/common/controller-switch.tsx (1)

29-35: Simplify the boolean comparison and consider adding a fallback for undefined values.

The === true comparison is redundant. Also, parseInt(undefined) returns NaN, which coerces to false - this works but is implicit.

           render={({ field: { value, onChange } }) => (
             <ToggleSwitch
-              value={Boolean(parseInt(value))}
-              onChange={() => (Boolean(parseInt(value)) === true ? onChange("0") : onChange("1"))}
+              value={Boolean(parseInt(value || "0"))}
+              onChange={() => (parseInt(value || "0") ? onChange("0") : onChange("1"))}
               size="sm"
             />
           )}

apps/api/plane/authentication/adapter/base.py (3)

112-134: Consider refactoring repetitive provider sync check to use a dictionary lookup.

The current implementation repeats the same pattern for each provider. A dict-based approach would be more maintainable.

     def check_sync_enabled(self):
         """Check if sync is enabled for the provider"""
-        if self.provider == "google":
-            (ENABLE_GOOGLE_SYNC,) = get_configuration_value([
-                {"key": "ENABLE_GOOGLE_SYNC", "default": os.environ.get("ENABLE_GOOGLE_SYNC", "0")}
-            ])
-            return ENABLE_GOOGLE_SYNC == "1"
-        elif self.provider == "github":
-            (ENABLE_GITHUB_SYNC,) = get_configuration_value([
-                {"key": "ENABLE_GITHUB_SYNC", "default": os.environ.get("ENABLE_GITHUB_SYNC", "0")}
-            ])
-            return ENABLE_GITHUB_SYNC == "1"
-        elif self.provider == "gitlab":
-            (ENABLE_GITLAB_SYNC,) = get_configuration_value([
-                {"key": "ENABLE_GITLAB_SYNC", "default": os.environ.get("ENABLE_GITLAB_SYNC", "0")}
-            ])
-            return ENABLE_GITLAB_SYNC == "1"
-        elif self.provider == "gitea":
-            (ENABLE_GITEA_SYNC,) = get_configuration_value([
-                {"key": "ENABLE_GITEA_SYNC", "default": os.environ.get("ENABLE_GITEA_SYNC", "0")}
-            ])
-            return ENABLE_GITEA_SYNC == "1"
-        return False
+        sync_keys = {
+            "google": "ENABLE_GOOGLE_SYNC",
+            "github": "ENABLE_GITHUB_SYNC",
+            "gitlab": "ENABLE_GITLAB_SYNC",
+            "gitea": "ENABLE_GITEA_SYNC",
+        }
+        key = sync_keys.get(self.provider)
+        if not key:
+            return False
+        (sync_enabled,) = get_configuration_value([
+            {"key": key, "default": os.environ.get(key, "0")}
+        ])
+        return sync_enabled == "1"

---

233-247: Inconsistent return value and redundant database lookup.

1. The method returns False on exception but implicitly returns None on success - this inconsistency could cause issues for callers.
2. Line 237 queries FileAsset.objects.get(pk=user.avatar_asset_id) when user.avatar_asset is already available from line 236's check.

     def delete_old_avatar(self, user):
         """Delete the old avatar if it exists"""
         try:
             if user.avatar_asset:
-                asset = FileAsset.objects.get(pk=user.avatar_asset_id)
+                asset = user.avatar_asset
                 storage = S3Storage(request=self.request)
                 storage.delete_files(object_names=[asset.asset.name])
                 user.avatar_asset = None
                 asset.delete()
                 user.save()
+            return True
         except FileAsset.DoesNotExist:
-            pass
+            return True
         except Exception as e:
             log_exception(e)
             return False

---

268-277: Avatar is always deleted and re-uploaded even if unchanged.

The current implementation deletes the old avatar and uploads a new one on every sync, even if the avatar URL hasn't changed. This wastes S3 operations and could cause brief avatar unavailability. Consider comparing the source URL before deletion.

         # Download and upload avatar only if the avatar is different from the one in the storage
         avatar = self.user_data.get("user", {}).get("avatar", "")
-        # Delete the old avatar if it exists
-        self.delete_old_avatar(user=user)
-        avatar_asset = self.download_and_upload_avatar(avatar_url=avatar, user=user)
-        if avatar_asset:
-            user.avatar_asset = avatar_asset
-        # If avatar upload fails, set the avatar to the original URL
-        else:
-            user.avatar = avatar
+        # Only update avatar if URL has changed
+        if avatar and avatar != user.avatar:
+            # Delete the old avatar if it exists
+            self.delete_old_avatar(user=user)
+            avatar_asset = self.download_and_upload_avatar(avatar_url=avatar, user=user)
+            if avatar_asset:
+                user.avatar_asset = avatar_asset
+            # If avatar upload fails, set the avatar to the original URL
+            else:
+                user.avatar = avatar

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 22339b9 and 0df31a8.

📒 Files selected for processing (10)

• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (5 hunks)
• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (5 hunks)
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (5 hunks)
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (5 hunks)
• apps/admin/core/components/common/controller-switch.tsx (1 hunks)
• apps/api/plane/authentication/adapter/base.py (5 hunks)
• apps/api/plane/db/models/user.py (1 hunks)
• apps/api/plane/settings/storage.py (1 hunks)
• apps/api/plane/utils/instance_config_variables/core.py (4 hunks)
• packages/types/src/instance/auth.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{ts,tsx,mts,cts}

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.{ts,tsx,mts,cts}: Use const type parameters for more precise literal inference in TypeScript 5.0+
Use the satisfies operator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicit is return types in filter/check functions
Use NoInfer<T> utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing in switch(true) blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacy experimentalDecorators
Use using declarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Use with { type: "json" } for import attributes; avoid deprecated assert syntax (TypeScript 5.3/5.8+)
Use import type explicitly when importing types to ensure they are erased during compilation, respecting verbatimModuleSyntax flag
Use .ts, .mts, .cts extensions in import type statements (TypeScript 5.2+)
Use import type { Type } from "mod" with { "resolution-mode": "import" } for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize new Set methods like union, intersection, etc., when available (TypeScript 5.5+)
Use Object.groupBy / Map.groupBy standard methods for grouping instead of external libraries (TypeScript 5.4+)
Use Promise.withResolvers() for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted, toSpliced, with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields via super in classes (TypeScript 5....

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/core/components/common/controller-switch.tsx
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• packages/types/src/instance/auth.ts

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Enable TypeScript strict mode and ensure all files are fully typed

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/core/components/common/controller-switch.tsx
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• packages/types/src/instance/auth.ts

**/*.{js,jsx,ts,tsx,json,css}

📄 CodeRabbit inference engine (AGENTS.md)

Use Prettier with Tailwind plugin for code formatting, run pnpm fix:format

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/core/components/common/controller-switch.tsx
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• packages/types/src/instance/auth.ts

**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/core/components/common/controller-switch.tsx
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• packages/types/src/instance/auth.ts

🧠 Learnings (3)

📚 Learning: 2025-07-14T11:22:43.964Z

Learnt from: gakshita
Repo: makeplane/plane PR: 7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.

Applied to files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx

📚 Learning: 2025-11-25T10:18:05.172Z

Learnt from: CR
Repo: makeplane/plane PR: 0
File: .github/instructions/typescript.instructions.md:0-0
Timestamp: 2025-11-25T10:18:05.172Z
Learning: Applies to **/*.{ts,tsx,mts,cts} : Utilize narrowing in `switch(true)` blocks for control flow analysis (TypeScript 5.3+)

Applied to files:

• apps/admin/core/components/common/controller-switch.tsx

📚 Learning: 2025-08-06T09:01:17.915Z

Learnt from: shivam-jainn
Repo: makeplane/plane PR: 7545
File: apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx:139-143
Timestamp: 2025-08-06T09:01:17.915Z
Learning: In the Plane project's Gitea authentication form (apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx), the GITEA_HOST field should not be included in the form reset after submission. It should default back to "https://gitea.com" rather than maintaining the submitted value, as this provides a sensible default for most users.

Applied to files:

• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

🧬 Code graph analysis (7)

apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (3)

apps/admin/core/components/common/controller-switch.tsx (2)

• TControllerSwitchFormField (11-14)
• ControllerSwitch (16-40)

packages/propel/src/toast/toast.tsx (1)

• setToast (245-265)

apps/space/core/store/root.store.ts (1)

• reset (62-75)

apps/admin/core/components/common/controller-switch.tsx (1)

packages/ui/src/button/toggle-switch.tsx (1)

• ToggleSwitch (56-56)

apps/api/plane/settings/storage.py (1)

apps/api/plane/utils/exception_logger.py (1)

• log_exception (9-20)

apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (1)

apps/admin/core/components/common/controller-switch.tsx (2)

• TControllerSwitchFormField (11-14)
• ControllerSwitch (16-40)

apps/api/plane/authentication/adapter/base.py (2)

apps/api/plane/settings/storage.py (2)

• S3Storage (15-201)
• delete_files (191-201)

apps/api/plane/db/models/user.py (4)

• save (150-168)
• User (42-176)
• get_display_name (171-176)
• avatar_url (129-137)

apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (1)

apps/admin/core/components/common/controller-switch.tsx (2)

• TControllerSwitchFormField (11-14)
• ControllerSwitch (16-40)

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)

apps/admin/core/components/common/controller-switch.tsx (2)

• TControllerSwitchFormField (11-14)
• ControllerSwitch (16-40)

⏰ 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: check:format
• GitHub Check: Build packages
• GitHub Check: Analyze (javascript)

🔇 Additional comments (14)

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)

15-16: LGTM on sync toggle integration.

The ENABLE_GITLAB_SYNC toggle is properly integrated: added to form state, defined as a switch field, rendered via ControllerSwitch, and included in the reset payload after submission. This follows the consistent pattern across other provider forms.

Also applies to: 46-46, 114-117, 194-194, 200-200

apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (2)

147-152: Verify GITEA_HOST reset behavior change is intentional.

Based on previous learnings, the GITEA_HOST field was intended to reset to the default "https://gitea.com" rather than the submitted value. This change now resets it from the response. Please confirm this behavioral change is intentional.

Also, similar to other provider forms, add an error toast for user feedback:

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save Gitea configuration. Please try again.",
+      });
     }

---

15-16: LGTM on sync toggle integration.

The ENABLE_GITEA_SYNC toggle is correctly integrated into the form following the same pattern as other provider forms.

Also applies to: 45-45, 109-112, 189-189, 194-194

apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (1)

17-18: LGTM on sync toggle integration.

The ENABLE_GITHUB_SYNC toggle is properly integrated following the consistent pattern across provider forms.

Also applies to: 48-48, 110-113, 212-212, 218-218

apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (5)

16-17: LGTM!

Imports are correctly added for the new ControllerSwitch component and its type definition.

---

43-47: LGTM!

The ENABLE_GOOGLE_SYNC field is correctly added to the form's default values, maintaining consistency with the existing configuration pattern.

---

99-102: LGTM!

The switch field descriptor is well-typed using TControllerSwitchFormField<GoogleConfigFormValues> which ensures type safety between the form values and the field configuration.

---

199-199: LGTM!

The ControllerSwitch is correctly wired with the form control and field configuration.

---

205-205: LGTM!

The onClick handler correctly wraps handleSubmit(onSubmit) with void to handle the async form submission while preserving proper event handling.

apps/admin/core/components/common/controller-switch.tsx (1)

6-14: LGTM!

The type definitions are well-structured with proper generic constraints, enabling type-safe usage across different form configurations.

apps/api/plane/utils/instance_config_variables/core.py (2)

47-52: LGTM!

The ENABLE_GOOGLE_SYNC configuration follows the established pattern, with a safe default of "0" (disabled) and correctly marked as non-encrypted since it's a boolean flag.

---

74-79: LGTM!

All provider sync flags (ENABLE_GITHUB_SYNC, ENABLE_GITLAB_SYNC, ENABLE_GITEA_SYNC) consistently follow the same pattern with safe defaults.

Also applies to: 102-107, 135-140

packages/types/src/instance/auth.ts (1)

19-40: LGTM!

Type definitions are correctly updated to include the new ENABLE_*_SYNC keys for all providers. The union types properly compose to form TInstanceAuthenticationConfigurationKeys.

apps/api/plane/authentication/adapter/base.py (1)

337-339: LGTM!

The sync integration correctly gates user data synchronization behind the check_sync_enabled() check, ensuring sync only occurs when explicitly enabled per provider.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

apps/api/plane/db/models/user.py (1)

170-178: Good fixes from previous review; avoid splitting email twice.

The previous issues with using self instead of cls and missing None checks have been addressed. However, the email is still split twice (lines 175 and 176), which is inefficient.

Apply this diff to store the split result:

 @classmethod
 def get_display_name(cls, email):
     if not email:
         return "".join(random.choice(string.ascii_letters) for _ in range(6))
+    parts = email.split("@")
     return (
-        email.split("@")[0]
-        if len(email.split("@")) == 2
+        parts[0]
+        if len(parts) == 2
         else "".join(random.choice(string.ascii_letters) for _ in range(6))
     )

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0df31a8 and 1601f76.

📒 Files selected for processing (1)

• apps/api/plane/db/models/user.py (1 hunks)

⏰ 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: check:lint
• GitHub Check: check:types
• GitHub Check: Analyze (javascript)

[Copilot]

Pull request overview

This PR adds synchronization configuration for multiple OAuth providers (Google, GitHub, GitLab, Gitea) in the authentication system. It introduces a mechanism to control whether user attributes (name, display name, avatar) should be refreshed from the OAuth provider during each sign-in.

Key changes:

• Adds check_sync_enabled method and sync_user_data method to the authentication adapter
• Adds ENABLE_{PROVIDER}_SYNC configuration variables for each OAuth provider
• Implements a new ControllerSwitch component for the admin UI to toggle sync settings
• Adds delete_files method to S3Storage for batch avatar deletion
• Adds get_display_name class method to User model for generating display names

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 22 comments.

Show a summary per file

File	Description
packages/types/src/instance/auth.ts	Adds TypeScript type definitions for new sync configuration keys for all four OAuth providers
apps/api/plane/utils/instance_config_variables/core.py	Adds configuration variables for enabling sync for Google, GitHub, GitLab, and Gitea with default value "0"
apps/api/plane/settings/storage.py	Adds delete_files method to S3Storage class for batch deletion of S3 objects
apps/api/plane/db/models/user.py	Adds get_display_name method to extract display name from email or generate fallback
apps/api/plane/authentication/adapter/base.py	Implements check_sync_enabled, sync_user_data, and delete_old_avatar methods; integrates sync into login flow
apps/admin/core/components/common/controller-switch.tsx	New reusable switch component for form-controlled toggle switches
apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx	Adds sync toggle UI, updates form handling to include ENABLE_GOOGLE_SYNC, improves error handling
apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx	Adds sync toggle UI, updates form handling to include ENABLE_GITLAB_SYNC, improves error handling
apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx	Adds sync toggle UI, updates form handling to include ENABLE_GITHUB_SYNC, improves error handling
apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx	Adds sync toggle UI, updates form handling to include ENABLE_GITEA_SYNC, improves error handling

Comments suppressed due to low confidence (1)

apps/api/plane/db/models/user.py:171

• Class methods or methods of a type deriving from type should have 'cls', rather than 'self', as their first parameter.

    def get_display_name(cls, email):

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The form defaultValues should provide a fallback value for ENABLE_GITHUB_SYNC in case config["ENABLE_GITHUB_SYNC"] is undefined. This could cause issues with the ControllerSwitch component which expects a string value to parse. Consider using 'config["ENABLE_GITHUB_SYNC"] || "0"' to ensure a valid default.

Suggested change

	ENABLE_GITHUB_SYNC: config["ENABLE_GITHUB_SYNC"],
	ENABLE_GITHUB_SYNC: config["ENABLE_GITHUB_SYNC"] || "0",

[coderabbitai]

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)

apps/api/plane/authentication/adapter/base.py (1)

286-286: Critical: is_signup logic is inverted, causing sync to run for new users instead of existing users.

Line 286 sets is_signup = bool(user), which is True when the user already exists (a login) and False when the user is None (a new signup). This naming is inverted from the semantic meaning.

The condition on line 332 not is_signup therefore triggers sync for new signups, not existing users. This causes:

1. Duplicate work: new user data was just set in lines 310-326
2. Missed sync: existing users logging in never get their IdP data synchronized

Fix the logic inversion:

         # Check if the user is present
         user = User.objects.filter(email=email).first()
-        # Check if sign up case or login
-        is_signup = bool(user)
+        # Check if this is an existing user (login) or new user (signup)
+        is_existing_user = user is not None
         # If user is not present, create a new user
         if not user:

Then update the sync condition:

-        # Check if IDP sync is enabled and user is not signing up
-        if self.check_sync_enabled() and not is_signup:
+        # Sync user data from IDP for existing users when enabled
+        if self.check_sync_enabled() and is_existing_user:
             user = self.sync_user_data(user=user)

And update the callback:

         # Call callback if present
         if self.callback:
-            self.callback(user, is_signup, self.request)
+            self.callback(user, not is_existing_user, self.request)

Also applies to: 331-333

♻️ Duplicate comments (1)

apps/api/plane/authentication/adapter/base.py (1)

263-271: Avatar deletion before download creates data loss risk.

The old avatar is deleted (line 265) before attempting to download the new one (line 266). If the download or upload fails, the user permanently loses their avatar asset and falls back to just a URL reference.

Delete the old avatar only after successfully uploading the new one:

         # Download and upload avatar only if the avatar is different from the one in the storage
         avatar = self.user_data.get("user", {}).get("avatar", "")
-        # Delete the old avatar if it exists
-        self.delete_old_avatar(user=user)
         avatar_asset = self.download_and_upload_avatar(avatar_url=avatar, user=user)
         if avatar_asset:
+            # Delete the old avatar only after successful upload
+            self.delete_old_avatar(user=user)
             user.avatar_asset = avatar_asset
         # If avatar upload fails, set the avatar to the original URL
         else:
             user.avatar = avatar

🧹 Nitpick comments (2)

apps/api/plane/authentication/adapter/base.py (2)

226-227: Consider using the existing FK reference directly.

user.avatar_asset already holds the FileAsset instance. The additional FileAsset.objects.get(pk=user.avatar_asset_id) query is redundant unless you're deliberately re-fetching to confirm DB state.

     def delete_old_avatar(self, user):
         """Delete the old avatar if it exists"""
         try:
             if user.avatar_asset:
-                asset = FileAsset.objects.get(pk=user.avatar_asset_id)
+                asset = user.avatar_asset
                 storage = S3Storage(request=self.request)
                 storage.delete_files(object_names=[asset.asset.name])

---

321-326: Clarify intent: both avatar_asset and avatar URL are stored on success.

Both the asset reference and the original URL are stored when upload succeeds (lines 322-323). If this is intentional as a fallback mechanism, a brief comment would clarify the design. Otherwise, line 323 duplicates line 326's fallback behavior.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1601f76 and 4576b20.

📒 Files selected for processing (1)

• apps/api/plane/authentication/adapter/base.py (5 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

apps/api/plane/authentication/adapter/base.py (4)

apps/api/plane/settings/storage.py (2)

• S3Storage (15-201)
• delete_files (191-201)

apps/api/plane/authentication/views/app/gitea.py (2)

• get (23-53)
• get (57-103)

apps/api/plane/db/models/asset.py (1)

• FileAsset (24-96)

apps/api/plane/db/models/user.py (4)

• save (150-168)
• User (42-178)
• get_display_name (171-178)
• avatar_url (129-137)

⏰ 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). (2)

• GitHub Check: Build packages
• GitHub Check: Analyze (javascript)

🔇 Additional comments (3)

apps/api/plane/authentication/adapter/base.py (3)

24-24: LGTM!

Moving the S3Storage import to module level improves clarity and follows Python import conventions.

---

94-96: LGTM!

Minor formatting improvement for readability.

---

112-124: LGTM!

Clean implementation using dictionary mapping for provider-to-config-key lookup. The configuration retrieval pattern is consistent with __check_signup.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (4)

apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (1)

46-46: Fallback value already present.

The past review comment suggested adding || "0" fallback, but the code already includes it. This ensures the ControllerSwitch component receives a valid string value to parse.

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)

45-45: Fallback value already present.

The past review comment suggested adding || "0" fallback, but it's already implemented correctly. This prevents issues with the ControllerSwitch component.

apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (2)

45-45: Fallback value already present.

The code already includes the || "0" fallback as suggested in past review comments, ensuring proper initialization for the ControllerSwitch component.

---

194-195: LGTM: Button props are correct.

The past review comment suggested adding size="lg", but it's already present on line 194. The void wrapper on line 195 correctly handles the async submission.

🧹 Nitpick comments (3)

apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (1)

161-177: Async/await refactor improves readability.

The migration from promise chaining to async/await with try/catch is cleaner. However, consider showing a user-facing error toast on failure for better UX, similar to the success toast.

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save GitHub authentication configuration. Please try again.",
+      });
     }

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (1)

144-160: Async/await refactor improves code clarity.

The change to async/await is cleaner than promise chaining. Consider adding an error toast for user feedback on failure, matching the success toast pattern.

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save GitLab authentication configuration. Please try again.",
+      });
     }

apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (1)

140-156: Async/await refactor improves maintainability.

The migration to async/await is cleaner. Consider adding a user-facing error toast for consistency with the success notification.

     } catch (err) {
       console.error(err);
+      setToast({
+        type: TOAST_TYPE.ERROR,
+        title: "Error!",
+        message: "Failed to save Gitea authentication configuration. Please try again.",
+      });
     }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4576b20 and 888b364.

📒 Files selected for processing (4)

• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (5 hunks)
• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (5 hunks)
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (5 hunks)
• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx (5 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• apps/admin/app/(all)/(dashboard)/authentication/google/form.tsx

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{ts,tsx,mts,cts}

📄 CodeRabbit inference engine (.github/instructions/typescript.instructions.md)

**/*.{ts,tsx,mts,cts}: Use const type parameters for more precise literal inference in TypeScript 5.0+
Use the satisfies operator to validate types without widening them
Leverage inferred type predicates to reduce the need for explicit is return types in filter/check functions
Use NoInfer<T> utility to block inference for specific type arguments when they should be determined by other arguments
Utilize narrowing in switch(true) blocks for control flow analysis (TypeScript 5.3+)
Rely on narrowing from direct boolean comparisons for type guards
Trust preserved narrowing in closures when variables aren't modified after the check (TypeScript 5.4+)
Use constant indices to narrow object/array properties (TypeScript 5.5+)
Use standard ECMAScript decorators (Stage 3) instead of legacy experimentalDecorators
Use using declarations for explicit resource management with Disposable pattern instead of manual cleanup (TypeScript 5.2+)
Use with { type: "json" } for import attributes; avoid deprecated assert syntax (TypeScript 5.3/5.8+)
Use import type explicitly when importing types to ensure they are erased during compilation, respecting verbatimModuleSyntax flag
Use .ts, .mts, .cts extensions in import type statements (TypeScript 5.2+)
Use import type { Type } from "mod" with { "resolution-mode": "import" } for specific module resolution contexts (TypeScript 5.3+)
Use new iterator methods (map, filter, etc.) if targeting modern environments (TypeScript 5.6+)
Utilize new Set methods like union, intersection, etc., when available (TypeScript 5.5+)
Use Object.groupBy / Map.groupBy standard methods for grouping instead of external libraries (TypeScript 5.4+)
Use Promise.withResolvers() for creating promises with exposed resolve/reject functions (TypeScript 5.7+)
Use copying array methods (toSorted, toSpliced, with) for immutable array operations (TypeScript 5.2+)
Avoid accessing instance fields via super in classes (TypeScript 5....

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Enable TypeScript strict mode and ensure all files are fully typed

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

**/*.{js,jsx,ts,tsx,json,css}

📄 CodeRabbit inference engine (AGENTS.md)

Use Prettier with Tailwind plugin for code formatting, run pnpm fix:format

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,jsx,ts,tsx}: Use ESLint with shared config across packages, adhering to max warnings limits per package
Use camelCase for variable and function names, PascalCase for components and types
Use try-catch with proper error types and log errors appropriately

Files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

🧠 Learnings (3)

📚 Learning: 2025-07-14T11:22:43.964Z

Learnt from: gakshita
Repo: makeplane/plane PR: 7393
File: apps/admin/app/(all)/(dashboard)/email/email-config-form.tsx:104-104
Timestamp: 2025-07-14T11:22:43.964Z
Learning: In the Plane project's SMTP configuration implementation, the email configuration form (email-config-form.tsx) hardcodes ENABLE_SMTP to "1" in form submission because the form is only rendered when SMTP is enabled. The enable/disable functionality is managed at the page level (page.tsx) with a toggle, and the form only handles configuration details when SMTP is already enabled.

Applied to files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx
• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

📚 Learning: 2025-10-09T20:42:31.843Z

Learnt from: lifeiscontent
Repo: makeplane/plane PR: 7922
File: apps/admin/app/(all)/(dashboard)/ai/form.tsx:19-19
Timestamp: 2025-10-09T20:42:31.843Z
Learning: In the makeplane/plane repository, React types are globally available through TypeScript configuration. Type annotations like React.FC, React.ReactNode, etc. can be used without explicitly importing the React namespace. The codebase uses the modern JSX transform, so React imports are not required for JSX or type references.

Applied to files:

• apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx

📚 Learning: 2025-08-06T09:01:17.915Z

Learnt from: shivam-jainn
Repo: makeplane/plane PR: 7545
File: apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx:139-143
Timestamp: 2025-08-06T09:01:17.915Z
Learning: In the Plane project's Gitea authentication form (apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx), the GITEA_HOST field should not be included in the form reset after submission. It should default back to "https://gitea.com" rather than maintaining the submitted value, as this provides a sensible default for most users.

Applied to files:

• apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx

🧬 Code graph analysis (1)

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (3)

apps/admin/core/components/common/controller-switch.tsx (2)

• TControllerSwitchFormField (11-14)
• ControllerSwitch (16-40)

packages/propel/src/toast/toast.tsx (1)

• setToast (245-265)

apps/space/core/store/root.store.ts (1)

• reset (62-75)

⏰ 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: check:lint
• GitHub Check: check:types
• GitHub Check: Analyze (javascript)

🔇 Additional comments (12)

apps/admin/app/(all)/(dashboard)/authentication/github/form.tsx (4)

15-16: LGTM: ControllerSwitch imports added correctly.

The type and component imports are necessary for the new synchronization toggle feature.

---

108-111: LGTM: Switch field definition is correct.

The GITHUB_FORM_SWITCH_FIELD properly defines the field descriptor for the ControllerSwitch component with appropriate name and label.

---

210-210: LGTM: ControllerSwitch integration is correct.

The component is properly bound to the form control and switch field descriptor.

---

216-216: LGTM: Void wrapper correctly handles async submit.

The void wrapper properly discards the promise returned by handleSubmit(onSubmit)(e) to satisfy event handler typing.

apps/admin/app/(all)/(dashboard)/authentication/gitlab/form.tsx (4)

14-15: LGTM: ControllerSwitch imports added correctly.

The imports are consistent with the other authentication forms and necessary for the new synchronization toggle feature.

---

113-116: LGTM: Switch field definition follows established pattern.

The GITLAB_FORM_SWITCH_FIELD is correctly defined and consistent with other provider forms.

---

193-193: LGTM: ControllerSwitch properly integrated.

The component is correctly bound to the form and renders the synchronization toggle.

---

199-199: LGTM: Void wrapper correctly handles async submission.

The void operator properly discards the promise for event handler compatibility.

apps/admin/app/(all)/(dashboard)/authentication/gitea/form.tsx (4)

15-16: LGTM: ControllerSwitch imports consistent with other forms.

The imports are correct and follow the established pattern across authentication forms.

---

109-112: LGTM: Switch field definition is correct.

The GITEA_FORM_SWITCH_FIELD follows the same pattern as other provider forms.

---

148-148: Verify GITEA_HOST reset behavior aligns with expectations.

Based on learnings, the GITEA_HOST field should default back to "https://gitea.com" rather than maintaining the submitted value. The current implementation resets to the response value, which may differ from the intended behavior.

Based on learnings, GITEA_HOST should not be included in the reset or should default to "https://gitea.com". Please confirm whether the current behavior is intentional.

---

189-189: LGTM: ControllerSwitch integration is correct.

The component is properly bound to form control and renders the synchronization toggle.