Skip to content

chore: Added endpoint and impl for ssh key pairs #39061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sondermanish merged 2 commits into from
Feb 7, 2025
Merged

chore: Added endpoint and impl for ssh key pairs #39061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1dc3abc
added endpoint and impl for ssh key pairs
sondermanish Feb 6, 2025
37f6866
added review comments
sondermanish Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...r/appsmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitAuth;
import com.appsmith.server.dtos.GitAuthDTO;
import reactor.core.publisher.Mono;

public interface ArtifactServiceCE {
Expand All @@ -21,4 +22,6 @@ public interface ArtifactServiceCE {
* @return public key which will be used by user to copy to relevant platform
*/
Mono<GitAuth> createOrUpdateSshKeyPair(ArtifactType artifactType, String branchedArtifactId, String keyType);

Mono<GitAuthDTO> getSshKey(ArtifactType artifactType, String branchedArtifactId);
}
59 changes: 59 additions & 0 deletions ...psmith-server/src/main/java/com/appsmith/server/artifacts/base/ArtifactServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@
import com.appsmith.server.artifacts.base.artifactbased.ArtifactBasedService;
import com.appsmith.server.artifacts.permissions.ArtifactPermission;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.Assets;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.domains.Application;
import com.appsmith.server.domains.ApplicationMode;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.domains.GitAuth;
import com.appsmith.server.dtos.GitAuthDTO;
import com.appsmith.server.dtos.GitDeployKeyDTO;
import com.appsmith.server.exceptions.AppsmithError;
import com.appsmith.server.exceptions.AppsmithException;
import com.appsmith.server.helpers.GitDeployKeyGenerator;
Expand All @@ -19,6 +22,7 @@
import org.springframework.util.StringUtils;
import reactor.core.publisher.Mono;

import java.util.List;
import java.util.Map;

@Slf4j
Expand Down Expand Up @@ -113,4 +117,59 @@ public Mono<GitAuth> createOrUpdateSshKeyPair(
})
.thenReturn(gitAuth);
}

/**
* Method to get the SSH public key
*
* @param branchedArtifactId artifact for which the SSH key is requested
* @return public SSH key
*/
@Override
public Mono<GitAuthDTO> getSshKey(ArtifactType artifactType, String branchedArtifactId) {
ArtifactBasedService<?> artifactBasedService = getArtifactBasedService(artifactType);
ArtifactPermission artifactPermission = artifactBasedService.getPermissionService();
return artifactBasedService
.findById(branchedArtifactId, artifactPermission.getEditPermission())
.switchIfEmpty(Mono.error(new AppsmithException(
AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.ARTIFACT_ID, branchedArtifactId)))
.flatMap(artifact -> {
GitArtifactMetadata gitData = artifact.getGitArtifactMetadata();
List<GitDeployKeyDTO> gitDeployKeyDTOList = GitDeployKeyGenerator.getSupportedProtocols();
if (gitData == null) {
return Mono.error(new AppsmithException(
AppsmithError.INVALID_GIT_CONFIGURATION,
"Can't find valid SSH key. Please configure the artifact with git"));
}
// Check if the artifact is base artifact
if (branchedArtifactId.equals(gitData.getDefaultArtifactId())) {
gitData.getGitAuth().setDocUrl(Assets.GIT_DEPLOY_KEY_DOC_URL);
GitAuthDTO gitAuthDTO = new GitAuthDTO();
gitAuthDTO.setPublicKey(gitData.getGitAuth().getPublicKey());
gitAuthDTO.setPrivateKey(gitData.getGitAuth().getPrivateKey());
gitAuthDTO.setDocUrl(gitData.getGitAuth().getDocUrl());
gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList);
Comment on lines +147 to +150
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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);

return Mono.just(gitAuthDTO);
}

if (gitData.getDefaultArtifactId() == null) {
throw new AppsmithException(
AppsmithError.INVALID_GIT_CONFIGURATION,
"Can't find root artifact. Please configure the artifact with git");
}

return artifactBasedService
.findById(gitData.getDefaultArtifactId(), artifactPermission.getEditPermission())
.map(baseArtifact -> {
GitAuthDTO gitAuthDTO = new GitAuthDTO();
GitAuth gitAuth =
baseArtifact.getGitArtifactMetadata().getGitAuth();
gitAuth.setDocUrl(Assets.GIT_DEPLOY_KEY_DOC_URL);
gitAuthDTO.setPublicKey(gitAuth.getPublicKey());
gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey());
gitAuthDTO.setDocUrl(gitAuth.getDocUrl());
Comment on lines +167 to +169
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
gitAuthDTO.setPublicKey(gitAuth.getPublicKey());
gitAuthDTO.setPrivateKey(gitAuth.getPrivateKey());
gitAuthDTO.setDocUrl(gitAuth.getDocUrl());
gitAuthDTO.setPublicKey(gitAuth.getPublicKey());
gitAuthDTO.setDocUrl(gitAuth.getDocUrl());

gitAuthDTO.setGitSupportedSSHKeyType(gitDeployKeyDTOList);
return gitAuthDTO;
});
});
}
}
6 changes: 3 additions & 3 deletions ...th-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package com.appsmith.server.git.controllers;

