fix(retrofit2): fix multiple slash issue in baseUrl of retrofit2 client (#1206) (#1207)

* test(github): add a test to demonstrate an error scenario where if the baseUrl of the Github client has multiple slashes and the end point start with a slash

* fix(github): fix the error where if the baseUrl of the Github client has multiple slashes and the end point start with a slash, the resultant api url leaves that part the base url coming after first slash.

* refactor(api): refactor retrofit2 client configuration for FiatService to fix any issues that may occur if the base url has multiple slashes

* refactor(web): refactor retrofit2 client configuration for IgorApi, ClouddriverApi and Front50Api to fix any issues that may occur if the base url has multiple slashes

(cherry picked from commit a7f7f9d)

Co-authored-by: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com>

Author: mergify[bot]

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java

@@ -22,6 +22,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spinnaker.config.ErrorConfiguration;
 import com.netflix.spinnaker.config.OkHttp3ClientConfiguration;
+import com.netflix.spinnaker.fiat.util.RetrofitUtils;
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
 import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator;
 import lombok.val;
@@ -61,7 +62,7 @@ public FiatService fiatService(
     objectMapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
 
     return new Retrofit.Builder()
-        .baseUrl(fiatConfigurationProperties.getBaseUrl())
+        .baseUrl(RetrofitUtils.getBaseUrl(fiatConfigurationProperties.getBaseUrl()))
         .client(okHttpClientConfig.createForRetrofit2().build())
         .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
         .addConverterFactory(JacksonConverterFactory.create(objectMapper))

fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java

@@ -33,7 +33,7 @@ public interface FiatService {
    * @param userId The username of the user
    * @return The full UserPermission of the user.
    */
-  @GET("/authorize/{userId}")
+  @GET("authorize/{userId}")
   Call<UserPermission.View> getUserPermission(@Path("userId") String userId);
 
   /**
@@ -43,7 +43,7 @@ public interface FiatService {
    * @param authorization The authorization in question (read, write, etc)
    * @return True if the user has access to the specified resource.
    */
-  @GET("/authorize/{userId}/{resourceType}/{resourceName}/{authorization}")
+  @GET("authorize/{userId}/{resourceType}/{resourceName}/{authorization}")
   Call<Void> hasAuthorization(
       @Path("userId") String userId,
       @Path("resourceType") String resourceType,
@@ -58,7 +58,7 @@ Call<Void> hasAuthorization(
    * @param resourceType The type of the resource
    * @param resource The resource to check
    */
-  @POST("/authorize/{userId}/{resourceType}/create")
+  @POST("authorize/{userId}/{resourceType}/create")
   Call<Void> canCreate(
       @Path("userId") String userId,
       @Path("resourceType") String resourceType,
@@ -69,7 +69,7 @@ Call<Void> canCreate(
    *
    * @return The number of non-anonymous users synced.
    */
-  @POST("/roles/sync")
+  @POST("roles/sync")
   Call<Long> sync();
 
   /**
@@ -78,7 +78,7 @@ Call<Void> canCreate(
    * @param roles Users with any role listed should be updated.
    * @return The number of non-anonymous users synced.
    */
-  @POST("/roles/sync")
+  @POST("roles/sync")
   Call<Long> sync(@Body List<String> roles);
 
   /**
@@ -89,15 +89,15 @@ Call<Void> canCreate(
    * @param roles The roles allowed for this service account.
    * @return The number of non-anonymous users synced.
    */
-  @POST("/roles/sync/serviceAccount/{serviceAccountId}")
+  @POST("roles/sync/serviceAccount/{serviceAccountId}")
   Call<Long> syncServiceAccount(
       @Path("serviceAccountId") String serviceAccountId, @Body List<String> roles);
 
   /**
    * @param userId The user being logged in
    * @return ignored.
    */
-  @POST("/roles/{userId}")
+  @POST("roles/{userId}")
   Call<Void> loginUser(@Path("userId") String userId);
 
   /**
@@ -107,13 +107,13 @@ Call<Long> syncServiceAccount(
    * @param roles Collection of roles from the identity provider
    * @return ignored.
    */
-  @PUT("/roles/{userId}")
+  @PUT("roles/{userId}")
   Call<Void> loginWithRoles(@Path("userId") String userId, @Body Collection<String> roles);
 
   /**
    * @param userId The user being logged out
    * @return ignored.
    */
-  @DELETE("/roles/{userId}")
+  @DELETE("roles/{userId}")
   Call<Void> logoutUser(@Path("userId") String userId);
 }

fiat-core/fiat-core.gradle

@@ -4,6 +4,7 @@ dependencies {
 
   implementation "com.fasterxml.jackson.core:jackson-annotations"
   implementation "com.google.code.findbugs:jsr305"
+  implementation "io.spinnaker.kork:kork-retrofit"
   implementation "org.slf4j:slf4j-api"
   implementation "org.springframework:spring-core"
   implementation "org.springframework.boot:spring-boot-starter-web"

fiat-core/src/main/java/com/netflix/spinnaker/fiat/util/RetrofitUtils.java

@@ -0,0 +1,42 @@
+/*
+ * Copyright 2025 OpsMx, Inc.
+ *
+ * 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 com.netflix.spinnaker.fiat.util;
+
+import okhttp3.HttpUrl;
+
+public class RetrofitUtils {
+
+  /**
+   * Converts a given URL to a valid base URL for use in a {@link retrofit2.Retrofit} instance. If
+   * the URL is invalid, an {@link IllegalArgumentException} is thrown. If the URL does not end with
+   * a slash, a slash is appended to the end of the URL.
+   *
+   * @param suppliedBaseUrl the URL to convert
+   * @return a valid base URL for use in a Retrofit instance
+   */
+  public static String getBaseUrl(String suppliedBaseUrl) {
+    HttpUrl parsedUrl = HttpUrl.parse(suppliedBaseUrl);
+    if (parsedUrl == null) {
+      throw new IllegalArgumentException("Invalid URL: " + suppliedBaseUrl);
+    }
+    String baseUrl = parsedUrl.newBuilder().build().toString();
+    if (!baseUrl.endsWith("/")) {
+      baseUrl += "/";
+    }
+    return baseUrl;
+  }
+}

fiat-github/fiat-github.gradle

@@ -9,4 +9,6 @@ dependencies {
   implementation "io.spinnaker.kork:kork-web"
   implementation "io.spinnaker.kork:kork-retrofit"
   implementation "javax.validation:validation-api"
+
+  testImplementation "com.github.tomakehurst:wiremock-jre8-standalone"
 }

fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java

@@ -3,6 +3,7 @@
 import com.netflix.spinnaker.config.OkHttp3ClientConfiguration;
 import com.netflix.spinnaker.fiat.roles.github.GitHubProperties;
 import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient;
+import com.netflix.spinnaker.fiat.util.RetrofitUtils;
 import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
 import java.io.IOException;
 import lombok.Setter;
@@ -35,7 +36,7 @@ public GitHubClient gitHubClient(OkHttp3ClientConfiguration okHttpClientConfig)
         new BasicAuthRequestInterceptor().setAccessToken(gitHubProperties.getAccessToken());
 
     return new Retrofit.Builder()
-        .baseUrl(gitHubProperties.getBaseUrl())
+        .baseUrl(RetrofitUtils.getBaseUrl(gitHubProperties.getBaseUrl()))
         .client(okHttpClientConfig.createForRetrofit2().addInterceptor(interceptor).build())
         .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
         .addConverterFactory(JacksonConverterFactory.create())

fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java

@@ -27,15 +27,15 @@
 /** Retrofit interface for interacting with a GitHub REST API. */
 public interface GitHubClient {
 
-  @GET("/orgs/{org}/teams")
+  @GET("orgs/{org}/teams")
   Call<List<Team>> getOrgTeams(
       @Path("org") String org, @Query("page") int page, @Query("per_page") int paginationValue);
 
-  @GET("/orgs/{org}/members")
+  @GET("orgs/{org}/members")
   Call<List<Member>> getOrgMembers(
       @Path("org") String org, @Query("page") int page, @Query("per_page") int paginationValue);
 
-  @GET("/orgs/{org}/teams/{teamSlug}/members")
+  @GET("orgs/{org}/teams/{teamSlug}/members")
   Call<List<Member>> getMembersOfTeam(
       @Path("org") String org,
       @Path("teamSlug") String teamSlug,

fiat-github/src/test/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProviderTest.java

@@ -0,0 +1,81 @@
+/*
+ * Copyright 2025 OpsMx, Inc.
+ *
+ * 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 com.netflix.spinnaker.fiat.roles.github;
+
+import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
+import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import com.github.tomakehurst.wiremock.client.WireMock;
+import com.github.tomakehurst.wiremock.junit5.WireMockExtension;
+import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient;
+import com.netflix.spinnaker.fiat.roles.github.model.Member;
+import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
+import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
+import java.util.List;
+import okhttp3.OkHttpClient;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import retrofit2.Retrofit;
+import retrofit2.converter.jackson.JacksonConverterFactory;
+
+public class GithubTeamsUserRolesProviderTest {
+  @RegisterExtension
+  static WireMockExtension wmGithub =
+      WireMockExtension.newInstance().options(wireMockConfig().dynamicPort()).build();
+
+  GitHubClient gitHubClient;
+  int port;
+  String baseUrl = "http://localhost:PORT/api/v3/";
+
+  @BeforeEach
+  void setUp() {
+
+    port = wmGithub.getPort();
+
+    baseUrl = baseUrl.replaceFirst("PORT", String.valueOf(port));
+
+    gitHubClient =
+        new Retrofit.Builder()
+            .baseUrl(baseUrl)
+            .client(new OkHttpClient())
+            .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
+            .addConverterFactory(JacksonConverterFactory.create())
+            .build()
+            .create(GitHubClient.class);
+  }
+
+  @Test
+  void testBaseUrlWithMultipleSlashes() {
+    wmGithub.stubFor(
+        WireMock.get(urlEqualTo("/api/v3/orgs/org1/members?page=1&per_page=2"))
+            .willReturn(
+                aResponse()
+                    .withStatus(200)
+                    .withBody(
+                        "[{\"login\": \"foo\",\"id\": 18634546},{\"login\": \"bar\",\"id\": 202758929}]")));
+
+    List<Member> members = Retrofit2SyncCall.execute(gitHubClient.getOrgMembers("org1", 1, 2));
+
+    wmGithub.verify(
+        1, WireMock.getRequestedFor(urlEqualTo("/api/v3/orgs/org1/members?page=1&per_page=2")));
+
+    assertThat(members.size()).isEqualTo(2);
+  }
+}

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApi.java

@@ -23,9 +23,9 @@
 import retrofit2.http.GET;
 
 public interface ClouddriverApi {
-  @GET("/credentials")
+  @GET("credentials")
   Call<List<Account>> getAccounts();
 
-  @GET("/applications?restricted=false&expand=false")
+  @GET("applications?restricted=false&expand=false")
   Call<List<Application>> getApplications();
 }

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50Api.java

@@ -24,20 +24,20 @@
 import retrofit2.http.Query;
 
 public interface Front50Api {
-  @GET("/permissions/applications")
+  @GET("permissions/applications")
   Call<List<Application>> getAllApplicationPermissions();
 
   /**
    * @deprecated for fiat's usage this is always going to be called with restricted = false, use the
    *     no arg method instead which has the same behavior.
    */
-  @GET("/v2/applications")
+  @GET("v2/applications")
   @Deprecated
   Call<List<Application>> getAllApplications(@Query("restricted") boolean restricted);
 
-  @GET("/v2/applications?restricted=false")
+  @GET("v2/applications?restricted=false")
   Call<List<Application>> getAllApplications();
 
-  @GET("/serviceAccounts")
+  @GET("serviceAccounts")
   Call<List<ServiceAccount>> getAllServiceAccounts();
 }