Skip to content

Commit e18078a

Browse files
raphaelDLraphaelDL
committed
Improve error messages in OidcIdTokenValidator
This commit ensures that error messages contain more specific information regarding the reported error. Fixes: gh-6323
1 parent 6e1db11 commit e18078a

File tree

2 files changed

+105
-58
lines changed

2 files changed

+105
-58
lines changed

‎oauth2/oauth2-client/src/main/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidator.java

+53-33
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2018 the original author or authors.
2+
* Copyright 2002-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -27,7 +27,10 @@
2727

2828
import java.net.URL;
2929
import java.time.Instant;
30+
import java.util.HashMap;
3031
import java.util.List;
32+
import java.util.Map;
33+
import java.util.stream.Collectors;
3134

3235
/**
3336
* An {@link OAuth2TokenValidator} responsible for
@@ -41,7 +44,6 @@
4144
* @see <a target="_blank" href="http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation">ID Token Validation</a>
4245
*/
4346
public final class OidcIdTokenValidator implements OAuth2TokenValidator<Jwt> {
44-
private static final OAuth2Error INVALID_ID_TOKEN_ERROR = new OAuth2Error("invalid_id_token");
4547
private final ClientRegistration clientRegistration;
4648

4749
public OidcIdTokenValidator(ClientRegistration clientRegistration) {
@@ -53,27 +55,10 @@ public OidcIdTokenValidator(ClientRegistration clientRegistration) {
5355
public OAuth2TokenValidatorResult validate(Jwt idToken) {
5456
// 3.1.3.7 ID Token Validation
5557
// http://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
58+
Map<String, Object> invalidClaims = validateRequiredClaims(idToken);
5659

57-
// Validate REQUIRED Claims
58-
URL issuer = idToken.getIssuer();
59-
if (issuer == null) {
60-
return invalidIdToken();
61-
}
62-
String subject = idToken.getSubject();
63-
if (subject == null) {
64-
return invalidIdToken();
65-
}
66-
List<String> audience = idToken.getAudience();
67-
if (CollectionUtils.isEmpty(audience)) {
68-
return invalidIdToken();
69-
}
70-
Instant expiresAt = idToken.getExpiresAt();
71-
if (expiresAt == null) {
72-
return invalidIdToken();
73-
}
74-
Instant issuedAt = idToken.getIssuedAt();
75-
if (issuedAt == null) {
76-
return invalidIdToken();
60+
if (!invalidClaims.isEmpty()){
61+
return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims));
7762
}
7863

7964
// 2. The Issuer Identifier for the OpenID Provider (which is typically obtained during Discovery)
@@ -85,21 +70,21 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) {
8570
// The aud (audience) Claim MAY contain an array with more than one element.
8671
// The ID Token MUST be rejected if the ID Token does not list the Client as a valid audience,
8772
// or if it contains additional audiences not trusted by the Client.
88-
if (!audience.contains(this.clientRegistration.getClientId())) {
89-
return invalidIdToken();
73+
if (!idToken.getAudience().contains(this.clientRegistration.getClientId())) {
74+
invalidClaims.put(IdTokenClaimNames.AUD, idToken.getAudience());
9075
}
9176

9277
// 4. If the ID Token contains multiple audiences,
9378
// the Client SHOULD verify that an azp Claim is present.
9479
String authorizedParty = idToken.getClaimAsString(IdTokenClaimNames.AZP);
95-
if (audience.size() > 1 && authorizedParty == null) {
96-
return invalidIdToken();
80+
if (idToken.getAudience().size() > 1 && authorizedParty == null) {
81+
invalidClaims.put(IdTokenClaimNames.AZP, authorizedParty);
9782
}
9883

9984
// 5. If an azp (authorized party) Claim is present,
10085
// the Client SHOULD verify that its client_id is the Claim Value.
10186
if (authorizedParty != null && !authorizedParty.equals(this.clientRegistration.getClientId())) {
102-
return invalidIdToken();
87+
invalidClaims.put(IdTokenClaimNames.AZP, authorizedParty);
10388
}
10489

10590
// 7. The alg value SHOULD be the default of RS256 or the algorithm sent by the Client
@@ -108,16 +93,16 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) {
10893

10994
// 9. The current time MUST be before the time represented by the exp Claim.
11095
Instant now = Instant.now();
111-
if (!now.isBefore(expiresAt)) {
112-
return invalidIdToken();
96+
if (!now.isBefore(idToken.getExpiresAt())) {
97+
invalidClaims.put(IdTokenClaimNames.EXP, idToken.getExpiresAt());
11398
}
11499

115100
// 10. The iat Claim can be used to reject tokens that were issued too far away from the current time,
116101
// limiting the amount of time that nonces need to be stored to prevent attacks.
117102
// The acceptable range is Client specific.
118103
Instant maxIssuedAt = now.plusSeconds(30);
119-
if (issuedAt.isAfter(maxIssuedAt)) {
120-
return invalidIdToken();
104+
if (idToken.getIssuedAt().isAfter(maxIssuedAt)) {
105+
invalidClaims.put(IdTokenClaimNames.IAT, idToken.getIssuedAt());
121106
}
122107

123108
// 11. If a nonce value was sent in the Authentication Request,
@@ -127,10 +112,45 @@ public OAuth2TokenValidatorResult validate(Jwt idToken) {
127112
// The precise method for detecting replay attacks is Client specific.
128113
// TODO Depends on gh-4442
129114

115+
if (!invalidClaims.isEmpty()) {
116+
return OAuth2TokenValidatorResult.failure(invalidIdToken(invalidClaims));
117+
}
118+
130119
return OAuth2TokenValidatorResult.success();
131120
}
132121

133-
private static OAuth2TokenValidatorResult invalidIdToken() {
134-
return OAuth2TokenValidatorResult.failure(INVALID_ID_TOKEN_ERROR);
122+
private static OAuth2Error invalidIdToken(Map<String, Object> invalidClaims) {
123+
String claimsDetail = invalidClaims.entrySet().stream()
124+
.map(it -> it.getKey()+ "("+it.getValue()+")")
125+
.collect(Collectors.joining(", "));
126+
127+
return new OAuth2Error("invalid_id_token", "The ID Token contains invalid claims: "+claimsDetail, null);
128+
}
129+
130+
private static Map<String, Object> validateRequiredClaims(Jwt idToken){
131+
Map<String, Object> requiredClaims = new HashMap<>();
132+
133+
URL issuer = idToken.getIssuer();
134+
if (issuer == null) {
135+
requiredClaims.put(IdTokenClaimNames.ISS, issuer);
136+
}
137+
String subject = idToken.getSubject();
138+
if (subject == null) {
139+
requiredClaims.put(IdTokenClaimNames.SUB, subject);
140+
}
141+
List<String> audience = idToken.getAudience();
142+
if (CollectionUtils.isEmpty(audience)) {
143+
requiredClaims.put(IdTokenClaimNames.AUD, audience);
144+
}
145+
Instant expiresAt = idToken.getExpiresAt();
146+
if (expiresAt == null) {
147+
requiredClaims.put(IdTokenClaimNames.EXP, expiresAt);
148+
}
149+
Instant issuedAt = idToken.getIssuedAt();
150+
if (issuedAt == null) {
151+
requiredClaims.put(IdTokenClaimNames.IAT, issuedAt);
152+
}
153+
154+
return requiredClaims;
135155
}
136156
}

‎oauth2/oauth2-client/src/test/java/org/springframework/security/oauth2/client/oidc/authentication/OidcIdTokenValidatorTests.java

+52-25
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2018 the original author or authors.
2+
* Copyright 2002-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -64,62 +64,63 @@ public void validateIdTokenWhenIssuerNullThenHasErrors() {
6464
this.claims.remove(IdTokenClaimNames.ISS);
6565
assertThat(this.validateIdToken())
6666
.hasSize(1)
67-
.extracting(OAuth2Error::getErrorCode)
68-
.contains("invalid_id_token");
67+
.extracting(OAuth2Error::getDescription)
68+
.allMatch(msg -> msg.contains(IdTokenClaimNames.ISS));
69+
6970
}
7071

7172
@Test
7273
public void validateIdTokenWhenSubNullThenHasErrors() {
7374
this.claims.remove(IdTokenClaimNames.SUB);
7475
assertThat(this.validateIdToken())
7576
.hasSize(1)
76-
.extracting(OAuth2Error::getErrorCode)
77-
.contains("invalid_id_token");
77+
.extracting(OAuth2Error::getDescription)
78+
.allMatch(msg -> msg.contains(IdTokenClaimNames.SUB));
7879
}
7980

8081
@Test
8182
public void validateIdTokenWhenAudNullThenHasErrors() {
8283
this.claims.remove(IdTokenClaimNames.AUD);
8384
assertThat(this.validateIdToken())
8485
.hasSize(1)
85-
.extracting(OAuth2Error::getErrorCode)
86-
.contains("invalid_id_token");
86+
.extracting(OAuth2Error::getDescription)
87+
.allMatch(msg -> msg.contains(IdTokenClaimNames.AUD));
8788
}
8889

8990
@Test
9091
public void validateIdTokenWhenIssuedAtNullThenHasErrors() {
9192
this.issuedAt = null;
9293
assertThat(this.validateIdToken())
9394
.hasSize(1)
94-
.extracting(OAuth2Error::getErrorCode)
95-
.contains("invalid_id_token");
95+
.extracting(OAuth2Error::getDescription)
96+
.allMatch(msg -> msg.contains(IdTokenClaimNames.IAT));
9697
}
9798

9899
@Test
99100
public void validateIdTokenWhenExpiresAtNullThenHasErrors() {
100101
this.expiresAt = null;
101102
assertThat(this.validateIdToken())
102103
.hasSize(1)
103-
.extracting(OAuth2Error::getErrorCode)
104-
.contains("invalid_id_token");
104+
.extracting(OAuth2Error::getDescription)
105+
.allMatch(msg -> msg.contains(IdTokenClaimNames.EXP));
105106
}
106107

107108
@Test
108109
public void validateIdTokenWhenAudMultipleAndAzpNullThenHasErrors() {
109110
this.claims.put(IdTokenClaimNames.AUD, Arrays.asList("client-id", "other"));
110111
assertThat(this.validateIdToken())
111112
.hasSize(1)
112-
.extracting(OAuth2Error::getErrorCode)
113-
.contains("invalid_id_token");
113+
.extracting(OAuth2Error::getDescription)
114+
.allMatch(msg -> msg.contains(IdTokenClaimNames.AZP));
114115
}
115116

116117
@Test
117118
public void validateIdTokenWhenAzpNotClientIdThenHasErrors() {
118119
this.claims.put(IdTokenClaimNames.AZP, "other");
119120
assertThat(this.validateIdToken())
120121
.hasSize(1)
121-
.extracting(OAuth2Error::getErrorCode)
122-
.contains("invalid_id_token");
122+
.extracting(OAuth2Error::getDescription)
123+
.allMatch(msg -> msg.contains(IdTokenClaimNames.AZP));
123124
}
124125

125126
@Test
@@ -135,17 +136,17 @@ public void validateIdTokenWhenMultipleAudAzpNotClientIdThenHasErrors() {
135136
this.claims.put(IdTokenClaimNames.AZP, "other-client");
136137
assertThat(this.validateIdToken())
137138
.hasSize(1)
138-
.extracting(OAuth2Error::getErrorCode)
139-
.contains("invalid_id_token");
139+
.extracting(OAuth2Error::getDescription)
140+
.allMatch(msg -> msg.contains(IdTokenClaimNames.AZP));
140141
}
141142

142143
@Test
143144
public void validateIdTokenWhenAudNotClientIdThenHasErrors() {
144145
this.claims.put(IdTokenClaimNames.AUD, Collections.singletonList("other-client"));
145146
assertThat(this.validateIdToken())
146147
.hasSize(1)
147-
.extracting(OAuth2Error::getErrorCode)
148-
.contains("invalid_id_token");
148+
.extracting(OAuth2Error::getDescription)
149+
.allMatch(msg -> msg.contains(IdTokenClaimNames.AUD));
149150
}
150151

151152
@Test
@@ -154,8 +155,8 @@ public void validateIdTokenWhenExpiredThenHasErrors() {
154155
this.expiresAt = this.issuedAt.plus(Duration.ofSeconds(1));
155156
assertThat(this.validateIdToken())
156157
.hasSize(1)
157-
.extracting(OAuth2Error::getErrorCode)
158-
.contains("invalid_id_token");
158+
.extracting(OAuth2Error::getDescription)
159+
.allMatch(msg -> msg.contains(IdTokenClaimNames.EXP));
159160
}
160161

161162
@Test
@@ -164,8 +165,8 @@ public void validateIdTokenWhenIssuedAtWayInFutureThenHasErrors() {
164165
this.expiresAt = this.issuedAt.plus(Duration.ofSeconds(1));
165166
assertThat(this.validateIdToken())
166167
.hasSize(1)
167-
.extracting(OAuth2Error::getErrorCode)
168-
.contains("invalid_id_token");
168+
.extracting(OAuth2Error::getDescription)
169+
.allMatch(msg -> msg.contains(IdTokenClaimNames.IAT));
169170
}
170171

171172
@Test
@@ -174,8 +175,34 @@ public void validateIdTokenWhenExpiresAtBeforeNowThenHasErrors() {
174175
this.expiresAt = Instant.from(this.issuedAt).plusSeconds(5);
175176
assertThat(this.validateIdToken())
176177
.hasSize(1)
177-
.extracting(OAuth2Error::getErrorCode)
178-
.contains("invalid_id_token");
178+
.extracting(OAuth2Error::getDescription)
179+
.allMatch(msg -> msg.contains(IdTokenClaimNames.EXP));
180+
}
181+
182+
@Test
183+
public void validateIdTokenWhenMissingClaimsThenHasErrors() {
184+
this.claims.remove(IdTokenClaimNames.SUB);
185+
this.claims.remove(IdTokenClaimNames.AUD);
186+
this.issuedAt = null;
187+
this.expiresAt = null;
188+
assertThat(this.validateIdToken())
189+
.hasSize(1)
190+
.extracting(OAuth2Error::getDescription)
191+
.allMatch(msg -> msg.contains(IdTokenClaimNames.SUB))
192+
.allMatch(msg -> msg.contains(IdTokenClaimNames.AUD))
193+
.allMatch(msg -> msg.contains(IdTokenClaimNames.IAT))
194+
.allMatch(msg -> msg.contains(IdTokenClaimNames.EXP));
195+
}
196+
197+
@Test(expected = IllegalArgumentException.class)
198+
public void validateIdTokenWhenNoClaimsThenHasErrors() {
199+
this.claims.remove(IdTokenClaimNames.ISS);
200+
this.claims.remove(IdTokenClaimNames.SUB);
201+
this.claims.remove(IdTokenClaimNames.AUD);
202+
this.issuedAt = null;
203+
this.expiresAt = null;
204+
assertThat(this.validateIdToken())
205+
.hasSize(1);
179206
}
180207

181208
private Collection<OAuth2Error> validateIdToken() {

0 commit comments

Comments
 (0)