Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ private void validate() {
"aud must be of type List");
}
if (this.claims.containsKey(OAuth2TokenIntrospectionClaimNames.ISS)) {
validateURL(this.claims.get(OAuth2TokenIntrospectionClaimNames.ISS), "iss must be a valid URL");
validateIssuer(this.claims.get(OAuth2TokenIntrospectionClaimNames.ISS), "iss must be a valid URL");
}
}

Expand All @@ -326,16 +326,24 @@ private void acceptClaimValues(String name, Consumer<List<String>> valuesConsume
valuesConsumer.accept(values);
}

private static void validateURL(Object url, String errorMessage) {
private static void validateIssuer(Object url, String errorMessage) {
if (URL.class.isAssignableFrom(url.getClass())) {
return;
}

String str = url.toString();
if (str.isEmpty()) {
throw new IllegalArgumentException(errorMessage);
}

try {
new URI(url.toString()).toURL();
// Try parsing as URI
new URI(str);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there really a benefit from first trying to parse a URI, and if this fails, still continue successfully? IMHO it would be enough to just check if the string is blank or not. Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback You’re right — if we allow non-URI strings (like client IDs), then attempting to parse as a URI may not add much value.

My initial thought behind keeping the URI parsing was to still preserve compatibility with tokens where iss is a proper URI, and not reject them unintentionally. But since the validation already ensures the string isn’t blank, simplifying to just a non-blank check would be sufficient and cleaner.

I can update the implementation to only validate that iss is not blank. Would you prefer that direction?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I would prefer simplicity. Compatibility is still preserved with such a simpler approach

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make the changes

// If this succeeds, it’s a valid URI
}
catch (Exception ex) {
throw new IllegalArgumentException(errorMessage, ex);
// If parsing fails, allow plain string (fix for iss claim)
// Only log/debug if needed, no exception thrown
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package org.springframework.security.oauth2.server.authorization;

import org.junit.jupiter.api.Test;
import org.springframework.security.oauth2.core.OAuth2TokenIntrospectionClaimNames;

import java.util.List;

public class OAuth2TokenIntrospectionTests {

@Test
void buildWhenIssuerIsNonUriStringThenDoesNotThrow() {
String issuer = "client-id-123"; // plain string, not a URI

org.assertj.core.api.Assertions.assertThatCode(() -> {
OAuth2TokenIntrospection token =
OAuth2TokenIntrospection.builder(true)
.issuer(issuer)
.subject("user-123")
.build();

Object issClaim = token.getClaim(OAuth2TokenIntrospectionClaimNames.ISS);
org.assertj.core.api.Assertions.assertThat(issClaim).isEqualTo(issuer);

Object activeClaim = token.getClaim(OAuth2TokenIntrospectionClaimNames.ACTIVE);
org.assertj.core.api.Assertions.assertThat(activeClaim).isEqualTo(true);
}).doesNotThrowAnyException();
}

@Test
void buildWhenIssuerIsValidUriThenAcceptsIssuer() {
String issuer = "https://issuer.example.com";

OAuth2TokenIntrospection token =
OAuth2TokenIntrospection.builder(true)
.issuer(issuer)
.subject("user-123")
.build();

Object issClaim = token.getClaim(OAuth2TokenIntrospectionClaimNames.ISS);
org.assertj.core.api.Assertions.assertThat(issClaim).isEqualTo(issuer);

Object activeClaim = token.getClaim(OAuth2TokenIntrospectionClaimNames.ACTIVE);
org.assertj.core.api.Assertions.assertThat(activeClaim).isEqualTo(true);
}

@Test
void buildWithMultipleScopes() {
OAuth2TokenIntrospection token =
OAuth2TokenIntrospection.builder(true)
.scope("read")
.scope("write")
.build();

List<String> scopes = (List<String>) token.getClaim(OAuth2TokenIntrospectionClaimNames.SCOPE);
org.assertj.core.api.Assertions.assertThat(scopes).containsExactly("read", "write");
}
}