Skip to content

Commit 08dc983

Browse files
at055612at055612
authored
Merge pull request #4848 from gchq/gh-4837-oidc-jersey-client
PR for #4837 - OIDC config GET uses httpClient, but token calls use jerseyClient
2 parents 74aa7e5 + e6c4702 commit 08dc983

File tree

7 files changed

+64
-84
lines changed

7 files changed

+64
-84
lines changed

‎stroom-config/stroom-config-app/src/test/resources/stroom/config/app/expected.yaml

-1
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,6 @@ appConfig:
794794
clientSecret: null
795795
expectedSignerPrefixes: []
796796
formTokenRequest: true
797-
httpClient: null
798797
identityProviderType: "INTERNAL_IDP"
799798
issuer: null
800799
jwksUri: null

‎stroom-proxy/stroom-proxy-app/src/main/java/stroom/proxy/app/ProxyOpenIdConfig.java

+3-7
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import stroom.security.openid.api.AbstractOpenIdConfig;
44
import stroom.security.openid.api.IdpType;
5-
import stroom.util.http.HttpClientConfiguration;
65
import stroom.util.shared.IsProxyConfig;
76

87
import com.fasterxml.jackson.annotation.JsonCreator;
@@ -42,8 +41,7 @@ public ProxyOpenIdConfig(
4241
@JsonProperty("validIssuers") final Set<String> validIssuers,
4342
@JsonProperty("uniqueIdentityClaim") final String uniqueIdentityClaim,
4443
@JsonProperty("userDisplayNameClaim") final String userDisplayNameClaim,
45-
@JsonProperty(PROP_NAME_EXPECTED_SIGNER_PREFIXES) final Set<String> expectedSignerPrefixes,
46-
@JsonProperty("httpClient") final HttpClientConfiguration httpClient) {
44+
@JsonProperty(PROP_NAME_EXPECTED_SIGNER_PREFIXES) final Set<String> expectedSignerPrefixes) {
4745

4846
super(identityProviderType,
4947
openIdConfigurationEndpoint,
@@ -62,8 +60,7 @@ public ProxyOpenIdConfig(
6260
validIssuers,
6361
uniqueIdentityClaim,
6462
userDisplayNameClaim,
65-
expectedSignerPrefixes,
66-
httpClient);
63+
expectedSignerPrefixes);
6764
}
6865

6966
@JsonIgnore
@@ -113,7 +110,6 @@ public ProxyOpenIdConfig withIdentityProviderType(final IdpType identityProvider
113110
getValidIssuers(),
114111
getUniqueIdentityClaim(),
115112
getUserDisplayNameClaim(),
116-
getExpectedSignerPrefixes(),
117-
getHttpClient());
113+
getExpectedSignerPrefixes());
118114
}
119115
}

‎stroom-proxy/stroom-proxy-app/src/test/resources/stroom/dist/proxy-expected.yaml

-1
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,6 @@ proxyConfig:
119119
clientSecret: null
120120
expectedSignerPrefixes: []
121121
formTokenRequest: true
122-
httpClient: null
123122
identityProviderType: "NO_IDP"
124123
issuer: null
125124
jwksUri: null

‎stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/ExternalIdpConfigurationProvider.java

+31-50
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,8 @@
88
import stroom.security.openid.api.OpenIdConfigurationResponse.Builder;
99
import stroom.util.HasHealthCheck;
1010
import stroom.util.NullSafe;
11-
import stroom.util.http.HttpClientConfiguration;
12-
import stroom.util.http.HttpClientFactory;
13-
import stroom.util.io.StreamUtil;
11+
import stroom.util.jersey.JerseyClientFactory;
12+
import stroom.util.jersey.JerseyClientName;
1413
import stroom.util.logging.LambdaLogger;
1514
import stroom.util.logging.LambdaLoggerFactory;
1615
import stroom.util.logging.LogUtil;
@@ -23,20 +22,16 @@
2322
import jakarta.inject.Provider;
2423
import jakarta.inject.Singleton;
2524
import jakarta.servlet.http.HttpServletResponse;
26-
import org.apache.hc.client5.http.classic.HttpClient;
27-
import org.apache.hc.client5.http.classic.methods.HttpGet;
28-
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
29-
import org.apache.hc.core5.http.HttpEntity;
30-
import org.apache.hc.core5.io.CloseMode;
25+
import jakarta.ws.rs.client.WebTarget;
26+
import jakarta.ws.rs.core.MediaType;
27+
import jakarta.ws.rs.core.Response;
3128

