feat(passport): add Frappe ID support - #318
Draft
prathameshkurunkar7 wants to merge 7 commits into
Draft
prathameshkurunkar7 wants to merge 7 commits into
prathameshkurunkar7 wants to merge 7 commits into
Conversation
The settings refused any http issuer, so Central could never be pointed at the local demo and the prototype worked around it by calling configure() directly — which meant the one path that mattered was never exercised. The opt-in is narrow: plain http only for a local hostname, and only when explicitly enabled. It does not permit http generally, and a test pins that.
Registration now names both of the site's own endpoints. The identity service no longer guesses where a site's sign-in begins or where its sign-out notices arrive; Central says, reusing core's own route helpers so those paths are defined in exactly one place. connect_central links as well as registers. Installing the registration alone put a "Continue with Frappe" button on Central's sign-in page that then refused every login, because a sign-in needs an identity link and nothing created one. It now links the operator running it and reports what it linked — Administrator can never hold a Frappe identity, so an operator signed in as one still has to name people, which link_cloud_users does. A daily reconcile withdraws registrations for sites that stopped running: the backstop for a Terminated event that never arrived, the same shape as the Atlas mirror's. It deliberately does not re-address a moved site, since re-addressing rotates the secret and only the site's own pull can install the new one.
The sign-in page lists identity providers alongside the existing social logins, and the dashboard links to the identity service's site picker when Frappe sign-in is configured.
…ient The registration payload carries the moved endpoints when a site is re-addressed; a site that stops running loses its registration through the reconcile alone; a named user is linked while system accounts, unknown addresses and disabled users are skipped. The provisioning fixtures moved to their own base class — subclassing the test class ran its assertions twice.
Joining a team should let you sign in to Central with your Frappe identity, so a team change queues the linking. Queued rather than inline: accepting an invitation must not fail because the identity service is briefly unreachable, and every team save reaches this handler. A daily pass links anyone who has since signed in to Frappe ID, which is what turns "invited before they had an identity" from a dead end into a wait. Central never creates an identity — only the person can. The pass is separate from the registration reconcile. One withdraws registrations for dead sites, the other links people; sharing a function made both harder to reason about and to test.
Removed the member linking functionality from the site registration process, as Central now only vouches for a site's address. Updated the registration payload to exclude member details and added logic to manage site registration based on status changes. Enhanced the handling of login URLs to ensure they are only provided when appropriate permissions are granted. Updated tests to reflect these changes and ensure proper functionality.
…egration Updated the site registration logic to improve handling of site statuses and streamline the reconciliation process. Replaced the site picker functionality with an identity portal, ensuring that the dashboard reflects the new structure. Adjusted related functions and types to support the changes, enhancing clarity and maintainability of the codebase.
prathameshkurunkar7
marked this pull request as draft
September 15, 2026 04:39
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+166
to
+170
| settings = CentralPassportSettings.active() | ||
|
|
||
| if record and record.enabled and settings: | ||
| record.subdomain = frappe.db.get_value("Site", site_name, "subdomain") | ||
| set_enabled(record, settings, False) |
Contributor
There was a problem hiding this comment.
If Passport enrollment is disabled before an enrolled site terminates, active() returns None, so both disable() and reconciliation skip the remote update. The existing Passport registration remains enabled indefinitely. Use retained operator settings for cleanup independently of the enrollment toggle.
How this was verified: Both termination cleanup paths require active settings, while changing the settings flag sends no remote disable request.
Prompt To Fix With AI
This is a comment left during a code review.
Path: central/integrations/passport.py
Line: 166-170
Comment:
**Disabled enrollment prevents revocation**
If Passport enrollment is disabled before an enrolled site terminates, `active()` returns `None`, so both `disable()` and reconciliation skip the remote update. The existing Passport registration remains enabled indefinitely. Use retained operator settings for cleanup independently of the enrollment toggle.
**How this was verified:** Both termination cleanup paths require active settings, while changing the settings flag sends no remote disable request.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+19
to
+23
| if not self.enabled: | ||
| return | ||
|
|
||
| if not self.issuer or not self.api_key or not self.api_secret: | ||
| frappe.throw(_("Frappe sign-in needs an issuer URL and operator credentials.")) |
Contributor
There was a problem hiding this comment.
Saving another valid issuer succeeds even after registrations exist, although the field documents that it cannot change. Registrations store client IDs without their original issuer, so subsequent moves and termination updates send those IDs to the new Passport instance instead of managing the original clients. Enforce issuer immutability once registrations exist, including while settings are disabled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: central/central/doctype/central_passport_settings/central_passport_settings.py
Line: 19-23
Comment:
**Issuer changes misroute existing clients**
Saving another valid issuer succeeds even after registrations exist, although the field documents that it cannot change. Registrations store client IDs without their original issuer, so subsequent moves and termination updates send those IDs to the new Passport instance instead of managing the original clients. Enforce issuer immutability once registrations exist, including while settings are disabled.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+175
to
+179
| PassportOperator(settings).readdress( | ||
| record.client_id, | ||
| title=_title({"name": record.name, "subdomain": record.subdomain}), | ||
| origin=record.origin, | ||
| enabled=enabled, |
Contributor
There was a problem hiding this comment.
Reconciliation can read a disabled Running registration at origin A, then a concurrent registration pull moves it to B. This update subsequently sends the captured origin A back to Passport while Central still records B, breaking the newly installed callback. Serialize lifecycle updates with registration pulls and reload the registration under the same lock before sending the update.
Prompt To Fix With AI
This is a comment left during a code review.
Path: central/integrations/passport.py
Line: 175-179
Comment:
**Lifecycle updates restore stale callbacks**
Reconciliation can read a disabled Running registration at origin A, then a concurrent registration pull moves it to B. This update subsequently sends the captured origin A back to Passport while Central still records B, breaking the newly installed callback. Serialize lifecycle updates with registration pulls and reload the registration under the same lock before sending the update.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+138
to
+142
| for record in drifted(enabled=1, status=GONE): | ||
| set_enabled(record, settings, False) | ||
|
|
||
| for record in drifted(enabled=0, status=["Running"]): | ||
| set_enabled(record, settings, True) |
Contributor
There was a problem hiding this comment.
A rejected Passport update aborts the entire pass, skipping unrelated registrations and the re-enable loop. One persistently invalid client can repeatedly obstruct cleanup for healthy clients. Isolate and log per-registration failures so the remaining records can still be processed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: central/integrations/passport.py
Line: 138-142
Comment:
**One failure aborts reconciliation**
A rejected Passport update aborts the entire pass, skipping unrelated registrations and the re-enable loop. One persistently invalid client can repeatedly obstruct cleanup for healthy clients. Isolate and log per-registration failures so the remaining records can still be processed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keeping this as a draft PR, to be merged after Atlas related changes are finished off in Central.