Skip to content

Commit

Permalink
fix #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 27, 2023
1 parent b2de609 commit 188bd37
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 13 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,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 All @@ -41,7 +42,7 @@
#### _**Note**_: Breaking changes
* Fix #3972: deprecated Parameterizable and methods on Serialization accepting parameters - that was only needed as a workaround for non-string parameters. You should instead include those parameter values in the map passed to processLocally.
* Fix #3972: OpenShiftClient.load will no longer implicitly process templates. Use OpenShiftClient.templates().load instead.
* Fix #3972: WARNING: future client versions will not provide the static yaml and json ObjectMappersSerialization.
* Fix #3972: WARNING: future client versions will not provide the static yaml and json ObjectMappers on Serialization.
* Fix #4574: fromServer has been deprecated - it no longer needs to be called. All get() operations will fetch the resource(s) from the api server. If you need the context item that was passed in from a resource, load, or resourceList methods, use the item or items method.
* Fix #4633: client.run().withRunConfig was deprecated. Use withNewRunConfig instead.
* Fix #4663: Config.maxConcurrentRequests and Config.maxConcurrentRequestsPerHost will no longer be used. Instead they will default to unlimited for all clients. Due to the ability of the fabric8 client to start long running requests (either websocket or regular http) and how this is treated by the underlying clients you can easily exhaust these values and enter a state where the client is unresponsive without any additional information on what is occurring.
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 188bd37

Please sign in to comment.