Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
@@ -1,9 +1,12 @@
package com.openframe.authz.service.auth;

import com.openframe.data.document.oauth.MongoOAuth2Authorization;
import com.openframe.data.document.oauth.RetiredRefreshToken;
import com.openframe.data.repository.oauth.MongoOAuth2AuthorizationRepository;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.security.oauth2.core.OAuth2RefreshToken;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
import org.springframework.security.oauth2.server.authorization.OAuth2Authorization;
import org.springframework.security.oauth2.server.authorization.OAuth2AuthorizationCode;
Expand All @@ -12,6 +15,9 @@
import org.springframework.security.oauth2.server.authorization.client.RegisteredClientRepository;
import org.springframework.stereotype.Service;

import java.time.Instant;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.Optional;
import java.util.stream.Stream;
Expand All @@ -27,14 +33,49 @@

private static final OAuth2TokenType AUTH_CODE = new OAuth2TokenType("code");

@Value("${openframe.authz.refresh-token-grace-seconds:60}")
private long refreshTokenGraceSeconds;

@Override
public void save(OAuth2Authorization authorization) {
log.debug("Saving authorization: {}", authorization.getId());

MongoOAuth2Authorization entity = MongoAuthorizationMapper.toEntity(authorization);
entity.setRetiredRefreshTokens(retiredTokensAfterSaving(entity));
repository.save(entity);

Check failure on line 45 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View check run for this annotation

Flamingo / Flamingo Code Review

Method call passed directly as argument to setRetiredRefreshTokens

Method call passed directly as argument to setRetiredRefreshTokens entity.setRetiredRefreshTokens(retiredTokensAfterSaving(entity)) calls retiredTokensAfterSaving(entity) inline as the argument of setRetiredRefreshTokens, violating OFJAVA-002's requirement that method calls be extracted to a named local before being passed as an argument. **What it risks:** Breakpoints and stack traces on this line cannot distinguish the computation from the assignment, making the rotation/retirement logic harder to debug. **How to fix:** Extract the call into a named local variable first, then pass the variable.
Comment thread
aliaska-varieva marked this conversation as resolved.
Resolved
}

/**
* Rotation replaces the refresh token, and the client only learns the new one from a response it
* may never receive: a cancelled request or a dropped connection, while the server still finishes.
* Keeping the replaced token for a short window lets that client's next refresh recover the session
* instead of ending it. A list rather than one slot, so a retry that rotates again doesn't kill the
* token a concurrent request has just received.
*/
private List<RetiredRefreshToken> retiredTokensAfterSaving(MongoOAuth2Authorization entity) {
MongoOAuth2Authorization stored = repository.findById(entity.getId()).orElse(null);

Check notice on line 56 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View check run for this annotation

Flamingo / Flamingo Code Review

entity.getId() called inline as argument to findById

entity.getId() called inline as argument to findById repository.findById(entity.getId()) calls entity.getId() directly inside findById's argument list, violating OFJAVA-002. **What it risks:** Stack traces and breakpoints on this line don't surface a named value for the id being looked up. **How to fix:** Extract entity.getId() into a named local variable before calling findById.
Comment thread
aliaska-varieva marked this conversation as resolved.
Resolved
if (stored == null) {
return null;
}
Instant now = Instant.now();
List<RetiredRefreshToken> retired = new ArrayList<>();
if (stored.getRetiredRefreshTokens() != null) {
stored.getRetiredRefreshTokens().stream()
.filter(token -> isWithinGrace(token, now))
.filter(token -> !token.value().equals(entity.getRefreshTokenValue()))
.forEach(retired::add);
}
String replaced = stored.getRefreshTokenValue();
if (replaced != null && !replaced.equals(entity.getRefreshTokenValue())) {
retired.add(new RetiredRefreshToken(replaced, now));
}
return retired.isEmpty() ? null : retired;
}

private boolean isWithinGrace(RetiredRefreshToken token, Instant now) {
return token.retiredAt().plusSeconds(refreshTokenGraceSeconds).isAfter(now);
}

@Override
public void remove(OAuth2Authorization authorization) {
repository.deleteById(authorization.getId());
Expand Down Expand Up @@ -97,10 +138,16 @@
.or(() -> repository.findByRefreshTokenValue(token))
.or(() -> repository.findByAuthorizationCodeValue(token))
.or(() -> repository.findByState(token));
if (found.isEmpty()) {
return findByRetiredRefreshToken(token);
}
} else if (OAuth2TokenType.ACCESS_TOKEN.equals(tokenType)) {
found = repository.findByAccessTokenValue(token);
} else if (OAuth2TokenType.REFRESH_TOKEN.equals(tokenType)) {
found = repository.findByRefreshTokenValue(token);
if (found.isEmpty()) {
return findByRetiredRefreshToken(token);
}
} else if (AUTH_CODE.equals(tokenType)) {
found = repository.findByAuthorizationCodeValue(token);
} else {
Expand All @@ -110,6 +157,28 @@
return found.map(entity -> MongoAuthorizationMapper.toDomain(entity, registeredClientRepository))
.orElse(null);
}

