chore: Moved git auth creation and updation to generified artifactservice #38312
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| package com.appsmith.server.artifacts.base; | ||
|
|
||
| public interface ArtifactService extends ArtifactServiceCE {} |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| package com.appsmith.server.artifacts.base; | ||
|
|
||
| import com.appsmith.server.artifacts.base.artifactbased.ArtifactBasedService; | ||
| import com.appsmith.server.constants.ArtifactType; | ||
| import com.appsmith.server.domains.Artifact; | ||
| import com.appsmith.server.domains.GitAuth; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| public interface ArtifactServiceCE { | ||
|
|
||
| /** | ||
| * This method returns the appropriate ArtifactBasedService based on the type of artifact. | ||
| */ | ||
| ArtifactBasedService<? extends Artifact> getArtifactBasedService(ArtifactType artifactType); | ||
|
|
||
| /** | ||
| * Generate SSH private and public keys required to communicate with remote. Keys will be stored only in the | ||
| * default/root application only and not the child branched application. This decision is taken because the combined | ||
| * size of keys is close to 4kB | ||
| * | ||
| * @return public key which will be used by user to copy to relevant platform | ||
| */ | ||
| Mono<GitAuth> createOrUpdateSshKeyPair(ArtifactType artifactType, String branchedArtifactId, String keyType); | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| package com.appsmith.server.artifacts.base; | ||
|
|
||
| import com.appsmith.external.constants.AnalyticsEvents; | ||
| 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.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.exceptions.AppsmithError; | ||
| import com.appsmith.server.exceptions.AppsmithException; | ||
| import com.appsmith.server.helpers.GitDeployKeyGenerator; | ||
| import com.appsmith.server.services.AnalyticsService; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.util.StringUtils; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| import java.util.Map; | ||
|
|
||
| @Slf4j | ||
| @Service | ||
| public class ArtifactServiceCEImpl implements ArtifactServiceCE { | ||
|
|
||
| protected final ArtifactBasedService<Application> applicationService; | ||
| private final AnalyticsService analyticsService; | ||
|
|
||
| public ArtifactServiceCEImpl( | ||
| ArtifactBasedService<Application> applicationService, AnalyticsService analyticsService) { | ||
| this.applicationService = applicationService; | ||
| this.analyticsService = analyticsService; | ||
| } | ||
|
|
||
| @Override | ||
| public ArtifactBasedService<? extends Artifact> getArtifactBasedService(ArtifactType artifactType) { | ||
| return applicationService; | ||
| } | ||
|
|
||
| @Override | ||
| public Mono<GitAuth> createOrUpdateSshKeyPair( | ||
| ArtifactType artifactType, String branchedArtifactId, String keyType) { | ||
| GitAuth gitAuth = GitDeployKeyGenerator.generateSSHKey(keyType); | ||
| ArtifactBasedService<? extends Artifact> artifactBasedService = getArtifactBasedService(artifactType); | ||
| ArtifactPermission artifactPermission = artifactBasedService.getPermissionService(); | ||
| final String artifactTypeName = artifactType.name().toLowerCase(); | ||
| return artifactBasedService | ||
| .findById(branchedArtifactId, artifactPermission.getEditPermission()) | ||
| .switchIfEmpty(Mono.error( | ||
| new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, artifactTypeName, branchedArtifactId))) | ||
| .flatMap(artifact -> { | ||
| GitArtifactMetadata gitData = artifact.getGitArtifactMetadata(); | ||
| // Check if the current artifact is the root artifact | ||
|
|
||
| if (gitData != null | ||
| && StringUtils.hasLength(gitData.getDefaultArtifactId()) | ||
| && branchedArtifactId.equals(gitData.getDefaultArtifactId())) { | ||
| // This is the root application with update SSH key request | ||
| gitAuth.setRegeneratedKey(true); | ||
| gitData.setGitAuth(gitAuth); | ||
| return artifactBasedService.save(artifact); | ||
| } else if (gitData == null) { | ||
| // This is a root application with generate SSH key request | ||
| GitArtifactMetadata gitArtifactMetadata = new GitArtifactMetadata(); | ||
| gitArtifactMetadata.setDefaultApplicationId(branchedArtifactId); | ||
| gitArtifactMetadata.setGitAuth(gitAuth); | ||
| artifact.setGitArtifactMetadata(gitArtifactMetadata); | ||
| return artifactBasedService.save(artifact); | ||
| } | ||
| // Children application with update SSH key request for root application | ||
| // Fetch root application and then make updates. We are storing the git metadata only in root | ||
| // application | ||
| if (!StringUtils.hasLength(gitData.getDefaultArtifactId())) { | ||
| return Mono.error(new AppsmithException( | ||
| AppsmithError.INVALID_GIT_CONFIGURATION, | ||
| "Unable to find root " + artifactTypeName + ", please connect your " + artifactTypeName | ||
| + " to remote repo to resolve this issue.")); | ||
| } | ||
| gitAuth.setRegeneratedKey(true); | ||
|
|
||
| return artifactBasedService | ||
| .findById(gitData.getDefaultArtifactId(), artifactPermission.getEditPermission()) | ||
| .flatMap(baseApplication -> { | ||
| GitArtifactMetadata gitArtifactMetadata = baseApplication.getGitArtifactMetadata(); | ||
| gitArtifactMetadata.setDefaultApplicationId(baseApplication.getId()); | ||
| gitArtifactMetadata.setGitAuth(gitAuth); | ||
| baseApplication.setGitArtifactMetadata(gitArtifactMetadata); | ||
| return artifactBasedService.save(baseApplication); | ||
| }); | ||
| }) | ||
|
Comment on lines
+73
to
+92
Contributor
There was a problem hiding this comment. 💡 Codebase verification Based on the code analysis, I can now provide a final response: Add optimistic locking to prevent concurrent SSH key updates Example locations that need attention:
🔗 Analysis chainFlag potential concurrency scenario Let me search for relevant code to verify the concurrency concerns. Let me also check for any transaction or version control mechanisms in the artifact service: This will help us understand if there are any existing concurrency controls and the full context of the SSH key generation flow. Let me try a different search pattern to find the relevant code: Let me check the underlying service implementation to understand if there are any concurrency controls: Let me try one more search to find the Application entity and service implementation: 🏁 Scripts executedThe following scripts were executed for the analysis: Error: No shell scripts found Script: #!/bin/bash
# Search for the class and method definition to understand the context
ast-grep --pattern 'class $_ {
$$$
generateSSHKeyPair($$$) {
$$$
}
$$$
}'
# Search for any existing concurrency control mechanisms
rg -i "lock|synchronized|atomic|concurrent" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/
Length of output: 205 Script: # Search for transaction annotations or version fields
rg -i "@transaction|@version|optimisticlock" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/
# Check the entity/model class for version fields
ast-grep --pattern 'class $_ {
$$$
@Version
private $_ version;
$$$
}'
Length of output: 199 Script: #!/bin/bash
# Search for the artifact service implementation
rg -l "class.*ArtifactService.*Impl" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/ | xargs cat
# Search for the entity class that might contain version field
fd "Artifact.*\.java" app/server/appsmith-server/src/main/java/com/appsmith/server/ -x cat {}
# Look for any save operations in the service
rg -A 5 "save\(" app/server/appsmith-server/src/main/java/com/appsmith/server/artifacts/
Length of output: 65961 Script: # Search for the ArtifactBasedService implementation
fd "ArtifactBasedService.*Impl\.java" app/server/appsmith-server/src/main/java/ -x cat {}
# Search for any MongoDB optimistic locking annotations
rg -i "@version|@optimisticlock" app/server/appsmith-server/src/main/java/com/appsmith/server/domains/
Length of output: 198 Script: # Search for Application entity class
rg -l "class Application" app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ | xargs cat
# Search for ApplicationService implementation
fd "ApplicationService.*Impl\.java" app/server/appsmith-server/src/main/java/ -x cat {}
# Look for any transaction or locking mechanisms in the service layer
rg -i "@transactional|@lock|synchronized" app/server/appsmith-server/src/main/java/com/appsmith/server/services/
Length of output: 77806 |
||
| .flatMap(artifact -> { | ||
| // Send generate SSH key analytics event | ||
| assert artifact.getId() != null; | ||
| final Map<String, Object> eventData = Map.of( | ||
| FieldName.APP_MODE, ApplicationMode.EDIT.toString(), FieldName.APPLICATION, artifact); | ||
| final Map<String, Object> data = Map.of( | ||
| FieldName.APPLICATION_ID, | ||
| artifact.getId(), | ||
| "organizationId", | ||
| artifact.getWorkspaceId(), | ||
| "isRegeneratedKey", | ||
| gitAuth.isRegeneratedKey(), | ||
| FieldName.EVENT_DATA, | ||
| eventData); | ||
| 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); | ||
| }); | ||
| }) | ||
| .thenReturn(gitAuth); | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package com.appsmith.server.artifacts.base; | ||
|
|
||
| import com.appsmith.server.artifacts.base.artifactbased.ArtifactBasedService; | ||
| import com.appsmith.server.domains.Application; | ||
| import com.appsmith.server.services.AnalyticsService; | ||
| import org.springframework.stereotype.Service; | ||
|
|
||
| @Service | ||
| public class ArtifactServiceImpl extends ArtifactServiceCEImpl implements ArtifactService { | ||
|
|
||
| public ArtifactServiceImpl( | ||
| ArtifactBasedService<Application> applicationService, AnalyticsService analyticsService) { | ||
| super(applicationService, analyticsService); | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package com.appsmith.server.artifacts.base.artifactbased; | ||
|
|
||
| import com.appsmith.server.domains.Artifact; | ||
|
|
||
| public interface ArtifactBasedService<T extends Artifact> extends ArtifactBasedServiceCE<T> {} |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package com.appsmith.server.artifacts.base.artifactbased; | ||
|
|
||
| import com.appsmith.server.acl.AclPermission; | ||
| import com.appsmith.server.artifacts.permissions.ArtifactPermission; | ||
| import com.appsmith.server.domains.Artifact; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| public interface ArtifactBasedServiceCE<T extends Artifact> { | ||
|
|
||
| Mono<T> findById(String id, AclPermission aclPermission); | ||
|
|
||
| Mono<T> save(Artifact artifact); | ||
|
|
||
| ArtifactPermission getPermissionService(); | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| package com.appsmith.server.artifacts.permissions; | ||
|
|
||
| public interface ArtifactPermission extends ArtifactPermissionCE {} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
ACL_NO_RESOURCE_FOUNDis a preferred oneEdit: It's fine to leave as is, since it's not aligned with the intent of the PR