feat: resource clone & delete with authz hardening#301
Open
ptone wants to merge 5 commits into
Open
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the Resource Clone and Delete features, enabling users to clone and delete templates and harness configurations across scopes, and adds robust authorization checks to these endpoints. Database migrations were also introduced to remove the unused Locked column and enforce unique slug indexes. The review feedback highlights several critical improvement opportunities: cleaning up copied storage files if database creation fails during cloning to prevent orphaned files, handling local paths on Windows more robustly in name derivation, using idiomatic defer f.Close() inside file upload goroutines, and fixing a copy-paste label bug and redundant cloning method in the frontend resource list component.
Comment on lines
+684
to
+691
| if err := s.store.CreateHarnessConfig(ctx, clone); err != nil { | ||
| if strings.Contains(err.Error(), "UNIQUE constraint failed") { | ||
| writeError(w, http.StatusConflict, "conflict", "A resource with this slug already exists in the target scope. Choose a different name.", nil) | ||
| return | ||
| } | ||
| writeErrorFromErr(w, err, "") | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
If CreateHarnessConfig fails (e.g., due to a unique constraint violation or database error), the copied files are left in storage, resulting in orphaned files. We should clean up the copied files in storage using stor.DeletePrefix on failure.
if err := s.store.CreateHarnessConfig(ctx, clone); err != nil {
if stor != nil {
_ = stor.DeletePrefix(ctx, storagePath)
}
if strings.Contains(err.Error(), "UNIQUE constraint failed") {
writeError(w, http.StatusConflict, "conflict", "A resource with this slug already exists in the target scope. Choose a different name.", nil)
return
}
writeErrorFromErr(w, err, "")
return
}
Comment on lines
830
to
837
| if err := s.store.CreateTemplate(ctx, clone); err != nil { | ||
| if strings.Contains(err.Error(), "UNIQUE constraint failed") { | ||
| writeError(w, http.StatusConflict, "conflict", "A resource with this slug already exists in the target scope. Choose a different name.", nil) | ||
| return | ||
| } | ||
| writeErrorFromErr(w, err, "") | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
If CreateTemplate fails, the copied files are left in storage, resulting in orphaned files. We should clean up the copied files in storage using stor.DeletePrefix on failure.
if err := s.store.CreateTemplate(ctx, clone); err != nil {
if stor != nil {
_ = stor.DeletePrefix(ctx, storagePath)
}
if strings.Contains(err.Error(), "UNIQUE constraint failed") {
writeError(w, http.StatusConflict, "conflict", "A resource with this slug already exists in the target scope. Choose a different name.", nil)
return
}
writeErrorFromErr(w, err, "")
return
}
Comment on lines
+37
to
+69
| // rather than the cache-hash directory name. | ||
| func DeriveResourceName(source string) string { | ||
| source = strings.TrimSpace(source) | ||
| if source == "" { | ||
| return "" | ||
| } | ||
|
|
||
| if strings.HasPrefix(source, "file://") { | ||
| localPath := strings.TrimPrefix(source, "file://") | ||
| return sanitizeDerivedName(filepath.Base(filepath.Clean(localPath))) | ||
| } | ||
|
|
||
| // rclone connection string, e.g. ":gcs:bucket/path". | ||
| if strings.HasPrefix(source, ":") { | ||
| parts := strings.SplitN(source, ":", 3) | ||
| if len(parts) == 3 { | ||
| return sanitizeDerivedName(path.Base(strings.TrimRight(parts[2], "/"))) | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| normalized := normalizeResourceSourceURL(source) | ||
| u, err := url.Parse(normalized) | ||
| if err != nil { | ||
| return sanitizeDerivedName(filepath.Base(filepath.Clean(source))) | ||
| } | ||
|
|
||
| cleanPath := strings.TrimRight(u.Path, "/") | ||
| if cleanPath == "" { | ||
| return sanitizeDerivedName(filepath.Base(filepath.Clean(source))) | ||
| } | ||
| return sanitizeDerivedName(path.Base(cleanPath)) | ||
| } |
Contributor
There was a problem hiding this comment.
For local paths (especially on Windows with backslashes), parsing with url.Parse and then using path.Base can fail to derive the correct name. Since IsRemoteURI is already available in the config package, we can check if the source is a remote URI first. If it is a local path, we can directly clean and extract the base name using filepath.Base and filepath.Clean.
func DeriveResourceName(source string) string {
source = strings.TrimSpace(source)
if source == "" {
return ""
}
if !IsRemoteURI(source) {
if strings.HasPrefix(source, "file://") {
source = strings.TrimPrefix(source, "file://")
}
return sanitizeDerivedName(filepath.Base(filepath.Clean(source)))
}
// rclone connection string, e.g. ":gcs:bucket/path".
if strings.HasPrefix(source, ":") {
parts := strings.SplitN(source, ":", 3)
if len(parts) == 3 {
return sanitizeDerivedName(path.Base(strings.TrimRight(parts[2], "/")))
}
return ""
}
normalized := normalizeResourceSourceURL(source)
u, err := url.Parse(normalized)
if err != nil {
return sanitizeDerivedName(filepath.Base(filepath.Clean(source)))
}
cleanPath := strings.TrimRight(u.Path, "/")
if cleanPath == "" {
return sanitizeDerivedName(filepath.Base(filepath.Clean(source)))
}
return sanitizeDerivedName(path.Base(cleanPath))
}
Comment on lines
+165
to
+174
| f, err := os.Open(fi.FullPath) | ||
| if err != nil { | ||
| return fmt.Errorf("%s: failed to open file %s: %w", label, fi.Path, err) | ||
| } | ||
|
|
||
| _, err = stor.Upload(ctx, objectPath, f, storage.UploadOptions{}) | ||
| _ = f.Close() | ||
| if err != nil { | ||
| return fmt.Errorf("%s: failed to upload file %s: %w", label, fi.Path, err) | ||
| } |
Contributor
There was a problem hiding this comment.
Using defer f.Close() right after successfully opening the file is more idiomatic in Go and ensures the file descriptor is closed robustly even if a panic occurs or if early returns are added in the future.
Suggested change
| f, err := os.Open(fi.FullPath) | |
| if err != nil { | |
| return fmt.Errorf("%s: failed to open file %s: %w", label, fi.Path, err) | |
| } | |
| _, err = stor.Upload(ctx, objectPath, f, storage.UploadOptions{}) | |
| _ = f.Close() | |
| if err != nil { | |
| return fmt.Errorf("%s: failed to upload file %s: %w", label, fi.Path, err) | |
| } | |
| f, err := os.Open(fi.FullPath) | |
| if err != nil { | |
| return fmt.Errorf("%s: failed to open file %s: %w", label, fi.Path, err) | |
| } | |
| defer f.Close() | |
| _, err = stor.Upload(ctx, objectPath, f, storage.UploadOptions{}) | |
| if err != nil { | |
| return fmt.Errorf("%s: failed to upload file %s: %w", label, fi.Path, err) | |
| } |
Comment on lines
+655
to
+703
| private renderCloneDialog() { | ||
| if (!this.cloneTarget) return nothing; | ||
|
|
||
| const isFromGlobal = | ||
| this.cloneFromGlobal && this.scope === 'project' && !this.items.find((i) => i.id === this.cloneTarget!.id); | ||
|
|
||
| return html` | ||
| <sl-dialog | ||
| label="Clone ${this.kindLabel}" | ||
| open | ||
| @sl-request-close=${(e: Event) => { | ||
| if (this.actionInProgress) e.preventDefault(); | ||
| else this.closeCloneDialog(); | ||
| }} | ||
| > | ||
| <p> | ||
| Clone <strong>${this.cloneTarget.displayName || this.cloneTarget.name}</strong> | ||
| ${isFromGlobal ? html` from global into this project` : nothing}. | ||
| </p> | ||
| <sl-input | ||
| label="New name" | ||
| .value=${this.cloneName} | ||
| @sl-input=${(e: Event) => { | ||
| this.cloneName = (e.target as HTMLInputElement).value; | ||
| }} | ||
| ></sl-input> | ||
| ${this.actionError ? html`<div class="dialog-error">${this.actionError}</div>` : nothing} | ||
| <div slot="footer"> | ||
| <sl-button | ||
| variant="default" | ||
| size="small" | ||
| ?disabled=${this.actionInProgress} | ||
| @click=${() => this.closeCloneDialog()} | ||
| > | ||
| Cancel | ||
| </sl-button> | ||
| <sl-button | ||
| variant="primary" | ||
| size="small" | ||
| ?loading=${this.actionInProgress} | ||
| ?disabled=${this.actionInProgress || !this.cloneName.trim()} | ||
| @click=${() => (isFromGlobal ? this.confirmCloneFromGlobal() : this.confirmClone())} | ||
| > | ||
| Clone | ||
| </sl-button> | ||
| </div> | ||
| </sl-dialog> | ||
| `; | ||
| } |
Contributor
There was a problem hiding this comment.
There is a copy-paste bug in the dialog label: it is set to "Delete ${this.kindLabel}" instead of "Clone ${this.kindLabel}". Additionally, confirmCloneFromGlobal is redundant because confirmClone already uses this.scope and this.scopeId (which are set to 'project' and the project ID on the list component). We can simplify the code by removing confirmCloneFromGlobal and using confirmClone directly.
private renderCloneDialog() {
if (!this.cloneTarget) return nothing;
const isFromGlobal =
this.cloneFromGlobal && this.scope === 'project' && !this.items.find((i) => i.id === this.cloneTarget!.id);
return html`
<sl-dialog
label="Clone ${this.kindLabel}"
open
@sl-request-close=${(e: Event) => {
if (this.actionInProgress) e.preventDefault();
else this.closeCloneDialog();
}}
>
<p>
Clone <strong>${this.cloneTarget.displayName || this.cloneTarget.name}</strong>
${isFromGlobal ? html` from global into this project` : nothing}.
</p>
<sl-input
label="New name"
.value=${this.cloneName}
@sl-input=${(e: Event) => {
this.cloneName = (e.target as HTMLInputElement).value;
}}
></sl-input>
${this.actionError ? html`<div class="dialog-error">${this.actionError}</div>` : nothing}
<div slot="footer">
<sl-button
variant="default"
size="small"
?disabled=${this.actionInProgress}
@click=${() => this.closeCloneDialog()}
>
Cancel
</sl-button>
<sl-button
variant="primary"
size="small"
?loading=${this.actionInProgress}
?disabled=${this.actionInProgress || !this.cloneName.trim()}
@click=${() => this.confirmClone()}
>
Clone
</sl-button>
</div>
</sl-dialog>
`;
}
Comment on lines
+454
to
+493
| private async confirmCloneFromGlobal(): Promise<void> { | ||
| if (!this.cloneTarget) return; | ||
| this.actionInProgress = true; | ||
| this.actionError = ''; | ||
| try { | ||
| const body: Record<string, string> = { | ||
| name: this.cloneName, | ||
| scope: 'project', | ||
| scopeId: this.scopeId, | ||
| }; | ||
|
|
||
| const response = await apiFetch( | ||
| `/api/v1/${this.apiResource}/${this.cloneTarget.id}/clone`, | ||
| { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(body), | ||
| } | ||
| ); | ||
| if (response.status === 409) { | ||
| this.actionError = 'A resource with this slug already exists. Choose a different name.'; | ||
| this.actionInProgress = false; | ||
| return; | ||
| } | ||
| if (!response.ok) { | ||
| throw new Error( | ||
| await extractApiError(response, `Failed to clone: HTTP ${response.status}`) | ||
| ); | ||
| } | ||
| const created = (await response.json()) as { id?: string }; | ||
| const sourceId = this.cloneTarget.id; | ||
| this.closeCloneDialog(); | ||
| await this.load(); | ||
| this.emitChanged('cloned', created.id ?? '', sourceId); | ||
| } catch (err) { | ||
| this.actionError = err instanceof Error ? err.message : 'Clone failed'; | ||
| } finally { | ||
| this.actionInProgress = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This method is redundant and can be removed because confirmClone already handles cloning into the current project scope.
…dels Remove the Locked bool field, all 16 enforcement sites across 6 handler files, the force query parameter from delete endpoints, 3 locked-template tests, and add a DB migration to drop the column. No production code ever set Locked=true — this simplifies the handlers for the upcoming clone/delete feature.
…iqueness - Add handleHarnessConfigClone mirroring template clone - Add CheckAccess authz to deleteTemplateV2, handleTemplateClone, deleteHarnessConfig, handleHarnessConfigClone - Add DB migration V55: UNIQUE constraint on (slug, scope, scope_id) - Return 409 Conflict on slug collision during clone - Add clone failure cleanup - Add tests for clone, authz, and slug collision
…urce list - Add Clone and Delete action menu to shared resource-list component - Add delete confirmation dialog with deleteFiles checkbox (default on) - Add clone dialog with name input and 409 collision handling - Add clone-from-global picker in project settings view - Unify on resource-changed event (migrate resource-imported) - Gate actions on capabilities (canClone, canDelete properties)
… remove redundant clone method - Add stor.DeletePrefix cleanup when CreateTemplate/CreateHarnessConfig fails after files were already copied (prevents orphaned storage files) - Remove redundant confirmCloneFromGlobal method — confirmClone already handles cross-scope clone via the component's scope/scopeId properties
scion-gteam
Bot
force-pushed
the
scion/template-harness-refactor
branch
from
ad9761a to
a5064db
Compare
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.
Summary
Implements the Resource Clone & Delete feature from the design doc, adding Clone and Delete management actions to the shared resource list component used by both Project Settings and Hub Resources views.
Closes issue 115
Phase 0 — Remove dead
LockedfieldLocked boolfrom Template and HarnessConfig models, all 16 enforcement sites across 6 handler files, theforcequery parameter from delete endpoints, and 3 locked-template testsPhase 1 — Backend: harness-config clone, authz, slug uniqueness
handleHarnessConfigClone(POST /api/v1/harness-configs/{id}/clone) mirroring the template clone endpointCheckAccessauthz todeleteTemplateV2,handleTemplateClone,deleteHarnessConfig, andhandleHarnessConfigClone— global scope requires hub-admin, project scope checks project capabilityUNIQUEconstraint on(slug, scope, scope_id)for both tables; clone returns 409 Conflict on slug collisionPhase 2 — Frontend: Clone/Delete row actions, clone-from-global, unified event
<scion-resource-list>{name}-copy, 409 collision error surfacingresource-changedevent withdetail.actiondiscriminator (imported | cloned | deleted); migrate import componentcanClone,canDelete,cloneFromGlobalproperties wired from host pagesTest plan
go build ./...passesgo test ./pkg/hub/... ./pkg/store/...— all tests pass including 6 new clone/delete/authz teststsc --noEmit)