Skip to content

Commit

Permalink
fix fabric8io#4698: adding tests, addressing smells, and adding docs
Browse files Browse the repository at this point in the history
  • Loading branch information
shawkins authored and manusa committed Jan 17, 2023
1 parent 5f3b68a commit 2f291a0
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 12 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* Fix #4654: Fix GatewayClass to not implement Namespaced interface
* Fix #4670: the initial informer listing will use a resourceVersion of 0 to utilize the watch cache if possible. This means that the initial cache state when the informer is returned, or the start future is completed, may not be as fresh as the previous behavior which forced the latest version. It will of course become more consistent as the watch will already have been established.
* Fix #4694: [java-generator] Option to override the package name of the generated code.
* Fix #4698: changes were made to improve authentication logic. If a username and password are specified and you are using a base KuberentesClient, then that will always be used as a basic auth header value. If a username and password are specified and you are using an OpenShiftClient, then a token will still be used if present, but upon an auth failure the username and password will be used to obtain a fresh token. If a new token is obtained it will be saved in the kubeconfig if one were used to create the Config.
* Fix #4720: interceptors close any response body if the response is not a 2xx response.
* Fix #4734: @KubernetesTest annotation can be used in base test classes
* Fix #4734: @KubernetesTest creates an ephemeral Namespace optionally (can opt-out)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ public static CompletableFuture<String> resolveOIDCTokenFromAuthConfig(Config cu
String clientSecret = currentAuthProviderConfig.getOrDefault(CLIENT_SECRET_KUBECONFIG, "");
String idpCert = currentAuthProviderConfig.get(IDP_CERT_DATA);
if (isTokenRefreshSupported(currentAuthProviderConfig)) {
return getOIDCProviderTokenEndpointAndRefreshToken(issuer, clientId, refreshToken, clientSecret, accessToken, idpCert,
clientBuilder).thenApply(map -> {
return getOIDCProviderTokenEndpointAndRefreshToken(issuer, clientId, refreshToken, clientSecret, idpCert, clientBuilder)
.thenApply(map -> {
Object token = map.get(ID_TOKEN_PARAM);
if (token == null) {
LOGGER.warn("token response did not contain an id_token, either the scope \\\"openid\\\" wasn't " +
Expand Down Expand Up @@ -307,8 +307,7 @@ private static Map<String, String> getRequestBodyContentForRefresh(String client
}

private static CompletableFuture<Map<String, Object>> getOIDCProviderTokenEndpointAndRefreshToken(String issuer,
String clientId, String refreshToken, String clientSecret, String accessToken, String idpCert,
HttpClient.Builder clientBuilder) {
String clientId, String refreshToken, String clientSecret, String idpCert, HttpClient.Builder clientBuilder) {
HttpClient newClient = getDefaultHttpClientWithPemCert(idpCert, clientBuilder);
CompletableFuture<Map<String, Object>> result = getOIDCDiscoveryDocumentAsMap(newClient, issuer)
.thenCompose(wellKnownOpenIdConfiguration -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
*/
public class TokenRefreshInterceptor implements Interceptor {

public static final String AUTHORIZATION = "Authorization";

public static final String NAME = "TOKEN";

private final Config config;
Expand All @@ -56,11 +58,11 @@ public Interceptor withConfig(Config config) {
@Override
public void before(BasicBuilder headerBuilder, HttpHeaders headers) {
if (isBasicAuth()) {
headerBuilder.header("Authorization", HttpClientUtils.basicCredentials(config.getUsername(), config.getPassword()));
headerBuilder.header(AUTHORIZATION, HttpClientUtils.basicCredentials(config.getUsername(), config.getPassword()));
return;
}
if (Utils.isNotNullOrEmpty(config.getOauthToken())) {
headerBuilder.header("Authorization", "Bearer " + config.getOauthToken());
headerBuilder.header(AUTHORIZATION, "Bearer " + config.getOauthToken());
}
if (isTimeToRefresh()) {
refreshToken(headerBuilder);
Expand Down Expand Up @@ -104,7 +106,7 @@ private CompletableFuture<String> extractNewAccessTokenFrom(Config newestConfig)

private boolean overrideNewAccessTokenToConfig(String newAccessToken, BasicBuilder headerBuilder, Config existConfig) {
if (Utils.isNotNullOrEmpty(newAccessToken)) {
headerBuilder.setHeader("Authorization", "Bearer " + newAccessToken);
headerBuilder.setHeader(AUTHORIZATION, "Bearer " + newAccessToken);
existConfig.setOauthToken(newAccessToken);

updateLatestRefreshTimestamp();
Expand Down
9 changes: 8 additions & 1 deletion openshift-client/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,14 @@
<type>test-jar</type>
<scope>test</scope>
</dependency>

<dependency>
<groupId>io.fabric8</groupId>
<artifactId>kubernetes-client-api</artifactId>
<type>test-jar</type>
<scope>test</scope>
</dependency>


<dependency>
<groupId>io.fabric8</groupId>
Expand All @@ -110,7 +118,6 @@
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
<version>${slf4j.version}</version>
<scope>test</scope>
</dependency>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,7 @@ public CompletableFuture<Boolean> afterFailure(Builder builder, HttpResponse<?>
try {
// TODO: we may need some protection here or in the persistKubeConfigWithUpdatedAuthInfo
// if the user has modified the username via the requestconfig are we writing a valid value?
OpenIDConnectionUtils
.persistKubeConfigWithUpdatedAuthInfo(config, a -> {
a.setToken(t);
});
OpenIDConnectionUtils.persistKubeConfigWithUpdatedAuthInfo(config, a -> a.setToken(t));
} catch (IOException e) {
LOGGER.warn("failure while persisting new token into KUBECONFIG", e);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/**
* Copyright (C) 2015 Red Hat, 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 io.fabric8.openshift.client.internal;

import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.http.HttpClient;
import io.fabric8.kubernetes.client.http.StandardHttpHeaders;
import io.fabric8.kubernetes.client.http.StandardHttpRequest;
import io.fabric8.kubernetes.client.http.TestHttpResponse;
import io.fabric8.kubernetes.client.utils.TokenRefreshInterceptor;
import org.junit.jupiter.api.Test;
import org.mockito.Mockito;

import java.net.HttpURLConnection;
import java.net.URI;
import java.util.Collections;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;

class OpenShiftOAuthInterceptorTest {

@Test
void testBasicAuthNotUsed() {
Config config = Config.empty();
config.setUsername("user");
config.setPassword("pass");

HttpClient client = Mockito.mock(HttpClient.class);

OpenShiftOAuthInterceptor interceptor = new OpenShiftOAuthInterceptor(client, config);

StandardHttpRequest.Builder builder = new StandardHttpRequest.Builder();
builder.uri("http://localhost");

interceptor.before(builder, new StandardHttpHeaders());

assertTrue(builder.build().headers(TokenRefreshInterceptor.AUTHORIZATION).isEmpty());
}

@Test
void testTokenUsed() {
Config config = Config.empty();
config.setUsername("user");
config.setPassword("pass");
config.setOauthToken("token");

HttpClient client = Mockito.mock(HttpClient.class);

OpenShiftOAuthInterceptor interceptor = new OpenShiftOAuthInterceptor(client, config);

StandardHttpRequest.Builder builder = new StandardHttpRequest.Builder();
builder.uri("http://localhost");

interceptor.before(builder, new StandardHttpHeaders());

assertEquals(Collections.singletonList("Bearer token"), builder.build().headers(TokenRefreshInterceptor.AUTHORIZATION));
}

@Test
void testTokenRefreshedFromConfig() {
Config config = Mockito.mock(Config.class, RETURNS_DEEP_STUBS);
Mockito.when(config.refresh().getOauthToken()).thenReturn("token");

HttpClient client = Mockito.mock(HttpClient.class);

OpenShiftOAuthInterceptor interceptor = new OpenShiftOAuthInterceptor(client, config);

StandardHttpRequest.Builder builder = new StandardHttpRequest.Builder();
builder.uri("http://localhost");

interceptor.afterFailure(builder, TestHttpResponse.from(HttpURLConnection.HTTP_UNAUTHORIZED, "not for you")
.withRequest(new StandardHttpRequest(null, URI.create("http://localhost"), "GET", null)));

assertEquals(Collections.singletonList("Bearer token"), builder.build().headers(TokenRefreshInterceptor.AUTHORIZATION));
Mockito.verify(config).setOauthToken("token");
}

}

0 comments on commit 2f291a0

Please sign in to comment.