Skip to content

Commit e56afb7

Browse files
okuminokumin
committed
Core: Stop fetchToken from sending the Authorization header
1 parent c22bac8 commit e56afb7

File tree

3 files changed

+107
-30
lines changed

3 files changed

+107
-30
lines changed

‎core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Util.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.concurrent.TimeUnit;
3535
import java.util.concurrent.atomic.AtomicReference;
3636
import java.util.regex.Pattern;
37+
import java.util.stream.Collectors;
3738
import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
3839
import org.apache.iceberg.relocated.com.google.common.base.Joiner;
3940
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
@@ -243,7 +244,14 @@ public static OAuthTokenResponse fetchToken(
243244
oauth2ServerUri,
244245
request,
245246
OAuthTokenResponse.class,
246-
headers,
247+
// RFC 6749 proposes two ways to send a credential: HTTP Basic authentication or
248+
// request-body. Historically, the Iceberg library prefers the latter one for
249+
// compatibility while the RFC recommends the former one. As sending both the
250+
// Authorization header and the request-body parameters might confuse some authorization
251+
// servers, the following line is excluding the Authorization header.
252+
headers.entrySet().stream()
253+
.filter(entry -> !AUTHORIZATION_HEADER.equals(entry.getKey()))
254+
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)),
247255
ErrorHandlers.oauthErrorHandler());
248256
response.validate();
249257

‎core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java‎

Lines changed: 95 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -415,15 +415,21 @@ public void testCatalogCredentialNoOauth2ServerUri() {
415415

416416
RESTCatalog catalog =
417417
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
418+
String catalogCredential = "catalog:secret";
418419
catalog.initialize(
419-
"prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", "catalog:secret"));
420+
"prod", ImmutableMap.of(CatalogProperties.URI, "ignored", "credential", catalogCredential));
420421

421422
assertThat(catalog.tableExists(TBL)).isFalse();
422423