/**
* The presented value goes onto the returned authorization because Spring's revocation looks the
* token up by value on it; the metadata stays the current token's, so a revoked session stays revoked.
*/
private OAuth2Authorization findByRetiredRefreshToken(String token) {
Instant now = Instant.now();
return repository.findByRetiredRefreshTokensValue(token)
.filter(entity -> entity.getRetiredRefreshTokens().stream()
.anyMatch(retired -> retired.value().equals(token) && isWithinGrace(retired, now)))

Check notice on line 169 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View check run for this annotation

Flamingo / Flamingo Code Review

Compound condition inline in retired-token filter lambda

Compound condition inline in retired-token filter lambda The filter predicate `retired.value().equals(token) && isWithinGrace(retired, now)` combines two conditions with && directly inside the stream lambda instead of being extracted into a named private predicate method, as OFJAVA-007 requires for any condition containing &&. **What it risks:** The condition's intent (matching AND still-valid) is only recoverable by reading the lambda body, and it can't be reused or unit-tested on its own. **How to fix:** Extract a private boolean method, e.g. isAcceptableRetiredToken(RetiredRefreshToken retired, String token, Instant now), and call it from the filter.
Comment thread
aliaska-varieva marked this conversation as resolved.
Resolved
.map(entity -> {
log.info("Accepted a rotated refresh token within the grace window: authorization {}", entity.getId());
OAuth2Authorization authorization = MongoAuthorizationMapper.toDomain(entity, registeredClientRepository);
OAuth2Authorization.Token<OAuth2RefreshToken> current = authorization.getRefreshToken();
OAuth2RefreshToken presented = new OAuth2RefreshToken(
token, current.getToken().getIssuedAt(), current.getToken().getExpiresAt());
return OAuth2Authorization.from(authorization)
.token(presented, metadata -> metadata.putAll(current.getMetadata()))

Check notice on line 177 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

Metadata copy on the retired token looks redundant

Removing metadata.putAll(current.getMetadata()) survives every test, including the ones that revoke a session and expect the retired token to be inactive. That suggests OAuth2Authorization.Builder.token already keeps the replaced token's metadata, so the line may do nothing. Needs verification against the Spring builder. Risk: Low. If the line is redundant it is only noise, and the guarantee it appears to give actually rests on framework behaviour. Fix: Confirm the builder behaviour, then drop the consumer and call .token(presented), or keep it and say in the comment why.

Check warning on line 177 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View check run for this annotation

Flamingo / Flamingo Code Review / tests and mutation

Mutant survived: VoidMethodCallMutator

removed call to java/util/Map::putAll (in lambda$findByRetiredRefreshToken$3). A test runs this line, but none fails when it changes.
Comment thread
aliaska-varieva marked this conversation as resolved.
Resolved
.build();

Check notice on line 178 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View workflow job for this annotation

GitHub Actions / Flamingo Code Review

Rotating a refresh token reuses the old token's expiry and metadata, extending token lifetime for revoked sessions

Rotating a refresh token reuses the old token's expiry and metadata, extending token lifetime for revoked sessions In findByRetiredRefreshToken, the authorization's refresh-token metadata (including the INVALIDATED_METADATA_NAME flag set on revoke) is copied forward with metadata.putAll(current.getMetadata()), and that is correct for revoked sessions (the retired token stays revoked). However the presented (retired) token is wrapped as `new OAuth2RefreshToken(token, current.getToken().getIssuedAt(), current.getToken().getExpiresAt())`, i.e. the retired token's issuedAt/expiresAt are overwritten with the CURRENT token's instants. This means a retired token, which may be close to or past its own original expiry, is reported as valid/expiry-matching the current token for as long as the current token lives, not for the actual grace window (refreshTokenGraceSeconds). The only real boundary enforced is the isWithinGrace check at lookup time, which is correct for acceptance, but any other code path that inspects the returned OAuth2Authorization's refresh token expiry (e.g. logging, metrics, or future features built on this type) will see the wrong expiry for what is actually a retired token. **What it risks:** Code relying on the refresh token's reported issuedAt/expiresAt for the object returned by findByRetiredRefreshToken (anything other than value-based lookup) will be misled into thinking the retired/rotated token has the same lifetime as the live one, understating that it is actually a short grace-window artifact. **How to fix:** Document/limit this to Spring's revocation-by-value path only, or carry the original retiredAt-derived instants into the synthesized OAuth2RefreshToken instead of reusing the current token's expiresAt, so a consumer reading the token's expiry sees a value consistent with the grace window.
Comment thread
aliaska-varieva marked this conversation as resolved.
Resolved
})

Check warning on line 179 in openframe-authorization-service-core/src/main/java/com/openframe/authz/service/auth/MongoAuthorizationService.java

View check run for this annotation

Flamingo / Flamingo Code Review

Multi-line lambda body in findByRetiredRefreshToken's .map()

Multi-line lambda body in findByRetiredRefreshToken's .map() The .map(entity -> { ... }) lambda in findByRetiredRefreshToken spans multiple statements (logging, variable assignment, construction of a new token, building a new OAuth2Authorization), violating OFJAVA-004's requirement that lambda bodies fit on one line and be extracted to a named method otherwise. **What it risks:** The rotated-token acceptance logic loses a named stack frame, making it harder to debug and test in isolation from the Optional pipeline. **How to fix:** Extract the lambda body into a private method, e.g. toAuthorizationWithPresentedToken(entity, token), and pass a method reference.
Comment thread
aliaska-varieva marked this conversation as resolved.
Resolved
.orElse(null);
}
}


Original file line number Diff line number Diff line change
@@ -0,0 +1,231 @@
package com.openframe.authz.service.auth;

import com.openframe.data.document.oauth.MongoOAuth2Authorization;
import com.openframe.data.document.oauth.RetiredRefreshToken;
import com.openframe.data.repository.oauth.MongoOAuth2AuthorizationRepository;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
import org.mockito.quality.Strictness;
import org.springframework.security.oauth2.core.AuthorizationGrantType;
import org.springframework.security.oauth2.core.ClientAuthenticationMethod;
import org.springframework.security.oauth2.core.OAuth2RefreshToken;
import org.springframework.security.oauth2.server.authorization.OAuth2Authorization;
import org.springframework.security.oauth2.server.authorization.OAuth2TokenType;
import org.springframework.security.oauth2.server.authorization.client.RegisteredClient;
import org.springframework.security.oauth2.server.authorization.client.RegisteredClientRepository;
import org.springframework.test.util.ReflectionTestUtils;

import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.LENIENT)
class MongoAuthorizationServiceRefreshGraceTest {

private static final String ID = "authorization-1";
private static final String PRINCIPAL = "user@example.com";
private static final long GRACE_SECONDS = 60;

@Mock
private MongoOAuth2AuthorizationRepository repository;
@Mock
private RegisteredClientRepository clients;

private final Map<String, MongoOAuth2Authorization> store = new HashMap<>();
private MongoAuthorizationService service;

@BeforeEach
void setUp() {
when(clients.findById(anyString())).thenReturn(RegisteredClient.withId("client-1")
.clientId("client")
.clientAuthenticationMethod(ClientAuthenticationMethod.CLIENT_SECRET_BASIC)
.authorizationGrantType(AuthorizationGrantType.REFRESH_TOKEN)
.build());
when(repository.save(any())).thenAnswer(call -> {
MongoOAuth2Authorization entity = call.getArgument(0);
store.put(entity.getId(), entity);
return entity;
});
when(repository.findById(anyString()))
.thenAnswer(call -> Optional.ofNullable(store.get(call.<String>getArgument(0))));
when(repository.findByRefreshTokenValue(anyString())).thenAnswer(call -> store.values().stream()
.filter(e -> call.<String>getArgument(0).equals(e.getRefreshTokenValue()))
.findFirst());
when(repository.findByRetiredRefreshTokensValue(anyString())).thenAnswer(call -> store.values().stream()
.filter(e -> retiredValues().contains(call.<String>getArgument(0)))
.findFirst());
when(repository.findByAccessTokenValue(anyString())).thenReturn(Optional.empty());
when(repository.findByAuthorizationCodeValue(anyString())).thenReturn(Optional.empty());
when(repository.findByState(anyString())).thenReturn(Optional.empty());
when(repository.findAllByPrincipalNameIgnoreCaseAndRefreshTokenValueNotNull(PRINCIPAL))
.thenAnswer(call -> new ArrayList<>(store.values()));

service = new MongoAuthorizationService(repository, clients);
ReflectionTestUtils.setField(service, "refreshTokenGraceSeconds", GRACE_SECONDS);
store.put(ID, authorizationWithRefreshToken("token-0"));
}

private static MongoOAuth2Authorization authorizationWithRefreshToken(String token) {
Instant now = Instant.now();
return MongoOAuth2Authorization.builder()
.id(ID)
.registeredClientId("client-1")
.principalName(PRINCIPAL)
.authorizationGrantType(AuthorizationGrantType.AUTHORIZATION_CODE.getValue())
.refreshTokenValue(token)
.refreshTokenIssuedAt(now)
.refreshTokenExpiresAt(now.plus(7, ChronoUnit.DAYS))
.refreshTokenMetadata(new HashMap<>())
.build();
}

private MongoOAuth2Authorization stored() {
return store.get(ID);
}

private List<String> retiredValues() {
List<RetiredRefreshToken> retired = stored().getRetiredRefreshTokens();
return retired == null ? List.of() : retired.stream().map(RetiredRefreshToken::value).toList();
}

private OAuth2Authorization find(String token) {
return service.findByToken(token, OAuth2TokenType.REFRESH_TOKEN);
}

private void rotate(String presented, String replacement) {
Instant now = Instant.now();
service.save(OAuth2Authorization.from(find(presented))
.token(new OAuth2RefreshToken(replacement, now, now.plus(7, ChronoUnit.DAYS)))
.build());
}

private void retire(String value, long secondsAgo) {
stored().setRetiredRefreshTokens(new ArrayList<>(List.of(
new RetiredRefreshToken(value, Instant.now().minusSeconds(secondsAgo)))));
}

@Test
void rejectsARetiredTokenOnceTheGraceWindowHasPassed() {
retire("expired", GRACE_SECONDS + 1);

assertThat(find("expired")).isNull();
}

@Test
void acceptsARetiredTokenJustInsideTheGraceWindow() {
retire("recent", GRACE_SECONDS - 5);

assertThat(find("recent")).isNotNull();
}

@Test
void rejectsATokenThatWasNeverIssued() {
assertThat(find("never-issued")).isNull();
}

@Test
void aRetryThatRotatesAgainKeepsEveryRetiredTokenUsable() {
rotate("token-0", "token-1");
rotate("token-0", "token-2");

assertThat(stored().getRefreshTokenValue()).isEqualTo("token-2");
assertThat(retiredValues()).containsExactlyInAnyOrder("token-0", "token-1");
assertThat(find("token-0").getRefreshToken().isActive()).isTrue();
assertThat(find("token-1").getRefreshToken().isActive()).isTrue();
}

@Test
void aRetiredTokenCarriesItsOwnValueSoSpringRevocationCanLookItUp() {
rotate("token-0", "token-1");

assertThat(find("token-0").getToken("token-0")).isNotNull();
}

@Test
void aRetiredTokenKeepsTheCurrentExpiry() {
rotate("token-0", "token-1");

assertThat(find("token-0").getRefreshToken().getToken().getExpiresAt())
.isEqualTo(stored().getRefreshTokenExpiresAt());
}

@Test
void revokingTheSessionAlsoEndsItsRetiredTokens() {
rotate("token-0", "token-1");

service.revokeAllForPrincipal(PRINCIPAL);

assertThat(find("token-0").getRefreshToken().isActive()).isFalse();
}

@Test
void revokingThroughARetiredTokenLeavesNoActiveTokenBehind() {
rotate("token-0", "token-1");
OAuth2Authorization byRetired = find("token-0");

service.save(OAuth2Authorization.from(byRetired)
.token(byRetired.getToken("token-0").getToken(),
metadata -> metadata.put(OAuth2Authorization.Token.INVALIDATED_METADATA_NAME, true))
.build());

assertThat(find("token-0").getRefreshToken().isActive()).isFalse();
assertThat(find("token-1").getRefreshToken().isActive()).isFalse();
assertThat(retiredValues()).containsExactly("token-1");
}

@Test
void savingWithoutRotatingRetiresNothingAndKeepsWhatIsRetired() {
retire("recent", 1);

service.save(find("token-0"));

assertThat(retiredValues()).containsExactly("recent");
}

@Test
void savingDropsRetiredTokensPastTheirWindow() {
retire("expired", GRACE_SECONDS + 1);

rotate("token-0", "token-1");

assertThat(retiredValues()).containsExactly("token-0");
}

@Test
void savingWithNothingRetiredStoresNoList() {
service.save(find("token-0"));

assertThat(stored().getRetiredRefreshTokens()).isNull();
}

@Test
void savingANewAuthorizationStoresNoList() {
OAuth2Authorization existing = find("token-0");
store.clear();

service.save(existing);

assertThat(stored().getRetiredRefreshTokens()).isNull();
}

@Test
void anUntypedLookupAlsoResolvesARetiredToken() {
rotate("token-0", "token-1");

assertThat(service.findByToken("token-0", null)).isNotNull();
}
}
Loading
Loading