feat: replace hardcoded jobs with configurable actions#2351
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In several places (e.g. building
actionItemsinDatasetTableActionsComponentandBatchViewComponent) you rely onas ActionItemDataset[]/as UserProfile; consider tightening the selector types (or introducing a typed facade) so you can avoid these casts and catch mismatches at compile time. - The
{ success: boolean }payload foractionFinishedis duplicated across multiple components; introducing a shared interface/type (and possibly centralizing the success-handling logic, e.g. driven offactionSuccessActionin an effect) would reduce duplication and make it easier to evolve the contract later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In several places (e.g. building `actionItems` in `DatasetTableActionsComponent` and `BatchViewComponent`) you rely on `as ActionItemDataset[]` / `as UserProfile`; consider tightening the selector types (or introducing a typed facade) so you can avoid these casts and catch mismatches at compile time.
- The `{ success: boolean }` payload for `actionFinished` is duplicated across multiple components; introducing a shared interface/type (and possibly centralizing the success-handling logic, e.g. driven off `actionSuccessAction` in an effect) would reduce duplication and make it easier to evolve the contract later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
after this PR I believe another enhancement of configurable-actions would be to allow for shared blocks, in particular the variables one. This way the config can stay slimmer since often variables in multiple actions inside the same config key are shared |
minottic
force-pushed
the
generalize_dialog
branch
4 times, most recently
from
f9081a9 to
3ab9cdf
Compare
This applies both to cart and dataset actions
There was a problem hiding this comment.
Pull request overview
This PR replaces the hardcoded archive/retrieve “jobs” UI with config-driven “batch actions” (via batchActionsEnabled/batchActions) and routes action success/failure through a centralized NgRx effect that shows standardized snackbars.
Changes:
- Added new NgRx actions + effect to translate generic action success/failure into
showMessageActionsnackbars. - Integrated
<configurable-actions>into the datasets overview and batch view, removing the dedicatedArchivingServiceflow and clearing selection on mode change / successful action. - Extended configurable-actions keyword support and updated technical + user documentation.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/state-management/effects/actions.effect.ts | New effect mapping action success/failure to snackbar messages |
| src/app/state-management/effects/actions.effect.spec.ts | Unit tests for the new effect behavior |
| src/app/state-management/actions/actions.actions.ts | New generic action success/failure action creators |
| src/app/state-management/actions/actions.actions.spec.ts | Unit tests for new action creators |
| src/app/shared/modules/configurable-actions/doc/configurable-actions-technical.md | Documents new dataset-related selector keywords |
| src/app/shared/modules/configurable-actions/configurable-actions.documentation.md | User-facing docs updated with new selector keywords |
| src/app/shared/modules/configurable-actions/configurable-action.interfaces.ts | Switch dataset action item type to OutputDatasetObsoleteDto + files? |
| src/app/shared/modules/configurable-actions/configurable-action.component.ts | Dispatch generic success/failure actions instead of using MatSnackBar; add new dataset keyword resolvers |
| src/app/shared/modules/configurable-actions/configurable-action.component.spec.ts | Adds tests for new keywords and xhr failure dispatch |
| src/app/datasets/datasets.module.ts | Removes ArchivingService and registers new ActionEffects |
| src/app/datasets/dataset-table/dataset-table.component.ts | Sync table selection clearing with store-selected datasets |
| src/app/datasets/dataset-table/dataset-table.component.spec.ts | Adds selector mocking for selected-datasets store |
| src/app/datasets/dataset-table/dataset-table.component.html | Adds template ref to drive clearSelection() via ViewChild |
| src/app/datasets/dataset-table-actions/dataset-table-actions.component.ts | Replaces archive/retrieve buttons with configurable actions; clears selection on mode changes and after successful actions |
| src/app/datasets/dataset-table-actions/dataset-table-actions.component.spec.ts | Updates tests for new mode-change behavior and action-finished selection clearing |
| src/app/datasets/dataset-table-actions/dataset-table-actions.component.html | Renders configurable actions and hides them when batch/cart is non-empty |
| src/app/datasets/dashboard/dashboard.component.ts | Removes selectedSets plumbing now that selection comes from the store |
| src/app/datasets/dashboard/dashboard.component.html | Drops selectedSets inputs to actions/table components |
| src/app/datasets/batch-view/batch-view.component.ts | Replaces archive/retrieve flow with configurable actions + actionItems wiring |
| src/app/datasets/batch-view/batch-view.component.spec.ts | Removes ArchivingService mocking |
| src/app/datasets/batch-view/batch-view.component.html | Uses configurable actions instead of archive/retrieve buttons |
| src/app/datasets/archiving.service.ts | Removes the legacy archiving service |
| src/app/datasets/archiving.service.spec.ts | Removes tests for the deleted service |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review Report: config_batch BranchThis report analyzes the changes introduced by the AssessmentOverviewThe Key Changes1. Archiving Service Removal (Major Improvement)
2. Configurable Actions Integration (Core Feature)
3. State Management for Actions (Best Practice)
4. Error Handling Improvements (Enhanced UX)
5. Selection Management (Cleaner Architecture)
6. Documentation Updates (Comprehensive)
Lines Changed Summary
Improvement Needed1. Error Handling Consistency (Medium Priority)Issue: The new action effects use hardcoded default messages ("Success!" and "An error occurred.") Current Implementation (actions.effect.ts): message: new Message(
action.message || "Success!",
MessageType.Success,
5000,
)Recommendation:
2. Action Type Safety (Medium Priority)Issue: The Current Implementation (configurable-action.interfaces.ts): export type ActionType = "form" | "link" | "json-download" | "xhr" | "dialog";Recommendation:
3. Testing Coverage (High Priority)Issue: While tests were added, some edge cases may not be covered. Missing Test Scenarios:
Recommendation:
4. Performance Optimization (Low Priority)Issue: The Current Implementation: combineLatest([
this.userProfile$,
this.store.select(selectArchiveViewMode),
this.selectSelectedDatasets$,
]).subscribe(...)Recommendation:
5. Backward Compatibility (Critical for Merge)Issue: The changes assume Current Implementation (batch-view.component.html): <configurable-actions *ngIf="appConfig.batchActionsEnabled"
[actionsConfig]="appConfig.batchActions"
...
></configurable-actions>Recommendation:
6. Documentation Gaps (Medium Priority)Missing Documentation:
Recommendation:
7. Type Narrowing (Low Priority)Issue: Some type assertions may use Recommendation:
8. Accessibility (Medium Priority)Issue: Configurable actions generate buttons dynamically, but accessibility attributes may not be consistent. Recommendation:
VerdictOverall Rating: 8.5/10 - Excellent with Minor Improvements NeededStrengths:
Weaknesses:
Recommendation: MERGE WITH MINOR CHANGESThis branch represents a significant improvement to the codebase and should be merged after addressing the following:
Estimated Effort for Improvements:
Total estimated effort: 1-2 days of work Final ThoughtsThis is an excellent refactoring that makes the application more flexible and maintainable. The configurable actions framework will allow future features to be added without modifying core application code, which is a significant architectural win. The changes are well-executed and follow best practices. With the minor improvements suggested above, this will be a production-ready feature that greatly enhances the SciCat frontend's extensibility. Generated with Mistral AI |
Code Review: config_batch Branch - Concise Technical AssessmentSummary of ChangesThe Files Changed: 23 files | Net: -181 lines (368 added, 549 removed) What the Code DoesRemoved Code (archiving.service.ts)
Modified Components1.
|
| Edge Case | Status | Location | Notes |
|---|---|---|---|
| Empty datasets array | ✅ Handled | configurable-action.component.ts:213 |
Returns [] via lodash |
| Null/undefined dataset fields | ✅ Handled | Multiple selectors | Uses ` |
| Non-JSON API response | typeXhr():371 |
Treated as success with empty data | |
| HTTP error status | ✅ Handled | typeXhr():370 |
Throws error with status code |
| Network failure | ✅ Handled | typeXhr():376-379 |
Caught and dispatched as failure |
| Circular variable dependencies | ✅ Handled | buildDependenciesGraph():298 |
Detects and logs error |
| No datasets selected | ✅ Handled | dataset-table-actions.component.ts:75 |
isEmptySelection() prevents actions |
| Missing config | ❌ Not handled | batch-view.component.html:532 |
No fallback if batchActions missing |
| Action type not supported | ❌ Not handled | configurable-action.component.ts |
No validation of action type |
Does It Make Sense?
✅ Yes - Strong Architecture Decision
Rationale:
- DRY Principle: Eliminates duplicate archive/retrieve logic
- Extensibility: New batch actions can be configured without code changes
- Consistency: All actions now use same framework and error handling
- Maintainability: Reduces code surface area by ~181 lines
- Separation of Concerns: Configuration (JSON) separate from implementation (TypeScript)
The changes are coherent and follow best practices:
- NgRx patterns for state management
- TypeScript interfaces for type safety
- Lodash for data manipulation
- Centralized error handling
- Store selectors over component inputs
Unreachable Code
None found in the current codebase after changes.
The only previously unreachable code was in batch-view.component.ts where queryParams["retrieve"] check called onRetrieve(), which was removed along with the method itself. All remaining code paths are reachable and tested.
Verdict
| Aspect | Rating | Notes |
|---|---|---|
| Logic Correctness | ✅ 9/10 | Sound implementation, one minor issue with non-JSON responses |
| Edge Cases | Most handled, missing config validation and fallback | |
| Code Sense | ✅ 10/10 | Excellent refactoring, follows best practices |
| Unreachable Code | ✅ 10/10 | None found |
| Overall | ✅ 8.5/10 | Approve with minor improvements |
Recommendation: MERGE - The changes are logically correct, make architectural sense, and handle most edge cases. The minor issues (config validation, non-JSON response handling) should be addressed but are not blockers.
Critical Actions Before Merge: None - all critical paths are handled correctly.
Recommended Improvements:
- Add fallback when
batchActionsconfig is missing - Handle non-JSON responses in XHR actions (don't treat as success)
- Remove redundant subscription in dataset-table-actions
- Add runtime validation for action types
Generated with Mistral AI
Security Review: config_batch BranchThis document provides a security-focused analysis of the code changes introduced by the 1. Injection VulnerabilitiesAssessment: One NEW potential vulnerability introduced
|
| Type | Status | Location | Risk | Notes |
|---|---|---|---|---|
Code Injection via Function constructor |
configurable-action.component.ts:284 |
High | Uses new Function() with interpolated expressions |
|
| Template Injection | ✅ Safe | interpolate(), resolve() methods |
Low | Proper escaping via String() conversion |
| XSS via URL | ✅ Safe | typeXhr(), typeForm() |
Low | URLs from config, not user input |
| Header Injection | ✅ Safe | typeXhr():360-362 |
Low | Headers from config with String() conversion |
Details
Critical: Code Injection via Function Constructor
Location: src/app/shared/modules/configurable-actions/configurable-action.component.ts:284
private evaluate(expr: string): boolean {
try {
const context = { variables: this.variables, context: { ... } };
const fn = new Function("ctx", `with(ctx){ return ${expr}; }`);
return fn(context);
} catch (e) {
console.error("Evaluation error:", expr, e);
return false;
}
}Risk Analysis:
- Vector: The
exprparameter comes from the action configuration'senabled,disabled,hidden, orauthorizationfields - Source: Configuration is loaded from admin-controlled JSON files (not user input)
- Impact: If an administrator sets a malicious expression, it executes with full application privileges
- Mitigation: Configuration files are admin-controlled, not user-controlled
Example Attack:
{
"enabled": "variables.someVar; maliciousCode(); true"
}Verdict: HIGH RISK but MITIGATED by admin-only config access. This is a new attack surface that didn't exist in the hardcoded archiving service.
Safe: Template Injection Handling
Location: interpolate() method (line 258-272)
private interpolate(template: string): string {
if (!template) return "";
return template.replace(
/\{\{\s*([@#][\w.]+(\[\])?)\s*\}\}/g,
(fullMatch, match) => {
const value = this.resolve(clean);
return isArray
? JSON.stringify(_.castArray(value ?? []))
: String(value ?? ""); // ✅ Safe: explicit String() conversion
},
);
}Verdict: ✅ Safe - All values are explicitly converted to strings via String() or JSON.stringify(). No HTML context, so no XSS risk.
Safe: Form Input Handling
Location: typeForm() method (line 383-409)
const value = this.resolve(def);
this.form!.appendChild(
this.addInputElement(input, String(value)) // ✅ Safe: explicit String() conversion
);Verdict: ✅ Safe - All values converted to strings before DOM insertion.
2. Sensitive User Data Exposure
Assessment: Same exposure as master, no NEW sensitive data exposed ✅
| Data Type | Status | Location | Risk | Notes |
|---|---|---|---|---|
| JWT Tokens | ✅ Same | authorizationTokens:50-56 |
Medium | Exposed via #jwt, #token, #tokenBearer selectors - SAME as old service |
| User Email | ✅ Same | createJob() old, now in variables |
Low | Was in archiving.service, now in action config |
| User Profile | ✅ Same | userProfile$ selector |
Low | Admin, groups, access info - SAME as before |
| Dataset Metadata | ✅ Same | Static map selectors | Low | PID, sourceFolder, size, files - SAME fields |
| File Paths | ✅ Same | Multiple selectors | Low | Paths were already exposed in old service |
| Job Parameters | ✅ Same | Now in payload config | Low | Same data, just configured differently |
Details
Old Implementation (archiving.service.ts):
const data = {
jobParams: {
username: user.username, // ✅ User data
...extra,
},
emailJobInitiator: user.email, // ✅ User email
datasetList: datasets.map((dataset) => ({
pid: dataset.pid, // ✅ Dataset PID
files: [],
})),
type: archive ? "archive" : "retrieve",
};New Implementation (configurable-action.component.ts):
// Selectors expose the same data:
"#Dataset0Pid": () => ds0?.pid,
"#Dataset0SourceFolder": () => ds0?.sourceFolder,
"#DatasetsFilesPath": () => _(datasets).flatMap("files").map("path").value(),
// ...
// Tokens exposed via selectors:
"#jwt": () => this.jwt,
"#token": () => this.authService.getToken()?.id,
"#tokenBearer": () => `Bearer ${this.authService.getToken()?.id}`,Verdict: ✅ No new sensitive data exposure. The configurable actions framework exposes the same user and dataset metadata that was already exposed by the archiving service. The data is just accessed via configuration rather than hardcoded methods.
One Concern: Token in URL
Location: typeForm():388, typeXhr():355
// Form action
this.form.action = this.actionConfig.url; // URL could contain tokens
// XHR action
const url = this.interpolate(this.actionConfig.url); // URL interpolatedRisk: If config contains:
{
"url": "https://api.example.com/action?token={{ #token }}"
}Verdict:
3. Insecure API Usage
Assessment: No NEW insecure API usage ✅
| API/Feature | Status | Location | Notes |
|---|---|---|---|
| fetch() | ✅ Secure | typeXhr():364 |
Standard browser API, uses HTTPS via backend config |
| Form submission | ✅ Secure | typeForm():407 |
Standard HTML form, target controlled by config |
| HTTP Methods | ✅ Secure | Multiple locations | POST by default, configurable but not to unsafe methods |
| Credentials | ✅ Secure | authorizationTokens |
Tokens from auth service, not hardcoded |
| CORS | ✅ Not applicable | N/A | Browser enforces CORS, server-side config controls endpoints |
Details
XHR Action (typeXhr):
fetch(url, {
method: this.actionConfig.method || "POST", // Defaults to POST
headers, // Content-Type: application/json
body: this.preparePayload(),
})Analysis:
- Uses
fetch()which respects browser security policies - Method defaults to POST (safe default)
- Headers are explicitly set, no user-controlled headers that could override security headers
- Body is either JSON string or empty object
- URL comes from config, but config is admin-controlled
Form Action (typeForm):
this.form.target = this.actionConfig.target || "_self";
this.form.method = this.actionConfig.method || "POST";
this.form.action = this.actionConfig.url;Analysis:
- Standard HTML form submission
- Target defaults to
_self(same window) - Could be configured to open in new tab (
_blank) which is standard behavior - No security headers can be set on HTML forms (browser limitation)
Verdict: ✅ No insecure API usage introduced. The branch uses standard, secure browser APIs with the same security posture as the deleted archiving service.
4. Authentication Bypass
Assessment: No authentication bypass vulnerabilities introduced ✅
| Attack Vector | Status | Analysis |
|---|---|---|
| Token Manipulation | ❌ Not possible | Tokens come from authService.getToken() and usersService.usersControllerGetUserJWTV3() |
| Token Theft via Config | ❌ Not possible | Config is loaded server-side, not user-modifiable at runtime |
| PID Manipulation | ❌ Not possible | Dataset selections come from store, which comes from user actions on data they can access |
| Action Execution Without Auth | ❌ Not possible | All actions require configured tokens, same as old service |
| Privilege Escalation | ❌ Not possible | Authorization checks are server-side (SciCat API) |
| Session Hijacking | ❌ Not possible | No new ways to obtain or use session tokens |
Details
Authentication Flow (Unchanged):
// In constructor:
this.usersService.usersControllerGetUserJWTV3().subscribe((jwt) => {
this.jwt = jwt.jwt; // JWT obtained from authenticated service call
});
// Token access:
"#jwt": () => this.jwt, // From authenticated service
"#token": () => this.authService.getToken()?.id, // From auth serviceAuthorization:
- The old archiving service checked:
if (user) { ... }and validated email - The new configurable actions use:
#datasetOwner,#userIsAdmininevaluate() - Both rely on server-side authorization via SciCat API
- Client-side checks are for UI convenience, not security
Data Access:
- Dataset PIDs come from the store (
actionItems.datasets) - Datasets in store come from authenticated API calls
- Users can only access datasets they have permission for (enforced by SciCat API)
- No way to inject or manipulate dataset selections beyond what user can see
Verdict: ✅ No authentication bypass. The branch maintains the same authentication and authorization model as master. All security checks remain server-side.
Summary
| Security Concern | Introduced by Branch? | Severity | Status |
|---|---|---|---|
Code injection via Function |
✅ Yes | High | NEW vulnerability (admin-only) |
| Template injection | ❌ No | None | Safe |
| XSS via URL | ❌ No | None | Safe |
| Sensitive data exposure | ❌ No | None | Same as master |
| JWT tokens in config | ❌ No | Low | Same as master |
| Insecure API usage | ❌ No | None | Safe |
| Authentication bypass | ❌ No | None | Safe |
Overall Security Assessment: ⚠️ MOSTLY SECURE with one new concern
Final Verdict: The config_batch branch introduces ONE new security concern (code injection via Function constructor) but does not introduce any authentication bypass, insecure API usage, or new sensitive data exposure.
New Security Issue:
- Code Injection (High Risk, Admin-Only): The use of
new Function()for evaluating expressions could allow arbitrary code execution if an administrator sets a malicious configuration. This is a new attack surface that requires admin access to the configuration files.
Pre-existing Security Pattern (Not Introduced by This Branch):
- Tokens in URLs: The pattern of including tokens in URLs exists in both old and new code. This can leak tokens in server logs.
No Change:
- Authentication flow unchanged
- Authorization model unchanged (server-side enforcement)
- Sensitive data exposure unchanged
- API security unchanged
Recommendation
Before Merge:
- CRITICAL: Replace
new Function()with a safe expression evaluator, or add strict validation/sanitization of expression strings in config - Recommended: Add documentation warning administrators not to use untrusted expressions in config
- Optional: Consider using a sandboxed expression evaluator library
After Merge:
- Audit all configurable action expressions in production configs
- Consider implementing a config validation pipeline that rejects potentially dangerous expressions
Security Rating: 7/10 - Approve with critical fix required
The branch is architecturally sound and follows the same security patterns as the existing codebase, with the exception of the Function constructor usage which introduces a new attack vector for administrators with config access.
Generated by Mistral AI
Description
By setting
batchActionsEnabledandbatchActionsin config.json it renders them as actions in the overview page and in the cart (common ones are preserved)Motivation
This facilitates facilities to set custom actions in the cart and in particular helps us in adding new workflows, delete and restore ones.
This is an example of the config
Changes:
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Replace hardcoded archive/retrieve dataset jobs with configurable batch actions and integrate them into the datasets overview and batch views.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: