Repository navigation
refactor: move the spaces lookup's credential resolution out of the controller - #267
Merged
Merged
Conversation
…ontroller The controller now handles only the request, permissions and JSON. SpacesLookupCredentials decides how to authenticate for each API key source. Two behaviour changes come with the move. A connection id that names a connection of another type is now refused. A blank or unknown API key source now uses the stored key, as builds do, instead of reporting OIDC. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
matt-richardson
commented
Oct 15, 2026
Comment on lines
-59
to
-67
| * <li>when a saved connection is named, its credentials are read server-side and the request's | ||
| * own {@code apiKey} is ignored, because TeamCity renders a stored secret as a placeholder | ||
| * and the browser therefore does not hold the real key; | ||
| * <li>a build configuration named by the request is used only when it belongs to that project, as | ||
| * a build of it would find the connection from there; | ||
| * <li>an API key held in a parameter is resolved only for a caller who also holds {@link | ||
| * Permission#RUN_BUILD} on the project, and each such lookup is logged. Its value may be a | ||
| * password the caller cannot read, but a caller who can edit and run a build can already send | ||
| * it anywhere from a build step; |
Contributor
Author
There was a problem hiding this comment.
Moved to the class doc of SpacesLookupCredentials.
Comment on lines
-174
to
-287
| /** | ||
| * Credentials for the lookup, always read from a saved connection. | ||
| * | ||
| * <p>The request names a connection; it never supplies a URL or key of its own. That is | ||
| * deliberate: it keeps the server from being asked to fetch an arbitrary caller-supplied address, | ||
| * so the only URLs it will ever call are ones an admin has already persisted into project | ||
| * configuration. It also sidesteps TeamCity rendering a saved secret as a placeholder, which | ||
| * means the browser could not supply the real key anyway. | ||
| * | ||
| * <p>The cost is that a brand-new connection has to be saved once before its spaces can be | ||
| * listed, and a step using inline credentials cannot list spaces at all. Both fall back to typing | ||
| * the space name, as does a connection that authenticates with OIDC. | ||
| */ | ||
| private Credentials resolveCredentials( | ||
| final HttpServletRequest request, final SProject project, final SUser user) | ||
| throws CredentialsUnavailableException { | ||
| final String connectionId = request.getParameter("connectionId"); | ||
| if (connectionId == null || connectionId.trim().isEmpty()) { | ||
| throw new CredentialsUnavailableException( | ||
| "Spaces can only be listed for a saved Octopus connection. Save this connection (or" | ||
| + " select one) and try again, or enter the space name instead."); | ||
| } | ||
|
|
||
| final Optional<OAuthConnectionDescriptor> connection = | ||
| connectionsManager.resolve(project, connectionId.trim()); | ||
| if (!connection.isPresent()) { | ||
| throw new CredentialsUnavailableException("That Octopus connection could not be resolved."); | ||
| } | ||
|
|
||
| final Map<String, String> params = connection.get().getParameters(); | ||
| final String serverUrl = params.get(ConnectionPropertyNames.SERVER_URL); | ||
| final String source = | ||
| params.getOrDefault( | ||
| ConnectionPropertyNames.API_KEY_SOURCE, ConnectionPropertyNames.API_KEY_SOURCE_KEY); | ||
| if (ConnectionPropertyNames.API_KEY_SOURCE_PARAMETER.equals(source)) { | ||
| final SBuildType buildType = projectBuildType(request, project); | ||
| if (!user.isPermissionGrantedForProject(project.getProjectId(), Permission.RUN_BUILD)) { | ||
| throw new CredentialsUnavailableException( | ||
| "Spaces can only be listed with an API key parameter by someone who can run builds" | ||
| + " here. Enter the space name instead."); | ||
| } | ||
| final String reference = apiKeyParameterReference(params); | ||
| final String apiKey = | ||
| resolveApiKeyParameter(reference, buildType == null ? project : buildType); | ||
| Loggers.SERVER.info( | ||
| "User " | ||
| + user.getUsername() | ||
| + " resolved the API key parameter " | ||
| + reference | ||
| + " of Octopus connection " | ||
| + connectionId.trim() | ||
| + " in " | ||
| + (buildType == null | ||
| ? "project " + project.getExternalId() | ||
| : "build configuration " + buildType.getExternalId()) | ||
| + " to list the spaces on " | ||
| + serverUrl); | ||
| return new Credentials(serverUrl, OctopusAuth.apiKey(apiKey)); | ||
| } | ||
| if (!ConnectionPropertyNames.API_KEY_SOURCE_KEY.equals(source)) { | ||
| throw new CredentialsUnavailableException( | ||
| "Spaces cannot be listed for a connection that authenticates with OIDC. Enter the space name instead." | ||
| + " name instead."); | ||
| } | ||
| return new Credentials( | ||
| serverUrl, OctopusAuth.apiKey(params.get(ConnectionPropertyNames.API_KEY))); | ||
| } | ||
|
|
||
| /** | ||
| * The step form's build configuration, or {@code null} on a form that has none. A missing one and | ||
| * one in another project get the same message, so the request cannot be used to find out which | ||
| * build configurations exist. | ||
| */ | ||
| @Nullable | ||
| private SBuildType projectBuildType(final HttpServletRequest request, final SProject project) | ||
| throws CredentialsUnavailableException { | ||
| final String buildTypeId = request.getParameter("buildTypeId"); | ||
| if (buildTypeId == null || buildTypeId.trim().isEmpty()) { | ||
| return null; | ||
| } | ||
| final SBuildType buildType = projectManager.findBuildTypeByExternalId(buildTypeId.trim()); | ||
| if (buildType == null || !project.getProjectId().equals(buildType.getProjectId())) { | ||
| throw new CredentialsUnavailableException( | ||
| "Build configuration '" + buildTypeId.trim() + "' was not found in this project."); | ||
| } | ||
| return buildType; | ||
| } | ||
|
|
||
| /** The connection's {@code %parameter%} reference, trimmed as a build injects it. */ | ||
| private static String apiKeyParameterReference(final Map<String, String> params) | ||
| throws CredentialsUnavailableException { | ||
| final String reference = params.get(ConnectionPropertyNames.API_KEY_PARAMETER); | ||
| if (reference == null || reference.trim().isEmpty()) { | ||
| throw new CredentialsUnavailableException( | ||
| "This connection does not use an API key parameter."); | ||
| } | ||
| return reference.trim(); | ||
| } | ||
|
|
||
| /** Resolves {@code reference} as a build of {@code scope} would. */ | ||
| private static String resolveApiKeyParameter( | ||
| final String reference, final ParametersSupport scope) | ||
| throws CredentialsUnavailableException { | ||
| final ProcessingResult resolved = scope.getValueResolver().resolve(reference); | ||
| final String apiKey = resolved.getResult(); | ||
| if (!resolved.isFullyResolved() || apiKey == null || apiKey.trim().isEmpty()) { | ||
| throw new CredentialsUnavailableException( | ||
| "The API key parameter " | ||
| + reference | ||
| + " can't be resolved here - it may only be set when a build runs. Enter the space" | ||
| + " name instead."); | ||
| } | ||
| return apiKey; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Moved to SpacesLookupCredentials.
Comment on lines
-319
to
-335
| private static final class Credentials { | ||
| private final String serverUrl; | ||
| private final OctopusAuth auth; | ||
|
|
||
| private Credentials(final String serverUrl, final OctopusAuth auth) { | ||
| this.serverUrl = serverUrl; | ||
| this.auth = auth; | ||
| } | ||
| } | ||
|
|
||
| private static final class CredentialsUnavailableException extends Exception { | ||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| private CredentialsUnavailableException(final String message) { | ||
| super(message); | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Moved to Credentials and CredentialsUnavailableException.
Comment on lines
-127
to
137
| void refusesToFetchWithoutASavedConnection() throws Exception { | ||
| // The request must never supply the address to fetch: the only URLs this endpoint will call are | ||
| // ones an admin already persisted into project configuration. | ||
| when(request.getParameter("serverUrl")).thenReturn("https://attacker.example.com"); | ||
| when(request.getParameter("apiKey")).thenReturn("API-TYPED"); | ||
| void returnsSpacesForASavedConnection() throws Exception { | ||
| storedKeyConnection(); | ||
|
|
||
| final String body = handle(); | ||
|
|
||
| assertThat(body).contains("saved Octopus connection"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| assertThat(body).contains("Spaces-123").contains("Example Space"); | ||
| assertThat(fetchedKeys).containsExactly("API-STORED"); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Moved to SpacesLookupCredentialsTest.refusesWithoutASavedConnection; explainsWhyCredentialsAreUnavailable covers the controller side.
Comment on lines
-195
to
321
| private Map<String, String> parameterSourcedConnection() { | ||
| final Map<String, String> params = new HashMap<>(); | ||
| params.put(ConnectionPropertyNames.SERVER_URL, "https://stored.example.com"); | ||
| params.put( | ||
| ConnectionPropertyNames.API_KEY_SOURCE, ConnectionPropertyNames.API_KEY_SOURCE_PARAMETER); | ||
| params.put(ConnectionPropertyNames.API_KEY_PARAMETER, "%octopus.apikey%"); | ||
| final OAuthConnectionDescriptor connection = connectionWith(params); | ||
| when(connectionsManager.resolve(eq(project), eq("octopus-conn-1"))) | ||
| .thenReturn(Optional.of(connection)); | ||
| when(request.getParameter("connectionId")).thenReturn("octopus-conn-1"); | ||
| when(user.isPermissionGrantedForProject(PROJECT_INTERNAL_ID, Permission.RUN_BUILD)) | ||
| .thenReturn(true); | ||
| return params; | ||
| } | ||
|
|
||
| private static ValueResolver resolverReturning(final String value, final boolean resolved) { | ||
| final ProcessingResult result = mock(ProcessingResult.class); | ||
| when(result.getResult()).thenReturn(value); | ||
| when(result.isFullyResolved()).thenReturn(resolved); | ||
| final ValueResolver resolver = mock(ValueResolver.class); | ||
| when(resolver.resolve("%octopus.apikey%")).thenReturn(result); | ||
| return resolver; | ||
| } | ||
|
|
||
| private SBuildType buildTypeInProject(final String projectInternalId) { | ||
| final SBuildType buildType = mock(SBuildType.class); | ||
| when(buildType.getProjectId()).thenReturn(projectInternalId); | ||
| when(buildType.getExternalId()).thenReturn("MyProject_Deploy"); | ||
| when(projectManager.findBuildTypeByExternalId("MyProject_Deploy")).thenReturn(buildType); | ||
| when(request.getParameter("buildTypeId")).thenReturn("MyProject_Deploy"); | ||
| return buildType; | ||
| } | ||
|
|
||
| @Test | ||
| void resolvesAParameterSourcedKeyAgainstTheBuildConfiguration() throws Exception { | ||
| parameterSourcedConnection(); | ||
| final SBuildType buildType = buildTypeInProject(PROJECT_INTERNAL_ID); | ||
| final ValueResolver resolver = resolverReturning("API-FROM-PARAMETER", true); | ||
| when(buildType.getValueResolver()).thenReturn(resolver); | ||
|
|
||
| void explainsWhyCredentialsAreUnavailable() throws Exception { | ||
| final String body = handle(); | ||
|
|
||
| assertThat(body).contains("Spaces-123"); | ||
| assertThat(fetchedKeys).containsExactly("API-FROM-PARAMETER"); | ||
| assertThat(body).doesNotContain("API-FROM-PARAMETER"); | ||
| } | ||
|
|
||
| @Test | ||
| void resolvesAParameterSourcedKeyAgainstTheProjectWithoutABuildConfiguration() throws Exception { | ||
| parameterSourcedConnection(); | ||
| final ValueResolver resolver = resolverReturning("API-FROM-PROJECT", true); | ||
| when(project.getValueResolver()).thenReturn(resolver); | ||
|
|
||
| handle(); | ||
|
|
||
| assertThat(fetchedKeys).containsExactly("API-FROM-PROJECT"); | ||
| } | ||
|
|
||
| @Test | ||
| void explainsAParameterThatOnlyResolvesDuringABuild() throws Exception { | ||
| parameterSourcedConnection(); | ||
| final ValueResolver resolver = resolverReturning("%octopus.apikey%", false); | ||
| when(project.getValueResolver()).thenReturn(resolver); | ||
|
|
||
| final String body = handle(); | ||
|
|
||
| assertThat(body).contains("%octopus.apikey%").contains("space name instead"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void refusesABuildConfigurationFromAnotherProject() throws Exception { | ||
| parameterSourcedConnection(); | ||
| final SBuildType buildType = buildTypeInProject("project999"); | ||
| when(user.isPermissionGrantedForProject("project999", Permission.EDIT_PROJECT)) | ||
| .thenReturn(true); | ||
| when(user.isPermissionGrantedForProject("project999", Permission.RUN_BUILD)).thenReturn(true); | ||
| final ValueResolver resolver = resolverReturning("API-FROM-PARAMETER", true); | ||
| when(buildType.getValueResolver()).thenReturn(resolver); | ||
|
|
||
| assertThat(handle()).contains("MyProject_Deploy").contains("was not found in this project"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void refusesAParameterSourcedKeyToAUserWhoCannotRunBuilds() throws Exception { | ||
| parameterSourcedConnection(); | ||
| when(user.isPermissionGrantedForProject(PROJECT_INTERNAL_ID, Permission.RUN_BUILD)) | ||
| .thenReturn(false); | ||
| final ValueResolver resolver = resolverReturning("API-FROM-PROJECT", true); | ||
| when(project.getValueResolver()).thenReturn(resolver); | ||
|
|
||
| assertThat(handle()).contains("can run builds").contains("space name instead"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void refusesAnUnknownBuildConfiguration() throws Exception { | ||
| parameterSourcedConnection(); | ||
| when(request.getParameter("buildTypeId")).thenReturn("Nope"); | ||
|
|
||
| assertThat(handle()).contains("Nope").contains("was not found"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void explainsThatAnOidcConnectionCannotListSpaces() throws Exception { | ||
| final Map<String, String> params = new HashMap<>(); | ||
| params.put(ConnectionPropertyNames.SERVER_URL, "https://stored.example.com"); | ||
| params.put(ConnectionPropertyNames.API_KEY_SOURCE, ConnectionPropertyNames.API_KEY_SOURCE_OIDC); | ||
| final OAuthConnectionDescriptor connection = connectionWith(params); | ||
| when(connectionsManager.resolve(eq(project), eq("octopus-conn-1"))) | ||
| .thenReturn(Optional.of(connection)); | ||
| when(request.getParameter("connectionId")).thenReturn("octopus-conn-1"); | ||
|
|
||
| assertThat(handle()).contains("authenticates with OIDC").contains("space name instead"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| void reportsAnUnresolvableConnection() throws Exception { | ||
| when(connectionsManager.resolve(eq(project), any())).thenReturn(Optional.empty()); | ||
| when(request.getParameter("connectionId")).thenReturn("gone"); | ||
|
|
||
| assertThat(handle()).contains("could not be resolved"); | ||
| verify(response).setStatus(HttpServletResponse.SC_OK); | ||
| assertThat(body).contains("saved Octopus connection"); | ||
| assertThat(fetchedUrls).isEmpty(); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Moved to SpacesLookupCredentialsTest.
APErebus
approved these changes
Oct 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of DPT-3630 (Renaming an Octopus space breaks TeamCity configurations that name it). Split out of #265 (feat: list spaces for a connection that authenticates with OIDC) to keep that PR to the OIDC work.
Why
The spaces controller handled the request and also decided how to authenticate for each API key source. #265 adds a third source, OIDC, which would have made the controller twice the size. The credential logic now lives in its own class; the controller keeps the request, permissions and JSON.
Notes
main, which reads "Enter the space name instead. name instead." after a review suggestion on feat: list spaces for a connection whose API key is held in a parameter #264.🤖 Generated with Claude Code