import com.appsmith.server.artifacts.base.ArtifactService;
import com.appsmith.server.constants.Url;
import com.appsmith.server.git.autocommit.AutoCommitService;
import com.appsmith.server.git.central.CentralGitService;
import com.appsmith.server.git.utils.GitProfileUtils;
import lombok.extern.slf4j.Slf4j;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RestController;
Expand All @@ -14,7 +14,7 @@
public class GitApplicationController extends GitApplicationControllerCE {

public GitApplicationController(
CentralGitService centralGitService, GitProfileUtils gitProfileUtils, AutoCommitService autoCommitService) {
super(centralGitService, gitProfileUtils, autoCommitService);
CentralGitService centralGitService, AutoCommitService autoCommitService, ArtifactService artifactService) {
super(centralGitService, autoCommitService, artifactService);
}
}
25 changes: 22 additions & 3 deletions ...-server/src/main/java/com/appsmith/server/git/controllers/GitApplicationControllerCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,23 @@
import com.appsmith.external.git.constants.ce.RefType;
import com.appsmith.external.views.Views;
import com.appsmith.git.dto.CommitDTO;
import com.appsmith.server.artifacts.base.ArtifactService;
import com.appsmith.server.constants.ArtifactType;
import com.appsmith.server.constants.FieldName;
import com.appsmith.server.constants.Url;
import com.appsmith.server.domains.Artifact;
import com.appsmith.server.domains.GitArtifactMetadata;
import com.appsmith.server.domains.GitAuth;
import com.appsmith.server.dtos.AutoCommitResponseDTO;
import com.appsmith.server.dtos.BranchProtectionRequestDTO;
import com.appsmith.server.dtos.GitAuthDTO;
import com.appsmith.server.dtos.GitConnectDTO;
import com.appsmith.server.dtos.GitMergeDTO;
import com.appsmith.server.dtos.GitPullDTO;
import com.appsmith.server.dtos.ResponseDTO;
import com.appsmith.server.git.autocommit.AutoCommitService;
import com.appsmith.server.git.central.CentralGitService;
import com.appsmith.server.git.central.GitType;
import com.appsmith.server.git.utils.GitProfileUtils;
import com.fasterxml.jackson.annotation.JsonView;
import jakarta.validation.Valid;
import lombok.RequiredArgsConstructor;
Expand Down Expand Up @@ -48,8 +50,8 @@
public class GitApplicationControllerCE {

protected final CentralGitService centralGitService;
protected final GitProfileUtils gitProfileUtils;
protected final AutoCommitService autoCommitService;
protected final ArtifactService artifactService;

protected static final ArtifactType ARTIFACT_TYPE = ArtifactType.APPLICATION;
protected static final GitType GIT_TYPE = GitType.FILE_SYSTEM;
Expand Down Expand Up @@ -96,7 +98,7 @@ public Mono<ResponseDTO<? extends Artifact>> createReference(
referencedApplicationId,
srcBranch);
return centralGitService
.createReference(referencedApplicationId, ArtifactType.APPLICATION, gitRefDTO, GIT_TYPE)
.createReference(referencedApplicationId, ARTIFACT_TYPE, gitRefDTO, GIT_TYPE)
.map(result -> new ResponseDTO<>(HttpStatus.CREATED.value(), result, null));
}

Expand Down Expand Up @@ -254,4 +256,21 @@ public Mono<ResponseDTO<List<GitRefDTO>>> getReferences(
branchedApplicationId, ARTIFACT_TYPE, BooleanUtils.isTrue(pruneBranches), GIT_TYPE)
.map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null));
}

@JsonView(Views.Public.class)
@GetMapping("/{branchedApplicationId}/ssh-keypair")
public Mono<ResponseDTO<GitAuthDTO>> getSSHKey(@PathVariable String branchedApplicationId) {
return artifactService
.getSshKey(ARTIFACT_TYPE, branchedApplicationId)
.map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null));
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
.map(created -> new ResponseDTO<>(HttpStatus.CREATED.value(), created, null));
.map(result -> new ResponseDTO<>(HttpStatus.OK.value(), result, null));

}
Comment on lines +262 to +266
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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));
}


@JsonView(Views.Public.class)
@PostMapping("/{branchedApplicationId}/ssh-keypair")
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));
}
Comment on lines +270 to +275
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 6, 2025

Choose a reason for hiding this comment

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

💡 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 on GitDeployKeyGenerator.getSupportedProtocols().
  • Reject or gracefully handle any unsupported key types early in the request lifecycle.

}
Loading