3229
import java.io.IOException;
33-
import java.io.InputStream;
3430
import java.time.Duration;
3531
import java.util.HashSet;
3632
import java.util.List;
3733
import java.util.Objects;
3834
import java.util.Set;
39-
import java.util.UUID;
4035
import java.util.function.BiConsumer;
4136
import java.util.function.Consumer;
4237
import java.util.function.Supplier;
@@ -52,22 +47,20 @@ public class ExternalIdpConfigurationProvider
5247
private static final LambdaLogger LOGGER = LambdaLoggerFactory.getLogger(ExternalIdpConfigurationProvider.class);
5348
private static final long MAX_SLEEP_TIME_MS = 30_000;
5449

55-
private final HttpClientFactory httpClientFactory;
50+
private final JerseyClientFactory jerseyClientFactory;
5651
// Important to use AbstractOpenIdConfig here and not OpenIdConfiguration as the former
5752
// is bound to the yaml config, the latter is what we are presenting this as.
5853
private final Provider<AbstractOpenIdConfig> localOpenIdConfigProvider;
5954

6055
private volatile String lastConfigurationEndpoint;
6156
private volatile OpenIdConfigurationResponse openIdConfigurationResp;
62-
private volatile CloseableHttpClient currentHttpClient;
63-
private volatile HttpClientConfiguration currentHttpClientConfiguration;
6457

6558
@Inject
6659
public ExternalIdpConfigurationProvider(
67-
final HttpClientFactory httpClientFactory,
68-
final Provider<AbstractOpenIdConfig> localOpenIdConfigProvider) {
69-
this.httpClientFactory = httpClientFactory;
60+
final Provider<AbstractOpenIdConfig> localOpenIdConfigProvider,
61+
final JerseyClientFactory jerseyClientFactory) {
7062
this.localOpenIdConfigProvider = localOpenIdConfigProvider;
63+
this.jerseyClientFactory = jerseyClientFactory;
7164
}
7265

7366
@Override
@@ -76,27 +69,12 @@ public OpenIdConfigurationResponse getConfigurationResponse() {
7669
final String configurationEndpoint = abstractOpenIdConfig.getOpenIdConfigurationEndpoint();
7770

7871
if (isFetchRequired(configurationEndpoint)) {
79-
updateOpenIdConfigurationResponse(abstractOpenIdConfig, getHttpClient(abstractOpenIdConfig));
72+
updateOpenIdConfigurationResponse(abstractOpenIdConfig);
8073
}
8174

8275
return openIdConfigurationResp;
8376
}
8477