423424
// no token or credential for catalog token exchange
424425
Mockito.verify(adapter)
425426
.execute(
426-
reqMatcher(HTTPMethod.POST, ResourcePaths.tokens(), emptyHeaders),
427+
reqMatcher(
428+
HTTPMethod.POST,
429+
ResourcePaths.tokens(),
430+
emptyHeaders,
431+
ImmutableMap.of(),
432+
createClientCredentialsRequest(catalogCredential)),
427433
eq(OAuthTokenResponse.class),
428434
any(),
429435
any());
@@ -454,13 +460,14 @@ public void testCatalogCredential(String oauth2ServerUri) {
454460

455461
RESTCatalog catalog =
456462
new RESTCatalog(SessionCatalog.SessionContext.createEmpty(), (config) -> adapter);
463+
String catalogCredential = "catalog:secret";
457464
catalog.initialize(
458465
"prod",
459466
ImmutableMap.of(
460467
CatalogProperties.URI,
461468
"ignored",
462469
"credential",
463-
"catalog:secret",
470+
catalogCredential,
464471
OAuth2Properties.OAUTH2_SERVER_URI,
465472
oauth2ServerUri));
466473

@@ -469,7 +476,12 @@ public void testCatalogCredential(String oauth2ServerUri) {
469476
// no token or credential for catalog token exchange
470477
Mockito.verify(adapter)
471478
.execute(
472-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders),
479+
reqMatcher(
480+
HTTPMethod.POST,
481+
oauth2ServerUri,
482+
emptyHeaders,
483+
ImmutableMap.of(),
484+
createClientCredentialsRequest(catalogCredential)),
473485
eq(OAuthTokenResponse.class),
474486
any(),
475487
any());
@@ -498,11 +510,12 @@ public void testCatalogBearerTokenWithClientCredential(String oauth2ServerUri) {
498510

499511
RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
500512

513+
String contextCredential = "user:secret";
501514
SessionCatalog.SessionContext context =
502515
new SessionCatalog.SessionContext(
503516
UUID.randomUUID().toString(),
504517
"user",
505-
ImmutableMap.of("credential", "user:secret"),
518+
ImmutableMap.of("credential", contextCredential),
506519
ImmutableMap.of());
507520

508521
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
@@ -525,10 +538,15 @@ public void testCatalogBearerTokenWithClientCredential(String oauth2ServerUri) {
525538
eq(ConfigResponse.class),
526539
any(),
527540
any());
528-
// use the bearer token to fetch the context token
541+
// use the request-body to fetch the context token
529542
Mockito.verify(adapter)
530543
.execute(
531-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders),
544+
reqMatcher(
545+
HTTPMethod.POST,
546+
oauth2ServerUri,
547+
ImmutableMap.of(),
548+
ImmutableMap.of(),
549+
createClientCredentialsRequest(contextCredential)),
532550
eq(OAuthTokenResponse.class),
533551
any(),
534552
any());
@@ -552,21 +570,23 @@ public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) {
552570

553571
RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
554572

573+
String contextCredential = "user:secret";
555574
SessionCatalog.SessionContext context =
556575
new SessionCatalog.SessionContext(
557576
UUID.randomUUID().toString(),
558577
"user",
559-
ImmutableMap.of("credential", "user:secret"),
578+
ImmutableMap.of("credential", contextCredential),
560579
ImmutableMap.of());
561580

562581
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
582+
String catalogCredential = "catalog:secret";
563583
catalog.initialize(
564584
"prod",
565585
ImmutableMap.of(
566586
CatalogProperties.URI,
567587
"ignored",
568588
"credential",
569-
"catalog:secret",
589+
catalogCredential,
570590
OAuth2Properties.OAUTH2_SERVER_URI,
571591
oauth2ServerUri));
572592

@@ -575,7 +595,12 @@ public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) {
575595
// call client credentials with no initial auth
576596
Mockito.verify(adapter)
577597
.execute(
578-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders),
598+
reqMatcher(
599+
HTTPMethod.POST,
600+
oauth2ServerUri,
601+
emptyHeaders,
602+
ImmutableMap.of(),
603+
createClientCredentialsRequest(catalogCredential)),
579604
eq(OAuthTokenResponse.class),
580605
any(),
581606
any());
@@ -589,7 +614,12 @@ public void testCatalogCredentialWithClientCredential(String oauth2ServerUri) {
589614
// use the client credential to fetch the context token
590615
Mockito.verify(adapter)
591616
.execute(
592-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders),
617+
reqMatcher(
618+
HTTPMethod.POST,
619+
oauth2ServerUri,
620+
emptyHeaders,
621+
ImmutableMap.of(),
622+
createClientCredentialsRequest(contextCredential)),
593623
eq(OAuthTokenResponse.class),
594624
any(),
595625
any());
@@ -613,11 +643,12 @@ public void testCatalogBearerTokenAndCredentialWithClientCredential(String oauth
613643

614644
RESTCatalogAdapter adapter = Mockito.spy(new RESTCatalogAdapter(backendCatalog));
615645

646+
String contextCredential = "user:secret";
616647
SessionCatalog.SessionContext context =
617648
new SessionCatalog.SessionContext(
618649
UUID.randomUUID().toString(),
619650
"user",
620-
ImmutableMap.of("credential", "user:secret"),
651+
ImmutableMap.of("credential", contextCredential),
621652
ImmutableMap.of());
622653

623654
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
@@ -652,7 +683,12 @@ public void testCatalogBearerTokenAndCredentialWithClientCredential(String oauth
652683
// use the client credential to fetch the context token
653684
Mockito.verify(adapter)
654685
.execute(
655-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders),
686+
reqMatcher(
687+
HTTPMethod.POST,
688+
oauth2ServerUri,
689+
ImmutableMap.of(),
690+
ImmutableMap.of(),
691+
createClientCredentialsRequest(contextCredential)),
656692
eq(OAuthTokenResponse.class),
657693
any(),
658694
any());
@@ -828,7 +864,19 @@ private void testClientAuth(
828864

829865
// token passes a static token. otherwise, validate a client credentials or token exchange
830866
// request
831-
if (!credentials.containsKey("token")) {
867+
if (!credentials.containsKey("token") && credentials.containsKey("credential")) {
868+
Mockito.verify(adapter)
869+
.execute(
870+
reqMatcher(
871+
HTTPMethod.POST,
872+
oauth2ServerUri,
873+
ImmutableMap.of(),
874+
ImmutableMap.of(),
875+
createClientCredentialsRequest(credentials.get("credential"))),
876+
eq(OAuthTokenResponse.class),
877+
any(),
878+
any());
879+
} else if (!credentials.containsKey("token")) {
832880
Mockito.verify(adapter)
833881
.execute(
834882
reqMatcher(HTTPMethod.POST, oauth2ServerUri, catalogHeaders),
@@ -1294,13 +1342,14 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) {
12941342
UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of());
12951343

12961344
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
1345+
String catalogCredential = "catalog:secret";
12971346
catalog.initialize(
12981347
"prod",
12991348
ImmutableMap.of(
13001349
CatalogProperties.URI,
13011350
"ignored",
13021351
"credential",
1303-
"catalog:secret",
1352+
catalogCredential,
13041353
OAuth2Properties.OAUTH2_SERVER_URI,
13051354
oauth2ServerUri));
13061355

@@ -1311,7 +1360,12 @@ public void testCatalogTokenRefresh(String oauth2ServerUri) {
13111360
// call client credentials with no initial auth
13121361
Mockito.verify(adapter)
13131362
.execute(
1314-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders),
1363+
reqMatcher(
1364+
HTTPMethod.POST,
1365+
oauth2ServerUri,
1366+
emptyHeaders,
1367+
ImmutableMap.of(),
1368+
createClientCredentialsRequest(catalogCredential)),
13151369
eq(OAuthTokenResponse.class),
13161370
any(),
13171371
any());
@@ -1400,13 +1454,14 @@ public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) {
14001454
UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of());
14011455

14021456
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
1457+
String catalogCredential = "catalog:secret";
14031458
catalog.initialize(
14041459
"prod",
14051460
ImmutableMap.of(
14061461
CatalogProperties.URI,
14071462
"ignored",
14081463
"credential",
1409-
"catalog:secret",
1464+
catalogCredential,
14101465
OAuth2Properties.OAUTH2_SERVER_URI,
14111466
oauth2ServerUri));
14121467

@@ -1418,20 +1473,14 @@ public void testCatalogRefreshedTokenIsUsed(String oauth2ServerUri) {
14181473
assertThat(catalog.tableExists(TBL)).isFalse();
14191474

14201475
// call client credentials with no initial auth
1421-
Map<String, String> clientCredentialsRequest =
1422-
ImmutableMap.of(
1423-
"grant_type", "client_credentials",
1424-
"client_id", "catalog",
1425-
"client_secret", "secret",
1426-
"scope", "catalog");
14271476
Mockito.verify(adapter)
14281477
.execute(
14291478
reqMatcher(
14301479
HTTPMethod.POST,
14311480
oauth2ServerUri,
14321481
emptyHeaders,
14331482
Map.of(),
1434-
clientCredentialsRequest),
1483+
createClientCredentialsRequest(catalogCredential)),
14351484
eq(OAuthTokenResponse.class),
14361485
any(),
14371486
any());
@@ -1516,13 +1565,14 @@ public void testCatalogTokenRefreshExchangeDisabled(String oauth2ServerUri) {
15161565
UUID.randomUUID().toString(), "user", contextCredentials, ImmutableMap.of());
15171566

15181567
RESTCatalog catalog = new RESTCatalog(context, (config) -> adapter);
1568+
String catalogCredential = "catalog:secret";
15191569
catalog.initialize(
15201570
"prod",
15211571
ImmutableMap.of(
15221572
CatalogProperties.URI,
15231573
"ignored",
15241574
OAuth2Properties.CREDENTIAL,
1525-
"catalog:secret",
1575+
catalogCredential,
15261576
OAuth2Properties.TOKEN_EXCHANGE_ENABLED,
15271577
"false",
15281578
OAuth2Properties.OAUTH2_SERVER_URI,
@@ -1535,7 +1585,12 @@ public void testCatalogTokenRefreshExchangeDisabled(String oauth2ServerUri) {
15351585
// call client credentials with no initial auth
15361586
Mockito.verify(adapter)
15371587
.execute(
1538-
reqMatcher(HTTPMethod.POST, oauth2ServerUri, emptyHeaders),
1588+
reqMatcher(
1589+
HTTPMethod.POST,
1590+
oauth2ServerUri,
1591+
emptyHeaders,
1592+
ImmutableMap.of(),
1593+
createClientCredentialsRequest(catalogCredential)),
15391594
eq(OAuthTokenResponse.class),
15401595
any(),
15411596
any());
@@ -3163,4 +3218,18 @@ private static List<HTTPRequest> allRequests(RESTCatalogAdapter adapter) {
31633218
verify(adapter, atLeastOnce()).execute(captor.capture(), any(), any(), any());
31643219
return captor.getAllValues();
31653220
}
3221+
3222+
private static Map<String, String> createClientCredentialsRequest(String credential) {
3223+
String[] parsed = credential.split(":");
3224+
assertThat(parsed.length).isEqualTo(2);
3225+
return ImmutableMap.of(
3226+
"grant_type",
3227+
"client_credentials",
3228+
"client_id",
3229+
parsed[0],
3230+
"client_secret",
3231+
parsed[1],
3232+
"scope",
3233+
"catalog");
3234+
}
31663235
}

‎core/src/test/java/org/apache/iceberg/rest/auth/TestOAuth2Manager.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ void contextualSessionCredentialsProvided() {
230230
SessionCatalog.SessionContext context =
231231
new SessionCatalog.SessionContext(
232232
"test", "test", Map.of(OAuth2Properties.CREDENTIAL, "client:secret"), Map.of());
233-
Map<String, String> properties = Map.of();
233+
Map<String, String> properties = Map.of(OAuth2Properties.CREDENTIAL, "client:secret");
234234
try (OAuth2Manager manager = new OAuth2Manager("test");
235235
OAuth2Util.AuthSession catalogSession = manager.catalogSession(client, properties);
236236
OAuth2Util.AuthSession contextualSession =
@@ -243,7 +243,7 @@ void contextualSessionCredentialsProvided() {
243243
.as("should create session cache for context with credentials")
244244
.satisfies(cache -> assertThat(cache.sessionCache().asMap()).hasSize(1));
245245
}
246-
Mockito.verify(client)
246+
Mockito.verify(client, times(2))
247247
.postForm(
248248
any(),
249249
eq(
@@ -255,7 +255,7 @@ void contextualSessionCredentialsProvided() {
255255
eq(OAuthTokenResponse.class),
256256
eq(Map.of()),
257257
any());
258-
Mockito.verify(client).withAuthSession(any());
258+
Mockito.verify(client, times(2)).withAuthSession(any());
259259
Mockito.verifyNoMoreInteractions(client);
260260
}
261261

0 commit comments

Comments
 (0)