chore: Added endpoint and impl for ssh key pairs#39061
Conversation
WalkthroughThis pull request introduces a new SSH key retrieval method within the Artifact Service. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitControllerCE
participant ArtifactService
Client->>GitControllerCE: GET /ssh-key (branchedApplicationId)
GitControllerCE->>ArtifactService: getSshKey(artifactType, branchedArtifactId)
ArtifactService->>ArtifactService: Validate artifact and Git config
ArtifactService-->>GitControllerCE: Return GitAuthDTO or error
GitControllerCE-->>Client: Return ResponseDTO with SSH key details
sequenceDiagram
participant Client
participant GitControllerCE
participant ArtifactService
Client->>GitControllerCE: POST /ssh-key (branchedApplicationId, keyType)
GitControllerCE->>ArtifactService: createOrUpdateSshKeyPair(artifactType, branchedArtifactId, keyType)
ArtifactService-->>GitControllerCE: Return GitAuth response
GitControllerCE-->>Client: Return ResponseDTO with SSH key pair
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
| AppsmithError.INVALID_GIT_CONFIGURATION, | ||
| "Can't find valid SSH key. Please configure the artifact with git")); | ||
| } | ||
| // Check if the package is base package |
There was a problem hiding this comment.
Nit: replace package by artifact
| if (gitData.getDefaultArtifactId() == null) { | ||
| throw new AppsmithException( | ||
| AppsmithError.INVALID_GIT_CONFIGURATION, | ||
| "Can't find root package. Please configure the package with git"); |
There was a problem hiding this comment.
Nit: comment should be generic here
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (3)
128-174: Consider implementing rate limiting for key retrieval.Add rate limiting to prevent potential abuse of the SSH key retrieval endpoint.
138-142: Consolidate error handling logic.Similar error messages are repeated in multiple places. Consider extracting these into constants or a helper method.
+ private static final String GIT_CONFIG_ERROR_PREFIX = "Can't find "; + private static final String GIT_CONFIG_ERROR_SUFFIX = ". Please configure the artifact with git"; + private Mono<GitAuthDTO> handleGitConfigError(String message) { return Mono.error(new AppsmithException( AppsmithError.INVALID_GIT_CONFIGURATION, - "Can't find valid SSH key. Please configure the artifact with git")); + GIT_CONFIG_ERROR_PREFIX + message + GIT_CONFIG_ERROR_SUFFIX)); }Also applies to: 154-158
145-145: Consider externalizing the documentation URL.The Git deploy key documentation URL is hardcoded. Consider moving it to a configuration file for easier maintenance.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java (3)
260-266: Add API documentation for the endpoint.Consider adding Swagger/OpenAPI annotations to document the endpoint's purpose, parameters, and response.
@Operation(summary = "Retrieve SSH key pair for Git authentication") @ApiResponses(value = { @ApiResponse(responseCode = "200", description = "SSH key pair retrieved successfully"), @ApiResponse(responseCode = "404", description = "Application not found") })
268-275: Add API documentation for key generation endpoint.Similar to the GET endpoint, add Swagger/OpenAPI annotations here as well.
@Operation(summary = "Generate new SSH key pair for Git authentication") @ApiResponses(value = { @ApiResponse(responseCode = "201", description = "SSH key pair generated successfully"), @ApiResponse(responseCode = "404", description = "Application not found") })
260-276: Add API documentation.Consider adding Swagger/OpenAPI annotations to document the new endpoints, their parameters, and responses.
@ApiOperation(value = "Get SSH Key Pair", notes = "Retrieves the SSH key pair for the specified application") @ApiResponses(value = { @ApiResponse(code = 200, message = "SSH key pair retrieved successfully"), @ApiResponse(code = 404, message = "Application not found") })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationController.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationController.java (2)
3-3: LGTM! Import change aligns with the new SSH key functionality.The addition of ArtifactService import supports the new SSH key management capabilities.
16-19: LGTM! Constructor changes properly integrate the ArtifactService.The constructor signature update and super call modifications correctly implement the shift to using ArtifactService for SSH key management.
Let's verify the parent class implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Verify the parent class constructor and SSH key methods ast-grep --pattern 'class GitApplicationControllerCE { $$$ GitApplicationControllerCE($_, $_, $_) { $$$ } $$$ }'Length of output: 126
🏁 Script executed:
#!/bin/bash # Locate the parent class file for GitApplicationControllerCE by searching for its declaration in Java files. echo "Searching for file names containing GitApplicationControllerCE.java..." fd -e java "GitApplicationControllerCE.java" echo -e "\nSearching for class definition of GitApplicationControllerCE in all Java files..." rg "class GitApplicationControllerCE" -n .Length of output: 684
🏁 Script executed:
#!/bin/bash # Extract parent's constructor definition with some context to verify its signature and any references to ArtifactService rg "public GitApplicationControllerCE\\(" -A 15 app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.javaLength of output: 157
🏁 Script executed:
#!/bin/bash # Display the lines containing the parent's constructor in GitApplicationControllerCE.java to verify its signature and context. rg -n "GitApplicationControllerCE\\(" -A 10 app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.javaLength of output: 153
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of GitApplicationControllerCE.java to manually inspect the constructor signature and usage of ArtifactService head -n 200 app/server/appsmith-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.javaLength of output: 10305
Verification Outcome: Parent Constructor Correctly Integrates ArtifactService
- The parent class file (
GitApplicationControllerCE.java) declaresartifactServiceas a final field.- The use of Lombok’s
@RequiredArgsConstructorin the parent ensures an implicit constructor that acceptsCentralGitService,AutoCommitService, andArtifactService.- The child class constructor properly calls
super()with these dependencies.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java (2)
7-7: LGTM! Clean interface addition.The new
getSshKeymethod signature is well-defined and follows reactive programming patterns.Also applies to: 26-26
26-26: LGTM! Well-defined interface method.The new
getSshKeymethod follows reactive programming patterns and maintains consistency with existing interface design.app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (1)
147-150:⚠️ Potential issueConsider security implications of exposing private key.
The GitAuthDTO includes both public and private keys. Consider if exposing the private key is necessary, as it's typically kept secret.
| gitAuthDTO.setPublicKey(gitAuth.getPublicKey()); | ||
| gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey()); | ||
| gitAuthDTO.setDocUrl(gitAuth.getDocUrl()); |
There was a problem hiding this comment.
Remove private key from response in root artifact case.
Similar to the previous comment, the private key should not be exposed here either.
gitAuthDTO.setPublicKey(gitAuth.getPublicKey());
-gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey());
gitAuthDTO.setDocUrl(gitAuth.getDocUrl());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gitAuthDTO.setPublicKey(gitAuth.getPublicKey()); | |
| gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey()); | |
| gitAuthDTO.setDocUrl(gitAuth.getDocUrl()); | |
| gitAuthDTO.setPublicKey(gitAuth.getPublicKey()); | |
| gitAuthDTO.setDocUrl(gitAuth.getDocUrl()); |
| gitAuthDTO.setPublicKey(gitData.getGitAuth().getPublicKey()); | ||
| gitAuthDTO.setPrivateKey(gitData.getGitAuth().getPrivateKey()); | ||
| gitAuthDTO.setDocUrl(gitData.getGitAuth().getDocUrl()); | ||
| gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList); |
There was a problem hiding this comment.
Consider exposing only the public key in GitAuthDTO.
The private key should be kept secure and not exposed through the API. Consider removing the private key from the response DTO.
GitAuthDTO gitAuthDTO = new GitAuthDTO();
gitAuthDTO.setPublicKey(gitData.getGitAuth().getPublicKey());
-gitAuthDTO.setPrivateKey(gitData.getGitAuth().getPrivateKey());
gitAuthDTO.setDocUrl(gitData.getGitAuth().getDocUrl());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gitAuthDTO.setPublicKey(gitData.getGitAuth().getPublicKey()); | |
| gitAuthDTO.setPrivateKey(gitData.getGitAuth().getPrivateKey()); | |
| gitAuthDTO.setDocUrl(gitData.getGitAuth().getDocUrl()); | |
| gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList); | |
| gitAuthDTO.setPublicKey(gitData.getGitAuth().getPublicKey()); | |
| gitAuthDTO.setDocUrl(gitData.getGitAuth().getDocUrl()); | |
| gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList); |
| public Mono<ResponseDTO<GitAuthDTO>> getSSHKey(@PathVariable String branchedApplicationId) { | ||
| return artifactService | ||
| .getSshKey(ARTIFACT_TYPE, branchedApplicationId) | ||
| .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null)); |
There was a problem hiding this comment.
Fix HTTP status code for GET operation.
For GET operations, use HttpStatus.OK instead of HttpStatus.CREATED.
- .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null));
+ .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null)); | |
| .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)); |
| public Mono<ResponseDTO<GitAuthDTO>> getSSHKey(@PathVariable String branchedApplicationId) { | ||
| return artifactService | ||
| .getSshKey(ARTIFACT_TYPE, branchedApplicationId) | ||
| .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null)); | ||
| } |
There was a problem hiding this comment.
Adjust HTTP status code for GET endpoint.
The GET endpoint is using CREATED (201) status code. For GET requests, OK (200) is more appropriate.
- .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null));
+ .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Mono<ResponseDTO<GitAuthDTO>> getSSHKey(@PathVariable String branchedApplicationId) { | |
| return artifactService | |
| .getSshKey(ARTIFACT_TYPE, branchedApplicationId) | |
| .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null)); | |
| } | |
| public Mono<ResponseDTO<GitAuthDTO>> getSSHKey(@PathVariable String branchedApplicationId) { | |
| return artifactService | |
| .getSshKey(ARTIFACT_TYPE, branchedApplicationId) | |
| .map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null)); | |
| } |
| public Mono<ResponseDTO<GitAuth>> generateSSHKeyPair( | ||
| @PathVariable String branchedApplicationId, @RequestParam(required = false) String keyType) { | ||
| return artifactService | ||
| .createOrUpdateSshKeyPair(ARTIFACT_TYPE, branchedApplicationId, keyType) | ||
| .map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add request validation for keyType parameter.
The keyType parameter should be validated to ensure it contains only supported key types.
🏁 Script executed:
#!/bin/bash
# Search for supported key types in the codebase
rg -A 5 "getSupportedProtocols|GitDeployKeyGenerator"Length of output: 13815
Validation Required: Request Parameter keyType
The current implementation in the controller method directly forwards the keyType parameter without any explicit validation against the supported key types defined in GitDeployKeyGenerator (which only supports, for example, ECDSA and RSA). Please add a validation step to check if the incoming keyType value is among the allowed values before invoking the service logic.
- Validate keyType in
generateSSHKeyPair(or using a constraint/enum conversion) based onGitDeployKeyGenerator.getSupportedProtocols(). - Reject or gracefully handle any unsupported key types early in the request lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (1)
148-148:⚠️ Potential issueRemove private key from response.
The private key should not be exposed through the API for security reasons.
Apply this diff in both locations where GitAuthDTO is populated:
- gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey());Also applies to: 168-168
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (2)
133-134: Make error message more specific to artifact type.The error message should include the artifact type for better clarity.
- AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.ARTIFACT_ID, branchedArtifactId))) + AppsmithError.ACL_NO_RESOURCE_FOUND, artifactType.name().toLowerCase(), branchedArtifactId)))
146-150: Extract GitAuthDTO population to a helper method.The GitAuthDTO population logic is duplicated. Consider extracting it to a helper method.
+ private GitAuthDTO populateGitAuthDTO(GitAuth gitAuth, List<GitDeployKeyDTO> gitDeployKeyDTOList) { + GitAuthDTO gitAuthDTO = new GitAuthDTO(); + gitAuthDTO.setPublicKey(gitAuth.getPublicKey()); + gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey()); + gitAuthDTO.setDocUrl(gitAuth.getDocUrl()); + gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList); + return gitAuthDTO; + }Also applies to: 167-170
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java(3 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java (1)
7-7: LGTM!The new imports are properly organized and utilized in the implementation.
Also applies to: 14-15
sondermanish
added
ok-to-test
## Description - Added implementation for creating and retrieving ssh keys for git artifacts - Added endpoint for application to create or retrieve ssh keys. > [!WARNING] > _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.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13179515187> > Commit: 37f6866 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13179515187&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Thu, 06 Feb 2025 13:59:44 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced enhanced SSH key management, allowing users to easily retrieve and generate SSH key pairs. - New endpoints provide improved handling and validation of SSH key details, ensuring reliable key operations for artifacts. - Added functionality to retrieve SSH key information based on artifact type and branched artifact ID. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Warning
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.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13179515187
Commit: 37f6866
Cypress dashboard.
Tags:
@tag.GitSpec:
Thu, 06 Feb 2025 13:59:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit