Skip to content
This repository was archived by the owner on May 31, 2022. It is now read-only.

Allow missing "active" field in check_token/introspect response. #1533

Closed
wants to merge 1 commit into from
Closed

Allow missing "active" field in check_token/introspect response. #1533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ...src/main/java/org/springframework/security/oauth2/provider/token/RemoteTokenServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public OAuth2Authentication loadAuthentication(String accessToken) throws Authen
}

// gh-838
if (!Boolean.TRUE.equals(map.get("active"))) {
if (map.containsKey("active") && !"true".equals(String.valueOf(map.get("active")))) {
Copy link

Choose a reason for hiding this comment

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

if Boolean.FALSE.equals(map.get("active")) thrown ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because "active" could also be null or a String instead of a false boolean...

Copy link

Choose a reason for hiding this comment

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

and you just want to throw the exception if it's false, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so, but taking String values into account. So would you prefer the following?

Suggested change
if (map.containsKey("active") && !"true".equals(String.valueOf(map.get("active")))) {
if (map.containsKey("active") && "false".equals(String.valueOf(map.get("active")))) {

Copy link

Choose a reason for hiding this comment

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

I'm not sure. I'd keep it simple. We're facing an issue with it here.
To me feels like you wanna to throw an exception just if it was explicitly disabled. If you have something else there it should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference with this change, is that a null active value (or any arbitrary value other than false) would be considered as a valid token... which may be a bit risky? Checking if it's true is more restrictive and looks safer to me.

logger.debug("check_token returned active attribute: " + map.get("active"));
throw new InvalidTokenException(accessToken);
}
Expand Down
22 changes: 18 additions & 4 deletions ...test/java/org/springframework/security/oauth2/provider/token/RemoteTokenServicesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public void setUp() {

// gh-838
@Test
public void loadAuthenticationWhenIntrospectionResponseContainsActiveTrueThenReturnAuthentication() throws Exception {
public void loadAuthenticationWhenIntrospectionResponseContainsActiveTrueBooleanThenReturnAuthentication() throws Exception {
Map responseAttrs = new HashMap();
responseAttrs.put("active", true); // "active" is the only required attribute as per RFC 7662 (https://tools.ietf.org/search/rfc7662#section-2.2)
ResponseEntity<Map> response = new ResponseEntity<Map>(responseAttrs, HttpStatus.OK);
Expand All @@ -65,6 +65,19 @@ public void loadAuthenticationWhenIntrospectionResponseContainsActiveTrueThenRet
assertNotNull(authentication);
}

@Test
public void loadAuthenticationWhenIntrospectionResponseContainsActiveTrueStringThenReturnAuthentication() throws Exception {
Map responseAttrs = new HashMap();
responseAttrs.put("active", "true"); // "active" is the only required attribute as per RFC 7662 (https://tools.ietf.org/search/rfc7662#section-2.2)
ResponseEntity<Map> response = new ResponseEntity<Map>(responseAttrs, HttpStatus.OK);
RestTemplate restTemplate = mock(RestTemplate.class);
when(restTemplate.exchange(anyString(), any(HttpMethod.class), any(HttpEntity.class), any(Class.class))).thenReturn(response);
this.remoteTokenServices.setRestTemplate(restTemplate);

OAuth2Authentication authentication = this.remoteTokenServices.loadAuthentication("access-token-1234");
assertNotNull(authentication);
}

// gh-838
@Test(expected = InvalidTokenException.class)
public void loadAuthenticationWhenIntrospectionResponseContainsActiveFalseThenThrowInvalidTokenException() throws Exception {
Expand All @@ -79,14 +92,15 @@ public void loadAuthenticationWhenIntrospectionResponseContainsActiveFalseThenTh
}

// gh-838
@Test(expected = InvalidTokenException.class)
public void loadAuthenticationWhenIntrospectionResponseMissingActiveAttributeThenThrowInvalidTokenException() throws Exception {
@Test
public void loadAuthenticationWhenIntrospectionResponseMissingActiveAttributeThenReturnAuthentication() throws Exception {
Map responseAttrs = new HashMap();
ResponseEntity<Map> response = new ResponseEntity<Map>(responseAttrs, HttpStatus.OK);
RestTemplate restTemplate = mock(RestTemplate.class);
when(restTemplate.exchange(anyString(), any(HttpMethod.class), any(HttpEntity.class), any(Class.class))).thenReturn(response);
this.remoteTokenServices.setRestTemplate(restTemplate);

this.remoteTokenServices.loadAuthentication("access-token-1234");
OAuth2Authentication authentication = this.remoteTokenServices.loadAuthentication("access-token-1234");
assertNotNull(authentication);
}
}