feat: enable users to regenerate Git deploy keys #41357
Conversation
WalkthroughAdds server-side SSH key persistence for artifacts, a new REST endpoint to save a user's SSH key pair to an artifact, and enhanced Git error classification that distinguishes SSH/authentication failures from network errors. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Ctrl as GitArtifactControllerCE
participant ArtSvc as ArtifactService
participant Session as SessionUserService
participant KeyRepo as GitDeployKeysRepository
participant Repo as Artifact Repository
User->>Ctrl: POST /{artifactId}/ssh-keypair?artifactType=...
Ctrl->>ArtSvc: saveSshKeyPair(artifactType, branchedArtifactId)
ArtSvc->>Session: getCurrentUser()
Session-->>ArtSvc: current user (email)
ArtSvc->>KeyRepo: findByEmail(userEmail)
KeyRepo-->>ArtSvc: GitDeployKeys (public/private)
ArtSvc->>ArtSvc: validate keys
ArtSvc->>Repo: save/update GitArtifactMetadata on artifact root
Repo-->>ArtSvc: updated Artifact
ArtSvc-->>Ctrl: Mono<Artifact>
Ctrl-->>User: 201 Created + ResponseDTO(artifact)
sequenceDiagram
participant GitOp as Git operation
participant Handler as CentralGitServiceCEImpl / GitRouteAspect
participant Detector as isInvalidSshKeyError()
GitOp->>Handler: operation fails (TransportException / Exception)
Handler->>Detector: isInvalidSshKeyError(error)
alt SSH/auth error
Detector-->>Handler: true
Handler-->>GitOp: map to INVALID_GIT_SSH_CONFIGURATION
else Network/connectivity error
Detector-->>Handler: false
Handler-->>GitOp: map to GIT_GENERIC_ERROR or GIT_ACTION_FAILED
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java (1)
368-368: Clear and actionable error message.The updated message effectively guides users to regenerate their deploy key when SSH authentication fails. The wording is appropriately instructive without being overly prescriptive.
If you want to be more explicit that this is specifically an authentication/authorization failure (as opposed to a network connectivity issue), consider refining the message to:
-"Appsmith couldn''t connect to this app''s Git repository. You may need to update the deploy key in the app''s Git settings and add the new key to your Git repository", +"Appsmith failed to authenticate with this app''s Git repository. Update the deploy key in the app''s Git settings and add the new key to your Git repository",However, the current phrasing with "may need to" is acceptable if you want to maintain a softer tone.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (1)
61-68: Consider simplifying the error handling.After calling
saveSshKeyToArtifact, you check if the artifact is connected to Git and throw an error with a generic message. SincesaveSshKeyToArtifactjust saved the SSH key, this check might be redundant. Consider whether this validation is necessary or if the method can simply return thegitAuthdirectly.app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java (2)
613-619: Preserve SSH failure context — When we remap toINVALID_GIT_SSH_CONFIGURATION, let’s feed the original message through so the toast/log keeps actionable details. For example:- if (isInvalidSshKeyError(error)) { - return Mono.error(new AppsmithException(AppsmithError.INVALID_GIT_SSH_CONFIGURATION)); + if (isInvalidSshKeyError(error)) { + return Mono.error(new AppsmithException( + AppsmithError.INVALID_GIT_SSH_CONFIGURATION, error.getMessage())); }This keeps the classification while retaining the actual failure text for support/debugging.
768-801: Factor shared SSH error detector — This helper duplicates the logic recently added toCentralGitServiceCEImpl. To avoid divergence on future tweaks (new keywords, casing, etc.), consider centralizing it (e.g., a shared utility undergit.utils) and reuse from both spots.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactController.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java(3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-09-20T07:55:30.235Z
Learnt from: sondermanish
Repo: appsmithorg/appsmith PR: 36413
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:514-518
Timestamp: 2024-09-20T07:55:30.235Z
Learning: In the `ImportServiceCEImpl` class, when throwing new `AppsmithException` instances, avoid adding the original `Throwable` as the cause due to messaging constraints.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
📚 Learning: 2024-12-10T10:52:38.873Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
📚 Learning: 2025-02-18T15:18:20.927Z
Learnt from: sharat87
Repo: appsmithorg/appsmith PR: 39337
File: app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java:154-160
Timestamp: 2025-02-18T15:18:20.927Z
Learning: Specific JSON parsing error handling using try-catch blocks isn't needed in the ImportServiceCEImpl's extractArtifactExchangeJson method as Mono.fromCallable provides adequate error handling.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java
📚 Learning: 2024-12-05T11:00:39.952Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 37984
File: app/client/src/git/requests/fetchGitMetadataRequest.ts:6-10
Timestamp: 2024-12-05T11:00:39.952Z
Learning: The `fetchGitMetadataRequest` function in `app/client/src/git/requests/fetchGitMetadataRequest.ts` does not require additional error handling or timeout configuration.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.javaapp/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
📚 Learning: 2024-12-05T10:56:13.739Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 37984
File: app/client/src/git/requests/fetchSSHKeyRequest.ts:5-9
Timestamp: 2024-12-05T10:56:13.739Z
Learning: In `app/client/src/git/requests/fetchSSHKeyRequest.ts`, the `fetchSSHKeyRequest` function does not require explicit error handling for `baseApplicationId`; the function should throw an error naturally if `baseApplicationId` is missing or invalid.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/exceptions/AppsmithError.java
🧬 Code graph analysis (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/GitUtils.java (1)
GitUtils(18-202)
⏰ 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: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (13)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceImpl.java (1)
14-19: Wiring updated correctly.Constructor now forwards the new Git deploy key and session-user dependencies to the CE base; looks aligned with the expanded signature.
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java (1)
26-35: LGTM! Well-documented API addition.The new
saveSshKeyPairmethod is clearly documented and follows the existing pattern. The Javadoc appropriately explains that this method fetches SSH keys from the database using the current user's email, ensuring the private key never travels through the client.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (3)
38-49: LGTM! Dependencies properly wired.The new dependencies (
GitDeployKeysRepositoryandSessionUserService) are correctly injected through the constructor and assigned to fields. This supports the new SSH key persistence functionality.
80-100: LGTM! Solid implementation of SSH key retrieval and validation.The method correctly:
- Fetches the SSH key from the database using the current user's email
- Provides clear error messages if the key is missing or invalid
- Validates that both public and private keys are present
- Delegates to the helper for saving
102-171: LGTM! Well-structured helper method.The
saveSshKeyToArtifacthelper correctly handles three scenarios:
- Root artifact with SSH key update
- Root artifact with new SSH key generation
- Branched artifact requesting SSH key update for root
The analytics event data has been appropriately updated to use artifact-centric terminology (
ARTIFACT,ARTIFACT_ID).app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (4)
80-84: LGTM! Necessary imports for network error detection.These network exception imports support the new
isInvalidSshKeyErrormethod's ability to distinguish SSH authentication failures from connectivity issues.
142-184: Excellent implementation of SSH error classification.The
isInvalidSshKeyErrormethod is well-designed:
- Logs at DEBUG level for troubleshooting without cluttering logs
- Comprehensively checks for SSH authentication indicators ("auth fail", "no more keys to try", "userauth")
- Explicitly excludes network/connectivity errors to reduce false positives
- Traverses the entire exception cause chain
This will significantly improve error messaging for users by distinguishing between "your SSH key is invalid" and "network is unreachable".
1114-1121: LGTM! Improved error handling for remote fetch.The error handling now distinguishes SSH authentication failures from other transport errors, providing more specific error messages to users. The conditional check using
isInvalidSshKeyErrorensures that network issues aren't misreported as SSH key problems.
1260-1261: LGTM! Consistent error handling across Git operations.The error handling updates across commit, status, and fetch operations follow a consistent pattern:
- Check if the error is a
TransportException- Use
isInvalidSshKeyErrorto distinguish SSH auth failures- Map to
INVALID_GIT_SSH_CONFIGURATIONfor SSH errors- Use appropriate context-specific errors for other cases
This consistency improves maintainability and provides clearer error messages to users.
Also applies to: 1755-1801, 2075-2107
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactControllerCE.java (2)
4-5: LGTM! Dependencies properly added.The new imports and
artifactServicefield support the new SSH key pair endpoint functionality.Also applies to: 7-7, 42-42
117-135: LGTM! Well-designed REST endpoint.The new
saveSshKeyPairendpoint is properly implemented:
- Comprehensive Javadoc explaining the security benefit
- RESTful path design
- Appropriate HTTP 201 CREATED status
- Proper delegation to the service layer
- Informative logging
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitArtifactController.java (1)
3-3: LGTM! Constructor properly updated for dependency injection.The constructor now accepts and forwards
ArtifactServiceto the superclass, enabling the new SSH key pair endpoint functionality.Also applies to: 16-18
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java (1)
560-565: Clear Redis failure surfacing — Thanks for mapping download failures toGIT_ROUTE_REDIS_DOWNLOAD_FAILED; this keeps the in-memory path aligned with existing error semantics.
Failed server tests
|
1 similar comment
Failed server tests
|
| 400, | ||
| AppsmithErrorCode.INVALID_GIT_SSH_CONFIGURATION.getCode(), | ||
| "SSH key is not configured correctly. Did you forget to add the SSH key to your remote repository? Please try again by reconfiguring the SSH key with write access.", | ||
| "Appsmith couldn''t connect to this app''s Git repository. You may need to update the deploy key in the app''s Git settings and add the new key to your Git repository", |
There was a problem hiding this comment.
Nit: multiple apostrophe ' present in the comments.
There was a problem hiding this comment.
It's required to escape the single quote. Ref:
https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/text/MessageFormat.html?utm_source=chatgpt.com#:~:text=Quoting%20Rules%20in,doesn%27t%20have%20them.
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/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (1)
153-169: Restore artifact emission after analytics side-effect.
Dropping the terminalthenReturn(artifact)means this branch now emits whateveranalyticsService.sendObjectEvent(...)yields (today that’s aMono<Void>), socreateOrUpdateSshKeyPair(...).map(...)receives a null artifact and throws when dereferencing it. Put the return of the saved artifact back on the happy path.return analyticsService .sendObjectEvent(AnalyticsEvents.GENERATE_SSH_KEY, artifact, data) .onErrorResume(e -> { log.warn("Error sending ssh key generation data point", e); - return Mono.just(artifact); - }); + return Mono.just(artifact); + }) + .thenReturn(artifact);
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java (1)
766-799: Avoid duplicating the SSH-key error classifier.CentralGitServiceCEImpl introduces the same
isInvalidSshKeyErrorhelper in this PR. Carrying two copies in separate classes will fall out of sync the next time someone tweaks the keyword list. Please consider extracting a shared utility (e.g., undergit.utils) and have both callers reuse it so future updates stay consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java(5 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-10T10:52:38.873Z
Learnt from: brayn003
Repo: appsmithorg/appsmith PR: 38060
File: app/client/src/git/sagas/fetchLocalProfileSaga.ts:8-13
Timestamp: 2024-12-10T10:52:38.873Z
Learning: In `app/client/src/git/sagas/fetchLocalProfileSaga.ts` and similar Git sagas, error handling for `baseArtifactId` is managed outside the scope, so validation checks for `baseArtifactId` within the saga functions are unnecessary.
Applied to files:
app/server/appsmith-server/src/main/java/com/appsmith/server/aspect/GitRouteAspect.java
⏰ 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). (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
Description
EE Counterpart: https://github.com/appsmithorg/appsmith-ee/pull/8313
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/19035218902
Commit: f77f153
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 03 Nov 2025 13:55:42 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements