Skip to content

Commit 8d713ec

Browse files
authored
Merge pull request #80 from swisscom/feature/44097-replace-msal4j-with-nimbus-oauth2-sdk
#44097 removed msal4j and replace with nimbus oauth2 sdk; this allowe…
2 parents 98c5834 + 077a13b commit 8d713ec

51 files changed

Lines changed: 797 additions & 426 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

README.md

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,13 +140,7 @@ The following environment variables can be set (to override their `dev` profile
140140

141141
The application JRE has to be started with the following system properties:
142142

143-
* `-Djdk.net.hosts.file=cdr-client-service/src/test/resources/msal4j_hosts`
144-
* `-Djavax.net.ssl.trustStore=cdr-client-service/src/test/resources/caddy_truststore.p12`
145-
* `-Djavax.net.ssl.trustStorePassword=changeit`
146-
147-
The first property sets a custom hosts file to resolve external servers that MSAL4J has hardcoded as valid IdPs and
148-
redirect them to `localhost`. The other properties are used to make the JRE trust the SSL certificate presented by the
149-
[caddy proxy](https://caddyserver.com/) server that we use to impersonate those servers.
143+
* currently none
150144

151145
### Hydraulic Conveyor
152146

cdr-client-common/src/main/kotlin/com/swisscom/health/des/cdr/client/common/DTOs.kt

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ class DTOs {
137137
DISABLED(true),
138138
ERROR(true),
139139
BROKEN(true),
140+
AUTHN_DENIED(true),
141+
AUTHN_COMMUNICATION_ERROR(true),
142+
AUTHN_UNKNOWN_ERROR(true),
140143
OFFLINE(false);
141144

142145
val isOfflineCategory: Boolean
@@ -190,7 +193,7 @@ class DTOs {
190193
cdrApi = Endpoint.EMPTY,
191194
filesInProgressCacheSize = EMPTY_STRING,
192195
idpCredentials = IdpCredentials.EMPTY,
193-
idpEndpoint = URI("http://localhost:8080").toURL(),
196+
idpEndpoint = URI("http://localhost").toURL(),
194197
localFolder = EMPTY_STRING,
195198
pullThreadPoolSize = 0,
196199
pushThreadPoolSize = 0,

cdr-client-service/build.gradle.kts

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,6 @@ idea {
2929
}
3030
}
3131

32-
tasks.bootRun {
33-
environment["JDK_JAVA_OPTIONS"] =
34-
listOfNotNull(
35-
environment["JDK_JAVA_OPTIONS"],
36-
"-Djavax.net.ssl.trustStore=src/test/resources/caddy_truststore.p12",
37-
"-Djavax.net.ssl.trustStorePassword=changeit",
38-
"-Djdk.net.hosts.file=src/test/resources/msal4j_hosts"
39-
).joinToString(" ")
40-
}
41-
4232
application {
4333
mainClass = "com.swisscom.health.des.cdr.client.CdrClientApplicationKt"
4434
}
@@ -63,7 +53,7 @@ dependencies {
6353
implementation(platform(libs.spring.boot.dependencies))
6454
implementation(platform(libs.spring.cloud.dependencies))
6555
implementation(libs.kache)
66-
implementation(libs.msal4j)
56+
implementation(libs.nimbusOAuth2Sdk)
6757
implementation(libs.okhttp)
6858
implementation(libs.kfswatch)
6959
implementation(libs.kotlin.logging)
@@ -100,8 +90,7 @@ dependencies {
10090
}
10191

10292
configurations.configureEach {
103-
// exclude globally so I don't have to explicitly exclude it from mockk and all of its transitive dependencies
104-
exclude(module = "junit", group = "junit")
93+
exclude(group = "junit", module = "junit")
10594
}
10695

10796
kapt {
@@ -173,11 +162,6 @@ tasks.withType<Test> {
173162
includeEngines("junit-jupiter")
174163
}
175164
finalizedBy(jacocoTestReport)
176-
177-
jvmArgs(
178-
// tests_hosts is used to redirect msal4j, which insists on talking to the Mothership, to our docker compose setup
179-
"-Djdk.net.hosts.file=${layout.projectDirectory.file("src/test/resources/test_hosts").asFile.absolutePath}"
180-
)
181165
}
182166

183167
jacoco {
@@ -252,13 +236,6 @@ tasks.register<Test>("integrationTest") {
252236
useJUnitPlatform {
253237
includeTags(Constants.INTEGRATION_TEST_TAG)
254238
}
255-
environment["JDK_JAVA_OPTIONS"] =
256-
listOfNotNull(
257-
environment["JDK_JAVA_OPTIONS"],
258-
"-Djavax.net.ssl.trustStore=src/test/resources/caddy_truststore.p12",
259-
"-Djavax.net.ssl.trustStorePassword=changeit",
260-
"-Djdk.net.hosts.file=src/test/resources/msal4j_hosts"
261-
).joinToString(" ")
262239
shouldRunAfter(tasks.test)
263240
// Ensure latest images get pulled
264241
dependsOn(tasks.composePull)

cdr-client-service/src/main/kotlin/com/swisscom/health/des/cdr/client/config/CdrClientContext.kt

Lines changed: 33 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,7 @@ package com.swisscom.health.des.cdr.client.config
33
import com.mayakapps.kache.InMemoryKache
44
import com.mayakapps.kache.KacheStrategy
55
import com.mayakapps.kache.ObjectKache
6-
import com.microsoft.aad.msal4j.ClientCredentialFactory
7-
import com.microsoft.aad.msal4j.ClientCredentialParameters
8-
import com.microsoft.aad.msal4j.ConfidentialClientApplication
9-
import com.microsoft.aad.msal4j.IConfidentialClientApplication
10-
import com.swisscom.health.des.cdr.client.common.Constants.EMPTY_STRING
6+
import com.swisscom.health.des.cdr.client.config.OAuth2AuthNService.AuthNResponse
117
import io.github.oshai.kotlinlogging.KotlinLogging
128
import kotlinx.coroutines.CoroutineDispatcher
139
import kotlinx.coroutines.Dispatchers
@@ -46,11 +42,35 @@ internal class CdrClientContext {
4642
@Bean
4743
fun okHttpClient(
4844
builder: OkHttpClient.Builder,
45+
oAuth2AuthNService: OAuth2AuthNService,
4946
@Value($$"${client.connection-timeout-ms}") timeout: Long,
5047
@Value($$"${client.read-timeout-ms}") readTimeout: Long
5148
): OkHttpClient =
5249
builder
5350
.connectTimeout(timeout, TimeUnit.MILLISECONDS).readTimeout(readTimeout, TimeUnit.MILLISECONDS)
51+
.addInterceptor { chain ->
52+
oAuth2AuthNService.getAccessToken()
53+
.let { authNResponse ->
54+
when (authNResponse) {
55+
is AuthNResponse.Success -> {
56+
chain
57+
.request()
58+
.newBuilder()
59+
.run {
60+
header("Authorization", "Bearer ${authNResponse.response.tokens.accessToken.value}")
61+
build()
62+
}.let { authenticatedRequest ->
63+
chain.proceed(authenticatedRequest)
64+
}
65+
}
66+
67+
else -> chain.proceed(chain.request()) // unauthenticated call; will probably fail with 401/403
68+
.also { _ ->
69+
logger.warn { "Authentication failed, proceeding unauthenticated; authentication response: '$authNResponse'" }
70+
}
71+
}
72+
}
73+
}
5474
.addInterceptor { chain ->
5575
val response: Response = chain.proceed(chain.request())
5676

@@ -59,7 +79,7 @@ internal class CdrClientContext {
5979
throw HttpServerErrorException(
6080
message = "Received error status code '${response.code}'.",
6181
statusCode = response.code,
62-
responseBody = response.body?.string() ?: EMPTY_STRING
82+
responseBody = response.body.string()
6383
)
6484
}
6585

@@ -121,26 +141,6 @@ internal class CdrClientContext {
121141
}
122142
}
123143

124-
/**
125-
* Creates and returns an instance of the MSAL4J client object through which we can obtain an OAuth2 token.
126-
*/
127-
@Bean
128-
fun confidentialClientApp(config: CdrClientConfig): IConfidentialClientApplication =
129-
ConfidentialClientApplication.builder(
130-
config.idpCredentials.clientId.id,
131-
ClientCredentialFactory.createFromSecret(config.idpCredentials.clientSecret.value)
132-
).authority(config.idpEndpoint.toString())
133-
// TODO: Implement application level retry of all remote calls and then comment in the line below
134-
// .disableInternalRetries()
135-
.build()
136-
137-
/**
138-
* Creates and returns an instance of credentials to be used with the MSAL4J client to obtain an OAuth2 token.
139-
*/
140-
@Bean
141-
fun clientCredentialParams(config: CdrClientConfig): ClientCredentialParameters =
142-
ClientCredentialParameters.builder(setOf(config.idpCredentials.scope.scope)).build()
143-
144144
/**
145145
* Creates and returns a spring retry-template that retries on IOExceptions up to three times before bailing out.
146146
*/
@@ -186,7 +186,13 @@ internal class CdrClientContext {
186186

187187
}
188188

189-
internal class HttpServerErrorException(message: String, val statusCode: Int, val responseBody: String) : RuntimeException(message)
189+
internal class HttpServerErrorException(message: String, val statusCode: Int, val responseBody: String) : RuntimeException(message, null, false, false) {
190+
override fun toString(): String {
191+
return "HttpServerErrorException(statusCode='$statusCode', responseBody='$responseBody', message='${message}')"
192+
}
193+
}
194+
195+
internal class WrongCredentialsException(message: String) : RuntimeException(message, null, false, false)
190196

191197
sealed interface FileBusyTester {
192198
suspend fun isBusy(file: Path): Boolean
Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
package com.swisscom.health.des.cdr.client.config
2+
3+
import com.nimbusds.oauth2.sdk.AccessTokenResponse
4+
import com.nimbusds.oauth2.sdk.AuthorizationGrant
5+
import com.nimbusds.oauth2.sdk.ClientCredentialsGrant
6+
import com.nimbusds.oauth2.sdk.Scope
7+
import com.nimbusds.oauth2.sdk.TokenRequest
8+
import com.nimbusds.oauth2.sdk.TokenResponse
9+
import com.nimbusds.oauth2.sdk.auth.ClientAuthentication
10+
import com.nimbusds.oauth2.sdk.auth.ClientSecretPost
11+
import com.nimbusds.oauth2.sdk.auth.Secret
12+
import com.nimbusds.oauth2.sdk.http.HTTPResponse
13+
import com.nimbusds.oauth2.sdk.id.ClientID
14+
import com.swisscom.health.des.cdr.client.config.OAuth2AuthNService.AuthNState.AUTHENTICATED
15+
import com.swisscom.health.des.cdr.client.config.OAuth2AuthNService.AuthNState.DENIED
16+
import com.swisscom.health.des.cdr.client.config.OAuth2AuthNService.AuthNState.RETRYABLE_FAILURE
17+
import com.swisscom.health.des.cdr.client.config.OAuth2AuthNService.AuthNState.UNAUTHENTICATED
18+
import org.springframework.retry.support.RetryTemplate
19+
import org.springframework.stereotype.Service
20+
import java.io.IOException
21+
import java.net.URI
22+
import java.net.URL
23+
import kotlin.time.Clock
24+
import kotlin.time.ExperimentalTime
25+
26+
@Service
27+
internal class OAuth2AuthNService @OptIn(ExperimentalTime::class) constructor(
28+
private val config: CdrClientConfig,
29+
private val retryIoErrors: RetryTemplate,
30+
private val clock: Clock = Clock.System,
31+
) {
32+
33+
private var accessTokenAuthNResponse: AuthNResponse = AuthNResponse.NotAuthenticated
34+
35+
internal enum class AuthNState {
36+
AUTHENTICATED,
37+
UNAUTHENTICATED,
38+
RETRYABLE_FAILURE,
39+
FAILED,
40+
DENIED;
41+
}
42+
43+
internal sealed interface AuthNResponse {
44+
data class Success(val response: AccessTokenResponse) : AuthNResponse
45+
data class Deny(val error: WrongCredentialsException) : AuthNResponse
46+
data class RetryableFailure(val error: IOException) : AuthNResponse
47+
data class Failed(val error: IllegalStateException) : AuthNResponse
48+
object NotAuthenticated : AuthNResponse
49+
}
50+
51+
@Synchronized
52+
internal fun currentAuthNState(): AuthNState =
53+
when (accessTokenAuthNResponse) {
54+
is AuthNResponse.Success -> AUTHENTICATED
55+
is AuthNResponse.RetryableFailure -> RETRYABLE_FAILURE
56+
is AuthNResponse.Failed -> AuthNState.FAILED
57+
is AuthNResponse.Deny -> DENIED
58+
is AuthNResponse.NotAuthenticated -> UNAUTHENTICATED
59+
}
60+
61+
@OptIn(ExperimentalTime::class)
62+
// could be improved with more fine-grained locking to avoid blocking all threads while a valid token is available and no renewal is required;
63+
// but we have only five threads for uploads, one for downloads (all of which are mostly waiting for IO), and one for the service status check,
64+
// so lock contention should be minimal
65+
@Synchronized
66+
internal fun getAccessToken(): AuthNResponse =
67+
when (val currentTokenResponse = accessTokenAuthNResponse) {
68+
is AuthNResponse.NotAuthenticated, is AuthNResponse.RetryableFailure -> {
69+
getNewAccessToken(config.idpCredentials, config.idpEndpoint)
70+
}
71+
72+
is AuthNResponse.Deny, is AuthNResponse.Failed -> {
73+
currentTokenResponse
74+
}
75+
76+
is AuthNResponse.Success -> {
77+
val expiresOn = currentTokenResponse.response.customParameters["expires_on"] as Long?
78+
if (expiresOn == null || clock.now().epochSeconds > expiresOn) {
79+
getNewAccessToken(config.idpCredentials, config.idpEndpoint)
80+
} else {
81+
currentTokenResponse
82+
}
83+
}
84+
}.also {
85+
accessTokenAuthNResponse = it
86+
}
87+
88+
private fun getNewAccessToken(idpCredentials: IdpCredentials, idpEndpoint: URL): AuthNResponse {
89+
val clientGrant: AuthorizationGrant = ClientCredentialsGrant()
90+
val clientID = ClientID(idpCredentials.clientId.id)
91+
val clientSecret = Secret(idpCredentials.clientSecret.value)
92+
// `ClientSecretBasic` is another option; it works in production, but our mock IdP is not set up to get the client id from the Basic Auth header,
93+
// instead we use the form parameter `client_id`
94+
val clientAuth: ClientAuthentication = ClientSecretPost(clientID, clientSecret)
95+
val scope = Scope(idpCredentials.scope.scope)
96+
val tokenEndpoint: URI = idpEndpoint.toURI()
97+
val request = TokenRequest(tokenEndpoint, clientAuth, clientGrant, scope)
98+
99+
val authNResponse: AuthNResponse =
100+
runCatching {
101+
retryIoErrors.execute<HTTPResponse, Throwable> { _ ->
102+
request.toHTTPRequest().send()
103+
}.run { TokenResponse.parse(this) }
104+
}.fold(
105+
onSuccess = { httpResponse: TokenResponse ->
106+
if (httpResponse.indicatesSuccess()) {
107+
AuthNResponse.Success(httpResponse.toSuccessResponse())
108+
} else {
109+
AuthNResponse.Deny(
110+
WrongCredentialsException(
111+
"Failed to login; client id: '${idpCredentials.clientId}'; IdP endpoint: '$idpEndpoint'; message: '${
112+
httpResponse.toErrorResponse().toJSONObject()
113+
}'"
114+
)
115+
)
116+
}
117+
},
118+
onFailure = { t ->
119+
when (t) {
120+
is IOException -> AuthNResponse.RetryableFailure(t)
121+
else -> AuthNResponse.Failed(
122+
IllegalStateException(
123+
"Failed to login; client id: '${idpCredentials.clientId}'; IdP endpoint: '$idpEndpoint'; root cause: '$t'",
124+
t,
125+
)
126+
)
127+
}
128+
}
129+
)
130+
131+
return authNResponse
132+
}
133+
}

0 commit comments

Comments
 (0)