85-
private synchronized HttpClient getHttpClient(final AbstractOpenIdConfig abstractOpenIdConfig) {
86-
if (currentHttpClient == null ||
87-
!Objects.equals(abstractOpenIdConfig.getHttpClient(), currentHttpClientConfiguration)) {
88-
if (currentHttpClient != null) {
89-
currentHttpClient.close(CloseMode.IMMEDIATE);
90-
}
91-
92-
currentHttpClientConfiguration = abstractOpenIdConfig.getHttpClient();
93-
currentHttpClient = httpClientFactory.get(
94-
"ExternalIdpConfigurationProvider-" + UUID.randomUUID(),
95-
currentHttpClientConfiguration);
96-
}
97-
return currentHttpClient;
98-
}
99-
10078
@Override
10179
public HealthCheck.Result getHealth() {
10280
final HealthCheck.ResultBuilder resultBuilder = HealthCheck.Result.builder();
@@ -124,7 +102,7 @@ public HealthCheck.Result getHealth() {
124102
try {
125103
resultBuilder.withDetail("configUri", configurationEndpoint);
126104
OpenIdConfigurationResponse response = fetchOpenIdConfigurationResponse(
127-
configurationEndpoint, abstractOpenIdConfig, getHttpClient(abstractOpenIdConfig));
105+
configurationEndpoint, abstractOpenIdConfig);
128106
if (response != null) {
129107
resultBuilder.healthy();
130108
} else {
@@ -147,8 +125,7 @@ private boolean isFetchRequired(final String configurationEndpoint) {
147125
}
148126

149127
private OpenIdConfigurationResponse updateOpenIdConfigurationResponse(
150-
final AbstractOpenIdConfig abstractOpenIdConfig,
151-
final HttpClient httpClient) {
128+
final AbstractOpenIdConfig abstractOpenIdConfig) {
152129
final String configurationEndpoint = abstractOpenIdConfig.getOpenIdConfigurationEndpoint();
153130

154131
LOGGER.debug("About to get lock to update open id configuration");
@@ -158,7 +135,7 @@ private OpenIdConfigurationResponse updateOpenIdConfigurationResponse(
158135
if (isFetchRequired(configurationEndpoint)) {
159136
try {
160137
final OpenIdConfigurationResponse response = fetchOpenIdConfigurationResponse(
161-
configurationEndpoint, abstractOpenIdConfig, httpClient);
138+
configurationEndpoint, abstractOpenIdConfig);
162139
openIdConfigurationResp = mergeResponse(response, abstractOpenIdConfig);
163140
} catch (final RuntimeException | IOException e) {
164141
LOGGER.error(e.getMessage(), e);
@@ -181,39 +158,43 @@ private OpenIdConfigurationResponse updateOpenIdConfigurationResponse(
181158

182159
private OpenIdConfigurationResponse fetchOpenIdConfigurationResponse(
183160
final String configurationEndpoint,
184-
final AbstractOpenIdConfig abstractOpenIdConfig,
185-
final HttpClient httpClient) throws IOException {
161+
final AbstractOpenIdConfig abstractOpenIdConfig) throws IOException {
186162

187163
Objects.requireNonNull(configurationEndpoint,
188164
"Property "
189165
+ abstractOpenIdConfig.getFullPathStr(AbstractOpenIdConfig.PROP_NAME_CONFIGURATION_ENDPOINT)
190166
+ " has not been set");
191167
LOGGER.info("Fetching open id configuration from: " + configurationEndpoint);
192168

193-
final HttpGet httpGet = new HttpGet(configurationEndpoint);
169+
// final HttpGet httpGet = new HttpGet(configurationEndpoint);
194170
long sleepMs = 500;
195171
Throwable lastThrowable = null;
196172

197173
while (true) {
198174
try {
199-
return httpClient.execute(httpGet, response -> {
200-
if (HttpServletResponse.SC_OK == response.getCode()) {
201-
final HttpEntity entity = response.getEntity();
202-
String msg;
203-
try (final InputStream is = entity.getContent()) {
204-
msg = StreamUtil.streamToString(is);
205-
}
206-
175+
final WebTarget webTarget = jerseyClientFactory.getNamedClient(JerseyClientName.OPEN_ID)
176+
.target(configurationEndpoint);
177+
178+
try (Response response = webTarget.request(MediaType.APPLICATION_JSON).get()) {
179+
if (response.getStatus() == HttpServletResponse.SC_OK) {
180+
// According to the spec, the response may contain unexpected props so as we don't easily
181+
// have access to configure the client to relax jackson, do it manually.
182+
// https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationResponse
183+
final String responseJson = response.readEntity(String.class);
184+
LOGGER.debug("configurationEndpoint: '{}'\n{}",
185+
configurationEndpoint,
186+
responseJson);
207187
final OpenIdConfigurationResponse openIdConfigurationResponse = parseConfigurationResponse(
208188
configurationEndpoint,
209-
msg);
189+
responseJson);
190+
210191
LOGGER.info("Successfully fetched open id configuration from: " + configurationEndpoint);
211192
return openIdConfigurationResponse;
212193
} else {
213-
throw new AuthenticationException("Received status " + response.getCode() +
194+
throw new AuthenticationException("Received status " + response.getStatus() +
214195
" from " + configurationEndpoint);
215196
}
216-
});
197+
}
217198
} catch (AuthenticationException e) {
218199
// This is not a connection issue so just bubble it up and likely crash the app
219200
// if this is happening as part of the guice injector init.

‎stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/StroomOpenIdConfig.java

+3-7
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import stroom.security.openid.api.IdpType;
55
import stroom.util.config.annotations.RequiresRestart;
66
import stroom.util.config.annotations.RequiresRestart.RestartScope;
7-
import stroom.util.http.HttpClientConfiguration;
87
import stroom.util.shared.IsStroomConfig;
98

109
import com.fasterxml.jackson.annotation.JsonCreator;
@@ -42,8 +41,7 @@ public StroomOpenIdConfig(
4241
@JsonProperty("validIssuers") final Set<String> validIssuers,
4342
@JsonProperty("uniqueIdentityClaim") final String uniqueIdentityClaim,
4443
@JsonProperty("userDisplayNameClaim") final String userDisplayNameClaim,
45-
@JsonProperty(PROP_NAME_EXPECTED_SIGNER_PREFIXES) final Set<String> expectedSignerPrefixes,
46-
@JsonProperty("httpClient") final HttpClientConfiguration httpClient) {
44+
@JsonProperty(PROP_NAME_EXPECTED_SIGNER_PREFIXES) final Set<String> expectedSignerPrefixes) {
4745
super(identityProviderType,
4846
openIdConfigurationEndpoint,
4947
issuer,
@@ -61,8 +59,7 @@ public StroomOpenIdConfig(
6159
validIssuers,
6260
uniqueIdentityClaim,
6361
userDisplayNameClaim,
64-
expectedSignerPrefixes,
65-
httpClient);
62+
expectedSignerPrefixes);
6663
}
6764

6865
@RequiresRestart(RestartScope.SYSTEM)
@@ -101,7 +98,6 @@ public StroomOpenIdConfig withIdentityProviderType(final IdpType identityProvide
10198
getValidIssuers(),
10299
getUniqueIdentityClaim(),
103100
getUserDisplayNameClaim(),
104-
getExpectedSignerPrefixes(),
105-
getHttpClient());
101+
getExpectedSignerPrefixes());
106102
}
107103
}

‎stroom-security/stroom-security-openid-api/src/main/java/stroom/security/openid/api/AbstractOpenIdConfig.java

+3-18
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
package stroom.security.openid.api;
22

3-
import stroom.util.http.HttpClientConfiguration;
43
import stroom.util.shared.AbstractConfig;
54
import stroom.util.shared.validation.AllMatchPattern;
65

@@ -130,8 +129,6 @@ public abstract class AbstractOpenIdConfig
130129

131130
private final Set<String> expectedSignerPrefixes;
132131

133-
private final HttpClientConfiguration httpClient;
134-
135132
public AbstractOpenIdConfig() {
136133
identityProviderType = getDefaultIdpType();
137134
openIdConfigurationEndpoint = null;
@@ -151,7 +148,6 @@ public AbstractOpenIdConfig() {
151148
uniqueIdentityClaim = OpenId.CLAIM__SUBJECT;
152149
userDisplayNameClaim = OpenId.CLAIM__PREFERRED_USERNAME;
153150
expectedSignerPrefixes = Collections.emptySet();
154-
httpClient = null;
155151
}
156152

157153
@JsonIgnore
@@ -176,8 +172,7 @@ public AbstractOpenIdConfig(
176172
@JsonProperty("validIssuers") final Set<String> validIssuers,
177173
@JsonProperty("uniqueIdentityClaim") final String uniqueIdentityClaim,
178174
@JsonProperty("userDisplayNameClaim") final String userDisplayNameClaim,
179-
@JsonProperty(PROP_NAME_EXPECTED_SIGNER_PREFIXES) final Set<String> expectedSignerPrefixes,
180-
@JsonProperty("httpClient") final HttpClientConfiguration httpClient) {
175+
@JsonProperty(PROP_NAME_EXPECTED_SIGNER_PREFIXES) final Set<String> expectedSignerPrefixes) {
181176

182177
this.identityProviderType = identityProviderType;
183178
this.openIdConfigurationEndpoint = openIdConfigurationEndpoint;
@@ -197,7 +192,6 @@ public AbstractOpenIdConfig(
197192
this.uniqueIdentityClaim = uniqueIdentityClaim;
198193
this.userDisplayNameClaim = userDisplayNameClaim;
199194
this.expectedSignerPrefixes = expectedSignerPrefixes;
200-
this.httpClient = httpClient;
201195
}
202196

203197
/**
@@ -384,12 +378,6 @@ public boolean isConfigurationEndpointValid() {
384378
|| (openIdConfigurationEndpoint != null && !openIdConfigurationEndpoint.isBlank());
385379
}
386380

387-
@JsonProperty
388-
@JsonPropertyDescription("Configuration for the HTTP client to communicate with the external IDP")
389-
public HttpClientConfiguration getHttpClient() {
390-
return httpClient;
391-
}
392-
393381
@Override
394382
public String toString() {
395383
return "OpenIdConfig{" +
@@ -408,7 +396,6 @@ public String toString() {
408396
", validateAudience=" + validateAudience +
409397
", uniqueIdentityClaim=" + uniqueIdentityClaim +
410398
", expectedSignerPrefixes=" + expectedSignerPrefixes +
411-
", httpClientConfiguration=" + httpClient +
412399
'}';
413400
}
414401

@@ -435,8 +422,7 @@ public boolean equals(final Object o) {
435422
Objects.equals(clientSecret, that.clientSecret) &&
436423
Objects.equals(requestScopes, that.requestScopes) &&
437424
Objects.equals(uniqueIdentityClaim, that.uniqueIdentityClaim) &&
438-
Objects.equals(expectedSignerPrefixes, that.expectedSignerPrefixes) &&
439-
Objects.equals(httpClient, that.httpClient);
425+
Objects.equals(expectedSignerPrefixes, that.expectedSignerPrefixes);
440426
}
441427

442428
@Override
@@ -455,7 +441,6 @@ public int hashCode() {
455441
requestScopes,
456442
validateAudience,
457443
uniqueIdentityClaim,
458-
expectedSignerPrefixes,
459-
httpClient);
444+
expectedSignerPrefixes);
460445
}
461446
}

‎unreleased_changes/20250403_094717_358__4837.md

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
* Issue **#4837** : Change the fetching of OIDC config to use jersey client instead of Apache http client. The yaml properties `appConfig.security.authentication.openId.httpClient` and `proxyConfig.security.authentication.openId.httpClient` have been removed. Configuration of the jersey client is now done using `jerseyClients.OPEN_ID.` (see https://gchq.github.io/stroom-docs/docs/install-guide/configuration/stroom-and-proxy/common-configuration/#jersey-http-client-configuration).
2+
3+
4+
```sh
5+
# ********************************************************************************
6+
# Issue title: OIDC config GET uses httpClient, but token calls use jerseyClient
7+
# Issue link: https://github.com/gchq/stroom/issues/4837
8+
# ********************************************************************************
9+
10+
# ONLY the top line will be included as a change entry in the CHANGELOG.
11+
# The entry should be in GitHub flavour markdown and should be written on a SINGLE
12+
# line with no hard breaks. You can have multiple change files for a single GitHub issue.
13+
# The entry should be written in the imperative mood, i.e. 'Fix nasty bug' rather than
14+
# 'Fixed nasty bug'.
15+
#
16+
# Examples of acceptable entries are:
17+
#
18+
#
19+
# * Issue **123** : Fix bug with an associated GitHub issue in this repository
20+
#
21+
# * Issue **namespace/other-repo#456** : Fix bug with an associated GitHub issue in another repository
22+
#
23+
# * Fix bug with no associated GitHub issue.
24+
```

0 commit comments

Comments
 (0)