diff --git a/frontend/resources/vue/utils/rerunArguments.js b/frontend/resources/vue/utils/rerunArguments.js index d214960..ecebceb 100644 --- a/frontend/resources/vue/utils/rerunArguments.js +++ b/frontend/resources/vue/utils/rerunArguments.js @@ -10,6 +10,24 @@ function isNonStorableArgumentType (type) { return nonStorableArgumentTypes.has(type) } +function argumentSkipsValidation (type) { + return type === 'confirmation' || type === 'html' +} + +/** + * Mirrors backend restartArgumentRequired: an argument needs a stored value + * when it is validated and has no default. Proto ActionArgument has no + * `required` field — only `defaultValue`. + */ +function rerunArgumentRequired (arg) { + if (argumentSkipsValidation(arg?.type)) { + return false + } + + const defaultValue = arg?.defaultValue ?? '' + return defaultValue === '' +} + export function logEntryArgumentsToStartActionArgs (logEntry) { return (logEntry?.arguments ?? []).map((arg) => ({ name: arg.name, @@ -18,7 +36,9 @@ export function logEntryArgumentsToStartActionArgs (logEntry) { } export function rerunNeedsArgumentForm (action, logEntry) { - if (actionRequiresJustification(action?.justification) && !logEntry?.justification) { + // Always re-prompt when justification is required so each execution is + // explicitly justified rather than silently reusing a prior reason. + if (actionRequiresJustification(action?.justification)) { return true } @@ -37,7 +57,7 @@ export function hasMissingRerunArguments (action, storedArgs) { return true } - if (arg.required && !stored.has(arg.name)) { + if (rerunArgumentRequired(arg) && !stored.has(arg.name)) { return true } } @@ -45,25 +65,23 @@ export function hasMissingRerunArguments (action, storedArgs) { return false } -export function buildArgumentFormQuery (logEntry) { - const query = {} +/** + * Builds history.state.prefilledArguments for ArgumentForm (not URL query), + * matching ActionButton's prefill pattern and keeping values out of the URL. + */ +export function buildRerunPrefilledArguments (logEntry) { + const prefilled = {} for (const arg of logEntry?.arguments ?? []) { - query[arg.name] = arg.value + prefilled[arg.name] = arg.value } - return query + return prefilled } -export function buildRerunStartActionArgs (bindingId, logEntry, action) { - const startActionArgs = { +export function buildRerunStartActionArgs (bindingId, logEntry) { + return { bindingId, arguments: logEntryArgumentsToStartActionArgs(logEntry) } - - if (actionRequiresJustification(action?.justification) && logEntry?.justification) { - startActionArgs.justification = logEntry.justification - } - - return startActionArgs } diff --git a/frontend/resources/vue/utils/rerunArguments.test.mjs b/frontend/resources/vue/utils/rerunArguments.test.mjs index 4f34b9f..d7e2b62 100644 --- a/frontend/resources/vue/utils/rerunArguments.test.mjs +++ b/frontend/resources/vue/utils/rerunArguments.test.mjs @@ -1,7 +1,7 @@ import test from 'node:test' import assert from 'node:assert/strict' import { - buildArgumentFormQuery, + buildRerunPrefilledArguments, buildRerunStartActionArgs, hasMissingRerunArguments, logEntryArgumentsToStartActionArgs, @@ -26,8 +26,8 @@ test('logEntryArgumentsToStartActionArgs maps proto arguments for StartAction', test('hasMissingRerunArguments requires password fields to be re-entered', () => { const action = { arguments: [ - { name: 'user', type: 'ascii_identifier', required: true }, - { name: 'pass', type: 'password', required: true } + { name: 'user', type: 'ascii_identifier' }, + { name: 'pass', type: 'password' } ] } @@ -40,8 +40,8 @@ test('hasMissingRerunArguments requires password fields to be re-entered', () => test('hasMissingRerunArguments requires very_dangerous_raw_string fields to be re-entered', () => { const action = { arguments: [ - { name: 'host', type: 'ascii_identifier', required: true }, - { name: 'payload', type: 'very_dangerous_raw_string', required: false } + { name: 'host', type: 'ascii_identifier' }, + { name: 'payload', type: 'very_dangerous_raw_string' } ] } @@ -51,9 +51,10 @@ test('hasMissingRerunArguments requires very_dangerous_raw_string fields to be r ) }) -test('hasMissingRerunArguments detects missing required stored arguments', () => { +test('hasMissingRerunArguments detects missing stored args without relying on required flag', () => { + // Proto ActionArgument has no `required` field — only defaultValue. const action = { - arguments: [{ name: 'host', type: 'ascii_identifier', required: true }] + arguments: [{ name: 'host', type: 'ascii_identifier' }] } assert.equal(hasMissingRerunArguments(action, []), true) @@ -63,9 +64,40 @@ test('hasMissingRerunArguments detects missing required stored arguments', () => ) }) +test('hasMissingRerunArguments treats empty defaultValue as required', () => { + const action = { + arguments: [{ name: 'host', type: 'ascii_identifier', defaultValue: '' }] + } + + assert.equal(hasMissingRerunArguments(action, []), true) +}) + +test('hasMissingRerunArguments allows args that have a defaultValue', () => { + const action = { + arguments: [{ name: 'host', type: 'ascii_identifier', defaultValue: 'example.com' }] + } + + assert.equal(hasMissingRerunArguments(action, []), false) +}) + +test('hasMissingRerunArguments ignores confirmation and html args', () => { + const action = { + arguments: [ + { name: 'confirm', type: 'confirmation' }, + { name: 'help', type: 'html' }, + { name: 'host', type: 'ascii_identifier' } + ] + } + + assert.equal( + hasMissingRerunArguments(action, [{ name: 'host', value: 'db-1' }]), + false + ) +}) + test('rerunNeedsArgumentForm can start directly when stored args are complete', () => { const action = { - arguments: [{ name: 'host', type: 'ascii_identifier', required: true }] + arguments: [{ name: 'host', type: 'ascii_identifier' }] } const logEntry = { arguments: [{ name: 'host', value: 'db-1' }] @@ -74,36 +106,36 @@ test('rerunNeedsArgumentForm can start directly when stored args are complete', assert.equal(rerunNeedsArgumentForm(action, logEntry), false) }) -test('rerunNeedsArgumentForm opens the form when justification is missing', () => { - const action = { justification: ' ', arguments: [] } +test('rerunNeedsArgumentForm always opens the form when justification is required', () => { + const action = { + justification: ' ', + arguments: [{ name: 'host', type: 'ascii_identifier' }] + } + const logEntry = { + arguments: [{ name: 'host', value: 'db-1' }], + justification: 'approved change' + } + assert.equal(rerunNeedsArgumentForm(action, logEntry), true) assert.equal(rerunNeedsArgumentForm(action, {}), true) - assert.equal( - rerunNeedsArgumentForm(action, { justification: 'approved change' }), - false - ) }) -test('buildRerunStartActionArgs includes stored justification', () => { +test('buildRerunStartActionArgs uses stored arguments without justification', () => { assert.deepEqual( buildRerunStartActionArgs('binding-1', { arguments: [{ name: 'host', value: 'db-1' }], justification: 'maintenance window' - }, { - justification: ' ', - arguments: [{ name: 'host', type: 'ascii_identifier' }] }), { bindingId: 'binding-1', - arguments: [{ name: 'host', value: 'db-1' }], - justification: 'maintenance window' + arguments: [{ name: 'host', value: 'db-1' }] } ) }) -test('buildArgumentFormQuery prefills non-password stored arguments', () => { +test('buildRerunPrefilledArguments maps stored args for history.state', () => { assert.deepEqual( - buildArgumentFormQuery({ + buildRerunPrefilledArguments({ arguments: [ { name: 'host', value: 'db-1' }, { name: 'port', value: '5432' } @@ -115,3 +147,8 @@ test('buildArgumentFormQuery prefills non-password stored arguments', () => { } ) }) + +test('buildRerunPrefilledArguments returns empty object when nothing was stored', () => { + assert.deepEqual(buildRerunPrefilledArguments({}), {}) + assert.deepEqual(buildRerunPrefilledArguments(undefined), {}) +}) diff --git a/frontend/resources/vue/views/ExecutionView.vue b/frontend/resources/vue/views/ExecutionView.vue index 0aadb94..9e7ac0c 100644 --- a/frontend/resources/vue/views/ExecutionView.vue +++ b/frontend/resources/vue/views/ExecutionView.vue @@ -100,7 +100,11 @@ import { WorkoutRunIcon, Cancel02Icon, ArrowLeftIcon, DashboardSquare01Icon, Cop import { useRouter } from 'vue-router' import { buttonResults } from '../stores/buttonResults' import { requestReconnectNow } from '../../../js/websocket.js' -import { needsArgumentForm } from '../utils/needsArgumentForm.js' +import { + buildRerunPrefilledArguments, + buildRerunStartActionArgs, + rerunNeedsArgumentForm +} from '../utils/rerunArguments.js' const router = useRouter() @@ -224,16 +228,16 @@ async function rerunAction() { try { const binding = await window.client.getActionBinding({ bindingId }) - if (needsArgumentForm(binding.action)) { - router.push(`/actionBinding/${bindingId}/argumentForm`) + if (rerunNeedsArgumentForm(binding.action, logEntry.value)) { + router.push({ + path: `/actionBinding/${bindingId}/argumentForm`, + state: { prefilledArguments: buildRerunPrefilledArguments(logEntry.value) } + }) return } requestReconnectNow() - const startActionArgs = { - bindingId: bindingId, - arguments: [] - } + const startActionArgs = buildRerunStartActionArgs(bindingId, logEntry.value) const res = await window.client.startAction(startActionArgs) router.push(`/logs/${res.executionTrackingId}`) diff --git a/service/internal/executor/executor.go b/service/internal/executor/executor.go index ec3658f..0ca30d1 100644 --- a/service/internal/executor/executor.go +++ b/service/internal/executor/executor.go @@ -846,6 +846,19 @@ func stepACLCheck(req *ExecutionRequest) bool { } func stepParseArgs(req *ExecutionRequest) bool { + if !prepareArgumentsForExecution(req) { + return false + } + + ok := parseActionForExecution(req) + if ok { + copyStorableArgumentsToLogEntry(req) + } + + return ok +} + +func prepareArgumentsForExecution(req *ExecutionRequest) bool { ensureArgumentMap(req) if !hasBindingAndAction(req) { @@ -856,14 +869,17 @@ func stepParseArgs(req *ExecutionRequest) bool { if err := injectSystemArgs(req); err != nil { return fail(req, err) } - mangleInvalidArgumentValues(req) - copyStorableArgumentsToLogEntry(req) + mangleInvalidArgumentValues(req) + return true +} + +func parseActionForExecution(req *ExecutionRequest) bool { if hasExec(req) { return handleExecBranch(req) - } else { - return handleShellBranch(req) } + + return handleShellBranch(req) } func handleExecBranch(req *ExecutionRequest) bool {