From d62f5772f57890782e8045d5f0909e22bced620e Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:13:04 +1000 Subject: [PATCH 1/7] feat: add helper to strip inline creds when a connection is selected --- .../ConnectionInlineFieldCleaner.java | 50 +++++++++++++ .../ConnectionInlineFieldCleanerTest.java | 70 +++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java create mode 100644 octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java diff --git a/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java b/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java new file mode 100644 index 00000000..733dc677 --- /dev/null +++ b/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java @@ -0,0 +1,50 @@ +/* + * Copyright 2000-2012 Octopus Deploy Pty. Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package octopus.teamcity.server.connection; + +import java.util.Map; + +import jetbrains.buildServer.util.StringUtil; +import octopus.teamcity.common.OctopusConstants; + +/** + * Removes a build step's inline Octopus credential parameters when the step references a reusable + * connection instead. The connection supplies the server URL, API key, and version at build start, + * so persisting the old inline values would only leave stale credentials behind the connection. + * + *

The space parameter is deliberately left untouched: a step's own space intentionally overrides + * the connection's, and this class has no project context with which to resolve the connection. + */ +public final class ConnectionInlineFieldCleaner { + private static final OctopusConstants CONSTANTS = new OctopusConstants(); + + private ConnectionInlineFieldCleaner() {} + + public static void stripInlineFieldsIfUsingConnection(final Map properties) { + if (properties == null) { + return; + } + final boolean usingConnection = + !StringUtil.isEmptyOrSpaces(properties.get(CONSTANTS.getConnectionIdKey())); + if (!usingConnection) { + return; + } + properties.remove(CONSTANTS.getServerKey()); + properties.remove(CONSTANTS.getApiKey()); + properties.remove(CONSTANTS.getOctopusVersion()); + } +} diff --git a/octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java b/octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java new file mode 100644 index 00000000..515cff96 --- /dev/null +++ b/octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java @@ -0,0 +1,70 @@ +package octopus.teamcity.server.connection; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.HashMap; +import java.util.Map; + +import octopus.teamcity.common.OctopusConstants; +import org.junit.jupiter.api.Test; + +class ConnectionInlineFieldCleanerTest { + private static final OctopusConstants CONSTANTS = new OctopusConstants(); + + @Test + void removesInlineCredentialFieldsWhenConnectionSelected() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + properties.put(CONSTANTS.getSpaceName(), "Default"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties) + .doesNotContainKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + assertThat(properties).containsEntry(CONSTANTS.getSpaceName(), "Default"); + assertThat(properties).containsEntry(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + } + + @Test + void leavesInlineFieldsWhenNoConnectionSelected() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties) + .containsKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + } + + @Test + void leavesInlineFieldsWhenConnectionIdIsBlank() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), " "); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties) + .containsKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + } + + @Test + void toleratesConnectionSelectedWithNoInlineFields() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties).containsOnlyKeys(CONSTANTS.getConnectionIdKey()); + } +} From 6cb0a3cbbd8962be14fc45d37fdb032a1b40b34c Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:19:38 +1000 Subject: [PATCH 2/7] feat: blank inline credentials on save when a connection is selected --- .../OctopusBuildInformationRunType.java | 5 +++ .../server/OctopusCreateReleaseRunType.java | 5 +++ .../server/OctopusDeployReleaseRunType.java | 5 +++ .../server/OctopusPromoteReleaseRunType.java | 5 +++ .../server/OctopusPushPackageRunType.java | 5 +++ ...pusDeployReleaseRunTypeValidationTest.java | 34 +++++++++++++++++++ 6 files changed, 59 insertions(+) diff --git a/octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java b/octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java index 0e52b12a..9cc54b05 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java @@ -28,6 +28,7 @@ import jetbrains.buildServer.util.StringUtil; import jetbrains.buildServer.web.openapi.PluginDescriptor; import octopus.teamcity.common.OctopusConstants; +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -85,6 +86,10 @@ public Collection process(@Nullable final Map p checkNotEmpty(p, c.getPackageIdKey(), "Package ID must be specified", result); checkNotEmpty(p, c.getPackageVersionKey(), "Package version be specified", result); + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + return result; } }; diff --git a/octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java b/octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java index 36411198..acd959ec 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java @@ -12,6 +12,7 @@ import jetbrains.buildServer.util.StringUtil; import jetbrains.buildServer.web.openapi.PluginDescriptor; import octopus.teamcity.common.OctopusConstants; +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -68,6 +69,10 @@ public Collection process(@Nullable final Map p } checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + return result; } }; diff --git a/octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java b/octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java index 8f51eeff..cf24f63e 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java @@ -28,6 +28,7 @@ import jetbrains.buildServer.util.StringUtil; import jetbrains.buildServer.web.openapi.PluginDescriptor; import octopus.teamcity.common.OctopusConstants; +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -86,6 +87,10 @@ public Collection process(@Nullable final Map p checkNotEmpty(p, c.getReleaseNumberKey(), "Release number must be specified", result); checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + return result; } }; diff --git a/octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java b/octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java index 024614c2..70bbae5c 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java @@ -28,6 +28,7 @@ import jetbrains.buildServer.util.StringUtil; import jetbrains.buildServer.web.openapi.PluginDescriptor; import octopus.teamcity.common.OctopusConstants; +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -87,6 +88,10 @@ public Collection process(@Nullable final Map p p, c.getPromoteFromKey(), "Environment to promote from must be specified", result); checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + return result; } }; diff --git a/octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java b/octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java index 5342328e..cd2d35e3 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java @@ -28,6 +28,7 @@ import jetbrains.buildServer.util.StringUtil; import jetbrains.buildServer.web.openapi.PluginDescriptor; import octopus.teamcity.common.OctopusConstants; +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -84,6 +85,10 @@ public Collection process(@Nullable final Map p } checkNotEmpty(p, c.getPackagePathsKey(), "Package paths must be specified", result); + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + return result; } }; diff --git a/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java b/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java index 036b7d9e..27fb8227 100644 --- a/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java +++ b/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java @@ -55,4 +55,38 @@ void neitherConnectionNorManualIsInvalid() { final Map properties = withMandatoryNonCredentialFields(new HashMap<>()); assertThat(validate(properties)).contains(CONSTANTS.getServerKey(), CONSTANTS.getApiKey()); } + + @Test + void stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses() { + final Map properties = withMandatoryNonCredentialFields(new HashMap<>()); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + properties.put(CONSTANTS.getSpaceName(), "Default"); + + validate(properties); + + assertThat(properties) + .doesNotContainKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + assertThat(properties).containsEntry(CONSTANTS.getSpaceName(), "Default"); + } + + @Test + void retainsInlineCredentialFieldsWhenValidationFails() { + // Missing mandatory project name => validation fails, so nothing is stripped. + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + + final Collection errors = validate(properties); + + assertThat(errors).contains(CONSTANTS.getProjectNameKey()); + assertThat(properties) + .containsKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + } } From 5cd304d7b5e06e22eecd241712dfdc55e5c14c62 Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:27:15 +1000 Subject: [PATCH 3/7] refactor: align copyright header and assert validation precondition --- .../ConnectionInlineFieldCleaner.java | 19 +++++++++---------- ...pusDeployReleaseRunTypeValidationTest.java | 2 +- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java b/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java index 733dc677..e20b0b1c 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java @@ -1,17 +1,16 @@ /* - * Copyright 2000-2012 Octopus Deploy Pty. Ltd. + * Copyright (c) Octopus Deploy and contributors. All rights reserved. * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use + * these files except in compliance with the License. You may obtain a copy of the + * License at * - * http://www.apache.org/licenses/LICENSE-2.0 + * http://www.apache.org/licenses/LICENSE-2.0 * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. + * Unless required by applicable law or agreed to in writing, software distributed + * under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR + * CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. */ package octopus.teamcity.server.connection; diff --git a/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java b/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java index 27fb8227..512f1f9b 100644 --- a/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java +++ b/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java @@ -65,7 +65,7 @@ void stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses() { properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); properties.put(CONSTANTS.getSpaceName(), "Default"); - validate(properties); + assertThat(validate(properties)).isEmpty(); assertThat(properties) .doesNotContainKeys( From 885db80e0de4e5b0519c71608ff4284a63b00588 Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:28:44 +1000 Subject: [PATCH 4/7] docs: add spec and plan for blanking inline creds on connection select Co-Authored-By: Claude Opus 4.8 (1M context) --- ...blank-inline-creds-on-connection-select.md | 414 ++++++++++++++++++ ...nline-creds-on-connection-select-design.md | 146 ++++++ 2 files changed, 560 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md create mode 100644 docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md diff --git a/docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md b/docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md new file mode 100644 index 00000000..602cf246 --- /dev/null +++ b/docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md @@ -0,0 +1,414 @@ +# Blank Inline Credentials When A Connection Is Selected — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When a build step is saved with an Octopus connection selected and validation passes, strip the now-redundant inline URL / API key / version runner parameters from the stored step config. + +**Architecture:** Add a small stateless helper in the `octopus.teamcity.server.connection` package that removes the three inline credential keys when `octopus_connection_id` is non-blank. Each of the five connection-aware runners' anonymous `PropertiesProcessor.process(...)` calls the helper as its final step, guarded by `result.isEmpty()` so stripping only happens on an otherwise-valid save. TeamCity persists a `PropertiesProcessor`-modified map on save (per its Javadoc), so removing keys there is the supported mechanism. + +**Tech Stack:** Java 8, JUnit 5 (Jupiter), AssertJ, Mockito, TeamCity server-api, Gradle. + +## Global Constraints + +- Java language level **8**; build with **JDK 11** (JDK 8 also works; newer JDKs break error-prone). CI runs `./gradlew build test`. +- error-prone runs with `-Werror` — warnings fail the build. +- Formatting is google-java-format 1.7 with import order `com.octopus`, `java`, `` (everything else last). Run `./gradlew spotlessApply` before committing Java changes; `spotlessCheck` is part of `build`. +- **No single-letter variable, field, or parameter names in new code.** (Pre-existing `c`/`p` in the runner processors stay as-is; new code uses descriptive names.) +- Inline credential parameter keys (from `OctopusConstants`): server URL = `octopus_host` (`getServerKey()`), API key = `secure:octopus_apikey` (`getApiKey()`), version = `octopus_version` (`getOctopusVersion()`), connection id = `octopus_connection_id` (`getConnectionIdKey()`). Space = `octopus_space_name` (`getSpaceName()`) is **NOT** stripped. + +--- + +### Task 1: `ConnectionInlineFieldCleaner` helper + +**Files:** +- Create: `octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java` +- Test: `octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java` + +**Interfaces:** +- Consumes: `OctopusConstants` (`getConnectionIdKey()`, `getServerKey()`, `getApiKey()`, `getOctopusVersion()`, `getSpaceName()`); `jetbrains.buildServer.util.StringUtil.isEmptyOrSpaces(String)`. +- Produces: `public static void ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(Map properties)` — when `octopus_connection_id` is present and non-blank, removes `octopus_host`, `secure:octopus_apikey`, and `octopus_version` from the map; otherwise leaves the map unchanged. Null-safe (no-op on a null map). + +- [ ] **Step 1: Write the failing test** + +Create `octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java`: + +```java +package octopus.teamcity.server.connection; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.HashMap; +import java.util.Map; + +import octopus.teamcity.common.OctopusConstants; +import org.junit.jupiter.api.Test; + +class ConnectionInlineFieldCleanerTest { + private static final OctopusConstants CONSTANTS = new OctopusConstants(); + + @Test + void removesInlineCredentialFieldsWhenConnectionSelected() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + properties.put(CONSTANTS.getSpaceName(), "Default"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties) + .doesNotContainKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + assertThat(properties).containsEntry(CONSTANTS.getSpaceName(), "Default"); + assertThat(properties).containsEntry(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + } + + @Test + void leavesInlineFieldsWhenNoConnectionSelected() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties) + .containsKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + } + + @Test + void leavesInlineFieldsWhenConnectionIdIsBlank() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), " "); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties) + .containsKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + } + + @Test + void toleratesConnectionSelectedWithNoInlineFields() { + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + + assertThat(properties).containsOnlyKeys(CONSTANTS.getConnectionIdKey()); + } +} +``` + +- [ ] **Step 2: Run the test to verify it fails** + +Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.connection.ConnectionInlineFieldCleanerTest"` +Expected: FAIL — compilation error, `ConnectionInlineFieldCleaner` does not exist. + +- [ ] **Step 3: Create the helper** + +Create `octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java`: + +```java +/* + * Copyright 2000-2012 Octopus Deploy Pty. Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package octopus.teamcity.server.connection; + +import java.util.Map; + +import jetbrains.buildServer.util.StringUtil; +import octopus.teamcity.common.OctopusConstants; + +/** + * Removes a build step's inline Octopus credential parameters when the step references a reusable + * connection instead. The connection supplies the server URL, API key, and version at build start, + * so persisting the old inline values would only leave stale credentials behind the connection. + * + *

The space parameter is deliberately left untouched: a step's own space intentionally overrides + * the connection's, and this class has no project context with which to resolve the connection. + */ +public final class ConnectionInlineFieldCleaner { + private static final OctopusConstants CONSTANTS = new OctopusConstants(); + + private ConnectionInlineFieldCleaner() {} + + public static void stripInlineFieldsIfUsingConnection(final Map properties) { + if (properties == null) { + return; + } + final boolean usingConnection = + !StringUtil.isEmptyOrSpaces(properties.get(CONSTANTS.getConnectionIdKey())); + if (!usingConnection) { + return; + } + properties.remove(CONSTANTS.getServerKey()); + properties.remove(CONSTANTS.getApiKey()); + properties.remove(CONSTANTS.getOctopusVersion()); + } +} +``` + +- [ ] **Step 4: Format** + +Run: `./gradlew spotlessApply` +Expected: BUILD SUCCESSFUL (reformats the new files if needed). + +- [ ] **Step 5: Run the test to verify it passes** + +Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.connection.ConnectionInlineFieldCleanerTest"` +Expected: PASS (4 tests). + +- [ ] **Step 6: Commit** + +```bash +git add octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java \ + octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java +git commit -m "feat: add helper to strip inline creds when a connection is selected" +``` + +--- + +### Task 2: Wire the cleaner into the five connection-aware runners + +**Files:** +- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java` +- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java` +- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java` +- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java` +- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java` +- Test: `octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java` + +**Interfaces:** +- Consumes: `ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(Map)` from Task 1. +- Produces: no new public API. Behavior change only: `getRunnerPropertiesProcessor().process(properties)` now strips the three inline credential keys from `properties` when a connection is selected and the returned `InvalidProperty` collection is empty. + +Note: `OctopusPackPackageRunType` has no connection handling (local packaging, no server credentials) and is intentionally left untouched. + +- [ ] **Step 1: Write the failing runner-level tests** + +Add these two tests to `octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java` (inside the class, after the existing tests). The existing `validate(...)` helper passes the map straight into `process(...)`, so mutations are observable on the same `properties` reference afterward. + +```java + @Test + void stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses() { + final Map properties = withMandatoryNonCredentialFields(new HashMap<>()); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + properties.put(CONSTANTS.getSpaceName(), "Default"); + + validate(properties); + + assertThat(properties) + .doesNotContainKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + assertThat(properties).containsEntry(CONSTANTS.getSpaceName(), "Default"); + } + + @Test + void retainsInlineCredentialFieldsWhenValidationFails() { + // Missing mandatory project name => validation fails, so nothing is stripped. + final Map properties = new HashMap<>(); + properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); + properties.put(CONSTANTS.getServerKey(), "https://octo"); + properties.put(CONSTANTS.getApiKey(), "API-KEY"); + properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); + + final Collection errors = validate(properties); + + assertThat(errors).contains(CONSTANTS.getProjectNameKey()); + assertThat(properties) + .containsKeys( + CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); + } +``` + +(The file already imports `java.util.Collection`, `java.util.HashMap`, `java.util.Map`, and statically imports `assertThat`, so no new imports are needed.) + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.OctopusDeployReleaseRunTypeValidationTest"` +Expected: FAIL — `stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses` fails because the inline keys are still present (stripping not yet wired in). `retainsInlineCredentialFieldsWhenValidationFails` should pass already (nothing strips yet), which is fine. + +- [ ] **Step 3: Wire the cleaner into `OctopusDeployReleaseRunType`** + +In `octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java`, add the import (with the other `octopus.*` imports): + +```java +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; +``` + +Then in `process(...)`, replace the final return block: + +```java + checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + checkNotEmpty(p, c.getReleaseNumberKey(), "Release number must be specified", result); + checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); + + return result; +``` + +with: + +```java + checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + checkNotEmpty(p, c.getReleaseNumberKey(), "Release number must be specified", result); + checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); + + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + + return result; +``` + +- [ ] **Step 4: Run the Deploy tests to verify they pass** + +Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.OctopusDeployReleaseRunTypeValidationTest"` +Expected: PASS (existing tests plus the two new ones). + +- [ ] **Step 5: Wire the cleaner into the other four runners** + +Apply the same two edits (add the import, and wrap the strip call in `if (result.isEmpty()) { ... }` immediately before `return result;`) to each of these. The import line is identical in every file: + +```java +import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; +``` + +**`OctopusCreateReleaseRunType.java`** — replace: + +```java + checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + + return result; +``` + +with: + +```java + checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + + return result; +``` + +**`OctopusPromoteReleaseRunType.java`** — replace: + +```java + checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + checkNotEmpty( + p, c.getPromoteFromKey(), "Environment to promote from must be specified", result); + checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); + + return result; +``` + +with: + +```java + checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); + checkNotEmpty( + p, c.getPromoteFromKey(), "Environment to promote from must be specified", result); + checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); + + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + + return result; +``` + +**`OctopusPushPackageRunType.java`** — replace: + +```java + checkNotEmpty(p, c.getPackagePathsKey(), "Package paths must be specified", result); + + return result; +``` + +with: + +```java + checkNotEmpty(p, c.getPackagePathsKey(), "Package paths must be specified", result); + + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + + return result; +``` + +**`OctopusBuildInformationRunType.java`** — replace: + +```java + checkNotEmpty(p, c.getPackageIdKey(), "Package ID must be specified", result); + checkNotEmpty(p, c.getPackageVersionKey(), "Package version be specified", result); + + return result; +``` + +with: + +```java + checkNotEmpty(p, c.getPackageIdKey(), "Package ID must be specified", result); + checkNotEmpty(p, c.getPackageVersionKey(), "Package version be specified", result); + + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); + } + + return result; +``` + +- [ ] **Step 6: Format** + +Run: `./gradlew spotlessApply` +Expected: BUILD SUCCESSFUL. + +- [ ] **Step 7: Full build and test (matches CI)** + +Run: `./gradlew build test` +Expected: BUILD SUCCESSFUL — compile, spotlessCheck, error-prone (`-Werror`), license check, and all unit tests pass. + +- [ ] **Step 8: Commit** + +```bash +git add octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java \ + octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java \ + octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java \ + octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java \ + octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java \ + octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java +git commit -m "feat: blank inline credentials on save when a connection is selected" +``` + +--- + +## Notes + +- **Space is never stripped** — deliberate; the step space overrides the connection space, and the processor has no project context to resolve the connection's space. +- **`octopus_apikey_source` / OIDC inline keys are left as-is** — `OctopusConnectionBuildStartProcessor` overwrites them from the connection at build start, so any leftover value is harmless. +- **e2e coverage** stays in its separate PR per the existing plan; this plan is unit-tested only. diff --git a/docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md b/docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md new file mode 100644 index 00000000..b5721f54 --- /dev/null +++ b/docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md @@ -0,0 +1,146 @@ +# Blank inline credentials when a connection is selected + +## Problem + +A build step can either supply Octopus credentials inline (URL / API key / version / +space) or reference a reusable Octopus connection via `octopus_connection_id`. Today, +switching a step from inline credentials to a connection leaves the old inline values +persisted in the step config — they are simply ignored at build time. Stale credentials +(including a masked-but-stored API key) linger behind the connection reference. + +When a step is saved with a connection selected, the now-redundant inline credential +fields should be removed from the stored runner parameters. + +## Behavior + +On save, when `octopus_connection_id` is non-blank **and validation passes** (the +processor produces no `InvalidProperty` entries), remove these inline runner-parameter +keys from the properties map before it is stored: + +- `octopus_host` — the server URL (`OctopusConstants.getServerKey()`) +- `secure:octopus_apikey` — the API key (`OctopusConstants.getApiKey()`) +- `octopus_version` — the Octopus version (`OctopusConstants.getOctopusVersion()`) + +`octopus_space_name` is **not** removed. See "Why space is left alone" below. + +When `octopus_connection_id` is blank (the step uses inline fields), or when validation +fails, the map is left untouched. Leaving the map intact on a failed save means the edit +form is redisplayed with the user's entered values, rather than having fields silently +cleared underneath them. + +### Why this persists + +`jetbrains.buildServer.serverSide.PropertiesProcessor.process(Map)` may modify the +properties map, not only validate it. Per its Javadoc: *"Properties map passed as +argument can be verified or modified by the processor ... If processor was called during +saving properties to a configuration file, then modified map will be stored without +changes."* Removing keys inside `process()` is therefore the supported way to strip them +on save. + +### Why space is left alone + +`octopus_space_name` has deliberate override semantics: +`OctopusConnectionBuildStartProcessor` injects the connection's space **only if the step +did not set one** — a step's own space intentionally wins over the connection's. Blanking +the step space on save would destroy that feature. + +### Scope: UI save path only + +Stripping runs inside the runner `PropertiesProcessor`, which is the save path for the +web edit form. Steps created or updated by other means — the REST API, versioned settings +(Kotlin DSL / project-config XML in VCS), or config import — may not route through +`process()`, so stale inline credentials could persist there. This is a hygiene +limitation, not a correctness issue: whenever a connection is selected those leftover +inline values are ignored at build time (`OctopusConnectionBuildStartProcessor` overwrites +URL / API key / version from the connection). + +### Why space is left alone (continued) + +Additionally, `PropertiesProcessor.process(Map)` receives only the properties map — no +project context — so it cannot resolve the connection via `OctopusConnectionsManager` to +check whether the connection itself defines a space. A conditional "blank the step space +only when the connection has one" rule is not implementable at this layer. + +Leaving the step space untouched does the right thing in both cases: if the connection's +space is blank the step keeps its space (the only source); if the connection defines a +space the step's retained space still wins, matching documented behavior. + +## Design + +A small shared helper in the `octopus.teamcity.server.connection` package: + +```java +public final class ConnectionInlineFieldCleaner { + // Removes the inline URL, API key, and version keys when a connection is selected. + public static void stripInlineFieldsIfUsingConnection(Map properties); +} +``` + +Each connection-aware runner's anonymous `PropertiesProcessor.process(...)` runs its +existing validation first, then calls the helper only when validation produced no +errors: + +```java +public Collection process(@Nullable final Map properties) { + final Collection result = new ArrayList<>(); + if (properties == null) return result; + + final boolean usingConnection = + !StringUtil.isEmptyOrSpaces(properties.get(constants.getConnectionIdKey())); + if (!usingConnection) { + checkNotEmpty(properties, constants.getApiKey(), "API key must be specified", result); + checkNotEmpty(properties, constants.getServerKey(), "Server must be specified", result); + } + // ... remaining required-field checks unchanged ... + + if (result.isEmpty()) { + ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); + } + return result; +} +``` + +The helper is self-contained: it re-checks that `octopus_connection_id` is non-blank and +removes the three inline keys, so callers only need to guard on validation success. +Extracting it avoids copy-pasting the same three `remove()` calls into five anonymous +processors. + +### Applies to + +The five connection-aware runners: + +- `OctopusCreateReleaseRunType` +- `OctopusDeployReleaseRunType` +- `OctopusPromoteReleaseRunType` +- `OctopusPushPackageRunType` +- `OctopusBuildInformationRunType` + +`OctopusPackPackageRunType` has no connection handling (local packaging, no server +credentials) and is left untouched. + +### Interaction with existing validation + +Validation is unchanged and runs first; stripping happens only afterward, and only when +the result collection is empty. When a connection is selected the inline API-key / +server-URL required checks are already skipped, so a connection-backed step validates +cleanly and its inline keys are then stripped. The `octopus_apikey_source` / OIDC inline +keys are left as-is — at build start +`OctopusConnectionBuildStartProcessor` overwrites them from the connection, so any +leftover value is harmless. + +## Testing + +Unit-test the helper directly: + +- Connection id set → `octopus_host`, `secure:octopus_apikey`, `octopus_version` removed; + `octopus_space_name` and unrelated keys retained. +- Connection id blank → map unchanged. +- Connection id set but inline keys already absent → no error, map unchanged aside from + the (absent) keys. + +Because the "strip only when validation passes" guard lives in each runner's +`process()`, also cover it at that level (at least one runner): a connection-selected +step that fails a required-field check (e.g. missing project name) returns the +`InvalidProperty` and leaves the inline URL / API key / version intact. + +e2e coverage stays in its separate PR per the existing plan. From 61fda916aeed4bca67d728051fdf6e736ecbabf6 Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:30:24 +1000 Subject: [PATCH 5/7] docs: remove implementation plan Co-Authored-By: Claude Opus 4.8 (1M context) --- ...blank-inline-creds-on-connection-select.md | 414 ------------------ 1 file changed, 414 deletions(-) delete mode 100644 docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md diff --git a/docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md b/docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md deleted file mode 100644 index 602cf246..00000000 --- a/docs/superpowers/plans/2026-07-02-blank-inline-creds-on-connection-select.md +++ /dev/null @@ -1,414 +0,0 @@ -# Blank Inline Credentials When A Connection Is Selected — Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** When a build step is saved with an Octopus connection selected and validation passes, strip the now-redundant inline URL / API key / version runner parameters from the stored step config. - -**Architecture:** Add a small stateless helper in the `octopus.teamcity.server.connection` package that removes the three inline credential keys when `octopus_connection_id` is non-blank. Each of the five connection-aware runners' anonymous `PropertiesProcessor.process(...)` calls the helper as its final step, guarded by `result.isEmpty()` so stripping only happens on an otherwise-valid save. TeamCity persists a `PropertiesProcessor`-modified map on save (per its Javadoc), so removing keys there is the supported mechanism. - -**Tech Stack:** Java 8, JUnit 5 (Jupiter), AssertJ, Mockito, TeamCity server-api, Gradle. - -## Global Constraints - -- Java language level **8**; build with **JDK 11** (JDK 8 also works; newer JDKs break error-prone). CI runs `./gradlew build test`. -- error-prone runs with `-Werror` — warnings fail the build. -- Formatting is google-java-format 1.7 with import order `com.octopus`, `java`, `` (everything else last). Run `./gradlew spotlessApply` before committing Java changes; `spotlessCheck` is part of `build`. -- **No single-letter variable, field, or parameter names in new code.** (Pre-existing `c`/`p` in the runner processors stay as-is; new code uses descriptive names.) -- Inline credential parameter keys (from `OctopusConstants`): server URL = `octopus_host` (`getServerKey()`), API key = `secure:octopus_apikey` (`getApiKey()`), version = `octopus_version` (`getOctopusVersion()`), connection id = `octopus_connection_id` (`getConnectionIdKey()`). Space = `octopus_space_name` (`getSpaceName()`) is **NOT** stripped. - ---- - -### Task 1: `ConnectionInlineFieldCleaner` helper - -**Files:** -- Create: `octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java` -- Test: `octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java` - -**Interfaces:** -- Consumes: `OctopusConstants` (`getConnectionIdKey()`, `getServerKey()`, `getApiKey()`, `getOctopusVersion()`, `getSpaceName()`); `jetbrains.buildServer.util.StringUtil.isEmptyOrSpaces(String)`. -- Produces: `public static void ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(Map properties)` — when `octopus_connection_id` is present and non-blank, removes `octopus_host`, `secure:octopus_apikey`, and `octopus_version` from the map; otherwise leaves the map unchanged. Null-safe (no-op on a null map). - -- [ ] **Step 1: Write the failing test** - -Create `octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java`: - -```java -package octopus.teamcity.server.connection; - -import static org.assertj.core.api.Assertions.assertThat; - -import java.util.HashMap; -import java.util.Map; - -import octopus.teamcity.common.OctopusConstants; -import org.junit.jupiter.api.Test; - -class ConnectionInlineFieldCleanerTest { - private static final OctopusConstants CONSTANTS = new OctopusConstants(); - - @Test - void removesInlineCredentialFieldsWhenConnectionSelected() { - final Map properties = new HashMap<>(); - properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); - properties.put(CONSTANTS.getServerKey(), "https://octo"); - properties.put(CONSTANTS.getApiKey(), "API-KEY"); - properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); - properties.put(CONSTANTS.getSpaceName(), "Default"); - - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); - - assertThat(properties) - .doesNotContainKeys( - CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); - assertThat(properties).containsEntry(CONSTANTS.getSpaceName(), "Default"); - assertThat(properties).containsEntry(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); - } - - @Test - void leavesInlineFieldsWhenNoConnectionSelected() { - final Map properties = new HashMap<>(); - properties.put(CONSTANTS.getServerKey(), "https://octo"); - properties.put(CONSTANTS.getApiKey(), "API-KEY"); - properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); - - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); - - assertThat(properties) - .containsKeys( - CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); - } - - @Test - void leavesInlineFieldsWhenConnectionIdIsBlank() { - final Map properties = new HashMap<>(); - properties.put(CONSTANTS.getConnectionIdKey(), " "); - properties.put(CONSTANTS.getServerKey(), "https://octo"); - properties.put(CONSTANTS.getApiKey(), "API-KEY"); - properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); - - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); - - assertThat(properties) - .containsKeys( - CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); - } - - @Test - void toleratesConnectionSelectedWithNoInlineFields() { - final Map properties = new HashMap<>(); - properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); - - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); - - assertThat(properties).containsOnlyKeys(CONSTANTS.getConnectionIdKey()); - } -} -``` - -- [ ] **Step 2: Run the test to verify it fails** - -Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.connection.ConnectionInlineFieldCleanerTest"` -Expected: FAIL — compilation error, `ConnectionInlineFieldCleaner` does not exist. - -- [ ] **Step 3: Create the helper** - -Create `octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java`: - -```java -/* - * Copyright 2000-2012 Octopus Deploy Pty. Ltd. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package octopus.teamcity.server.connection; - -import java.util.Map; - -import jetbrains.buildServer.util.StringUtil; -import octopus.teamcity.common.OctopusConstants; - -/** - * Removes a build step's inline Octopus credential parameters when the step references a reusable - * connection instead. The connection supplies the server URL, API key, and version at build start, - * so persisting the old inline values would only leave stale credentials behind the connection. - * - *

The space parameter is deliberately left untouched: a step's own space intentionally overrides - * the connection's, and this class has no project context with which to resolve the connection. - */ -public final class ConnectionInlineFieldCleaner { - private static final OctopusConstants CONSTANTS = new OctopusConstants(); - - private ConnectionInlineFieldCleaner() {} - - public static void stripInlineFieldsIfUsingConnection(final Map properties) { - if (properties == null) { - return; - } - final boolean usingConnection = - !StringUtil.isEmptyOrSpaces(properties.get(CONSTANTS.getConnectionIdKey())); - if (!usingConnection) { - return; - } - properties.remove(CONSTANTS.getServerKey()); - properties.remove(CONSTANTS.getApiKey()); - properties.remove(CONSTANTS.getOctopusVersion()); - } -} -``` - -- [ ] **Step 4: Format** - -Run: `./gradlew spotlessApply` -Expected: BUILD SUCCESSFUL (reformats the new files if needed). - -- [ ] **Step 5: Run the test to verify it passes** - -Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.connection.ConnectionInlineFieldCleanerTest"` -Expected: PASS (4 tests). - -- [ ] **Step 6: Commit** - -```bash -git add octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java \ - octopus-server/src/test/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleanerTest.java -git commit -m "feat: add helper to strip inline creds when a connection is selected" -``` - ---- - -### Task 2: Wire the cleaner into the five connection-aware runners - -**Files:** -- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java` -- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java` -- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java` -- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java` -- Modify: `octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java` -- Test: `octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java` - -**Interfaces:** -- Consumes: `ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(Map)` from Task 1. -- Produces: no new public API. Behavior change only: `getRunnerPropertiesProcessor().process(properties)` now strips the three inline credential keys from `properties` when a connection is selected and the returned `InvalidProperty` collection is empty. - -Note: `OctopusPackPackageRunType` has no connection handling (local packaging, no server credentials) and is intentionally left untouched. - -- [ ] **Step 1: Write the failing runner-level tests** - -Add these two tests to `octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java` (inside the class, after the existing tests). The existing `validate(...)` helper passes the map straight into `process(...)`, so mutations are observable on the same `properties` reference afterward. - -```java - @Test - void stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses() { - final Map properties = withMandatoryNonCredentialFields(new HashMap<>()); - properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); - properties.put(CONSTANTS.getServerKey(), "https://octo"); - properties.put(CONSTANTS.getApiKey(), "API-KEY"); - properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); - properties.put(CONSTANTS.getSpaceName(), "Default"); - - validate(properties); - - assertThat(properties) - .doesNotContainKeys( - CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); - assertThat(properties).containsEntry(CONSTANTS.getSpaceName(), "Default"); - } - - @Test - void retainsInlineCredentialFieldsWhenValidationFails() { - // Missing mandatory project name => validation fails, so nothing is stripped. - final Map properties = new HashMap<>(); - properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); - properties.put(CONSTANTS.getServerKey(), "https://octo"); - properties.put(CONSTANTS.getApiKey(), "API-KEY"); - properties.put(CONSTANTS.getOctopusVersion(), "3.0+"); - - final Collection errors = validate(properties); - - assertThat(errors).contains(CONSTANTS.getProjectNameKey()); - assertThat(properties) - .containsKeys( - CONSTANTS.getServerKey(), CONSTANTS.getApiKey(), CONSTANTS.getOctopusVersion()); - } -``` - -(The file already imports `java.util.Collection`, `java.util.HashMap`, `java.util.Map`, and statically imports `assertThat`, so no new imports are needed.) - -- [ ] **Step 2: Run the tests to verify they fail** - -Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.OctopusDeployReleaseRunTypeValidationTest"` -Expected: FAIL — `stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses` fails because the inline keys are still present (stripping not yet wired in). `retainsInlineCredentialFieldsWhenValidationFails` should pass already (nothing strips yet), which is fine. - -- [ ] **Step 3: Wire the cleaner into `OctopusDeployReleaseRunType`** - -In `octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java`, add the import (with the other `octopus.*` imports): - -```java -import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; -``` - -Then in `process(...)`, replace the final return block: - -```java - checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); - checkNotEmpty(p, c.getReleaseNumberKey(), "Release number must be specified", result); - checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); - - return result; -``` - -with: - -```java - checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); - checkNotEmpty(p, c.getReleaseNumberKey(), "Release number must be specified", result); - checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); - - if (result.isEmpty()) { - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); - } - - return result; -``` - -- [ ] **Step 4: Run the Deploy tests to verify they pass** - -Run: `./gradlew :octopus-server:test --tests "octopus.teamcity.server.OctopusDeployReleaseRunTypeValidationTest"` -Expected: PASS (existing tests plus the two new ones). - -- [ ] **Step 5: Wire the cleaner into the other four runners** - -Apply the same two edits (add the import, and wrap the strip call in `if (result.isEmpty()) { ... }` immediately before `return result;`) to each of these. The import line is identical in every file: - -```java -import octopus.teamcity.server.connection.ConnectionInlineFieldCleaner; -``` - -**`OctopusCreateReleaseRunType.java`** — replace: - -```java - checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); - - return result; -``` - -with: - -```java - checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); - - if (result.isEmpty()) { - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); - } - - return result; -``` - -**`OctopusPromoteReleaseRunType.java`** — replace: - -```java - checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); - checkNotEmpty( - p, c.getPromoteFromKey(), "Environment to promote from must be specified", result); - checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); - - return result; -``` - -with: - -```java - checkNotEmpty(p, c.getProjectNameKey(), "Project name must be specified", result); - checkNotEmpty( - p, c.getPromoteFromKey(), "Environment to promote from must be specified", result); - checkNotEmpty(p, c.getDeployToKey(), "Deploy to must be specified", result); - - if (result.isEmpty()) { - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); - } - - return result; -``` - -**`OctopusPushPackageRunType.java`** — replace: - -```java - checkNotEmpty(p, c.getPackagePathsKey(), "Package paths must be specified", result); - - return result; -``` - -with: - -```java - checkNotEmpty(p, c.getPackagePathsKey(), "Package paths must be specified", result); - - if (result.isEmpty()) { - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); - } - - return result; -``` - -**`OctopusBuildInformationRunType.java`** — replace: - -```java - checkNotEmpty(p, c.getPackageIdKey(), "Package ID must be specified", result); - checkNotEmpty(p, c.getPackageVersionKey(), "Package version be specified", result); - - return result; -``` - -with: - -```java - checkNotEmpty(p, c.getPackageIdKey(), "Package ID must be specified", result); - checkNotEmpty(p, c.getPackageVersionKey(), "Package version be specified", result); - - if (result.isEmpty()) { - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(p); - } - - return result; -``` - -- [ ] **Step 6: Format** - -Run: `./gradlew spotlessApply` -Expected: BUILD SUCCESSFUL. - -- [ ] **Step 7: Full build and test (matches CI)** - -Run: `./gradlew build test` -Expected: BUILD SUCCESSFUL — compile, spotlessCheck, error-prone (`-Werror`), license check, and all unit tests pass. - -- [ ] **Step 8: Commit** - -```bash -git add octopus-server/src/main/java/octopus/teamcity/server/OctopusCreateReleaseRunType.java \ - octopus-server/src/main/java/octopus/teamcity/server/OctopusDeployReleaseRunType.java \ - octopus-server/src/main/java/octopus/teamcity/server/OctopusPromoteReleaseRunType.java \ - octopus-server/src/main/java/octopus/teamcity/server/OctopusPushPackageRunType.java \ - octopus-server/src/main/java/octopus/teamcity/server/OctopusBuildInformationRunType.java \ - octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java -git commit -m "feat: blank inline credentials on save when a connection is selected" -``` - ---- - -## Notes - -- **Space is never stripped** — deliberate; the step space overrides the connection space, and the processor has no project context to resolve the connection's space. -- **`octopus_apikey_source` / OIDC inline keys are left as-is** — `OctopusConnectionBuildStartProcessor` overwrites them from the connection at build start, so any leftover value is harmless. -- **e2e coverage** stays in its separate PR per the existing plan; this plan is unit-tested only. From 7167e548b3ec7efe64af001458fa16ada253b9af Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:30:38 +1000 Subject: [PATCH 6/7] docs: remove design spec Co-Authored-By: Claude Opus 4.8 (1M context) --- ...nline-creds-on-connection-select-design.md | 146 ------------------ 1 file changed, 146 deletions(-) delete mode 100644 docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md diff --git a/docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md b/docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md deleted file mode 100644 index b5721f54..00000000 --- a/docs/superpowers/specs/2026-07-02-blank-inline-creds-on-connection-select-design.md +++ /dev/null @@ -1,146 +0,0 @@ -# Blank inline credentials when a connection is selected - -## Problem - -A build step can either supply Octopus credentials inline (URL / API key / version / -space) or reference a reusable Octopus connection via `octopus_connection_id`. Today, -switching a step from inline credentials to a connection leaves the old inline values -persisted in the step config — they are simply ignored at build time. Stale credentials -(including a masked-but-stored API key) linger behind the connection reference. - -When a step is saved with a connection selected, the now-redundant inline credential -fields should be removed from the stored runner parameters. - -## Behavior - -On save, when `octopus_connection_id` is non-blank **and validation passes** (the -processor produces no `InvalidProperty` entries), remove these inline runner-parameter -keys from the properties map before it is stored: - -- `octopus_host` — the server URL (`OctopusConstants.getServerKey()`) -- `secure:octopus_apikey` — the API key (`OctopusConstants.getApiKey()`) -- `octopus_version` — the Octopus version (`OctopusConstants.getOctopusVersion()`) - -`octopus_space_name` is **not** removed. See "Why space is left alone" below. - -When `octopus_connection_id` is blank (the step uses inline fields), or when validation -fails, the map is left untouched. Leaving the map intact on a failed save means the edit -form is redisplayed with the user's entered values, rather than having fields silently -cleared underneath them. - -### Why this persists - -`jetbrains.buildServer.serverSide.PropertiesProcessor.process(Map)` may modify the -properties map, not only validate it. Per its Javadoc: *"Properties map passed as -argument can be verified or modified by the processor ... If processor was called during -saving properties to a configuration file, then modified map will be stored without -changes."* Removing keys inside `process()` is therefore the supported way to strip them -on save. - -### Why space is left alone - -`octopus_space_name` has deliberate override semantics: -`OctopusConnectionBuildStartProcessor` injects the connection's space **only if the step -did not set one** — a step's own space intentionally wins over the connection's. Blanking -the step space on save would destroy that feature. - -### Scope: UI save path only - -Stripping runs inside the runner `PropertiesProcessor`, which is the save path for the -web edit form. Steps created or updated by other means — the REST API, versioned settings -(Kotlin DSL / project-config XML in VCS), or config import — may not route through -`process()`, so stale inline credentials could persist there. This is a hygiene -limitation, not a correctness issue: whenever a connection is selected those leftover -inline values are ignored at build time (`OctopusConnectionBuildStartProcessor` overwrites -URL / API key / version from the connection). - -### Why space is left alone (continued) - -Additionally, `PropertiesProcessor.process(Map)` receives only the properties map — no -project context — so it cannot resolve the connection via `OctopusConnectionsManager` to -check whether the connection itself defines a space. A conditional "blank the step space -only when the connection has one" rule is not implementable at this layer. - -Leaving the step space untouched does the right thing in both cases: if the connection's -space is blank the step keeps its space (the only source); if the connection defines a -space the step's retained space still wins, matching documented behavior. - -## Design - -A small shared helper in the `octopus.teamcity.server.connection` package: - -```java -public final class ConnectionInlineFieldCleaner { - // Removes the inline URL, API key, and version keys when a connection is selected. - public static void stripInlineFieldsIfUsingConnection(Map properties); -} -``` - -Each connection-aware runner's anonymous `PropertiesProcessor.process(...)` runs its -existing validation first, then calls the helper only when validation produced no -errors: - -```java -public Collection process(@Nullable final Map properties) { - final Collection result = new ArrayList<>(); - if (properties == null) return result; - - final boolean usingConnection = - !StringUtil.isEmptyOrSpaces(properties.get(constants.getConnectionIdKey())); - if (!usingConnection) { - checkNotEmpty(properties, constants.getApiKey(), "API key must be specified", result); - checkNotEmpty(properties, constants.getServerKey(), "Server must be specified", result); - } - // ... remaining required-field checks unchanged ... - - if (result.isEmpty()) { - ConnectionInlineFieldCleaner.stripInlineFieldsIfUsingConnection(properties); - } - return result; -} -``` - -The helper is self-contained: it re-checks that `octopus_connection_id` is non-blank and -removes the three inline keys, so callers only need to guard on validation success. -Extracting it avoids copy-pasting the same three `remove()` calls into five anonymous -processors. - -### Applies to - -The five connection-aware runners: - -- `OctopusCreateReleaseRunType` -- `OctopusDeployReleaseRunType` -- `OctopusPromoteReleaseRunType` -- `OctopusPushPackageRunType` -- `OctopusBuildInformationRunType` - -`OctopusPackPackageRunType` has no connection handling (local packaging, no server -credentials) and is left untouched. - -### Interaction with existing validation - -Validation is unchanged and runs first; stripping happens only afterward, and only when -the result collection is empty. When a connection is selected the inline API-key / -server-URL required checks are already skipped, so a connection-backed step validates -cleanly and its inline keys are then stripped. The `octopus_apikey_source` / OIDC inline -keys are left as-is — at build start -`OctopusConnectionBuildStartProcessor` overwrites them from the connection, so any -leftover value is harmless. - -## Testing - -Unit-test the helper directly: - -- Connection id set → `octopus_host`, `secure:octopus_apikey`, `octopus_version` removed; - `octopus_space_name` and unrelated keys retained. -- Connection id blank → map unchanged. -- Connection id set but inline keys already absent → no error, map unchanged aside from - the (absent) keys. - -Because the "strip only when validation passes" guard lives in each runner's -`process()`, also cover it at that level (at least one runner): a connection-selected -step that fails a required-field check (e.g. missing project name) returns the -`InvalidProperty` and leaves the inline URL / API key / version intact. - -e2e coverage stays in its separate PR per the existing plan. From 95231f783801a1ecd4d7548ad4771149a63b3d59 Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Thu, 2 Jul 2026 15:33:33 +1000 Subject: [PATCH 7/7] Comment cleanup --- .../server/connection/ConnectionInlineFieldCleaner.java | 7 ++----- .../server/OctopusDeployReleaseRunTypeValidationTest.java | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java b/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java index e20b0b1c..2146c683 100644 --- a/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java +++ b/octopus-server/src/main/java/octopus/teamcity/server/connection/ConnectionInlineFieldCleaner.java @@ -22,11 +22,8 @@ /** * Removes a build step's inline Octopus credential parameters when the step references a reusable - * connection instead. The connection supplies the server URL, API key, and version at build start, - * so persisting the old inline values would only leave stale credentials behind the connection. - * - *

The space parameter is deliberately left untouched: a step's own space intentionally overrides - * the connection's, and this class has no project context with which to resolve the connection. + * connection. The connection supplies the server URL, API key, and version; keeping these old + * inline values leaves stale credentials behind. */ public final class ConnectionInlineFieldCleaner { private static final OctopusConstants CONSTANTS = new OctopusConstants(); diff --git a/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java b/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java index 512f1f9b..aee15130 100644 --- a/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java +++ b/octopus-server/src/test/java/octopus/teamcity/server/OctopusDeployReleaseRunTypeValidationTest.java @@ -75,7 +75,7 @@ void stripsInlineCredentialFieldsWhenConnectionSelectedAndValidationPasses() { @Test void retainsInlineCredentialFieldsWhenValidationFails() { - // Missing mandatory project name => validation fails, so nothing is stripped. + // Missing mandatory project name. Validation fails, so nothing should be stripped. final Map properties = new HashMap<>(); properties.put(CONSTANTS.getConnectionIdKey(), "PROJECT_EXT_1"); properties.put(CONSTANTS.getServerKey(), "https://octo");