From 6cc4bcd13dd5c432a27f9587412e92b35fa5e98c Mon Sep 17 00:00:00 2001 From: Vedran Pavic Date: Fri, 7 Sep 2018 07:46:29 +0200 Subject: [PATCH] Verify session existence before update in ReactiveRedisOperationsSessionRepository Currently, ReactiveRedisOperationsSessionRepository#save does not ensure session's existence before executing update. This can result in an invalid session record in Redis, since write use only delta, and in turn to error while retrieving the invalid session record. This commit adds check for session existence if session is being updated. Closes gh-1111 --- ...edisOperationsSessionRepositoryITests.java | 24 ++++++++ ...ctiveRedisOperationsSessionRepository.java | 60 +++++++++++-------- ...RedisOperationsSessionRepositoryTests.java | 8 +++ 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryITests.java b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryITests.java index 69861eb8..816cef94 100644 --- a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryITests.java +++ b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryITests.java @@ -16,6 +16,8 @@ package org.springframework.session.data.redis; +import java.time.Instant; + import org.junit.Test; import org.junit.runner.RunWith; @@ -191,6 +193,28 @@ public class ReactiveRedisOperationsSessionRepositoryITests extends AbstractRedi assertThat(this.repository.findById(originalId).block()).isNull(); } + // gh-1111 + @Test + public void changeSessionSaveOldSessionInstance() { + ReactiveRedisOperationsSessionRepository.RedisSession toSave = this.repository + .createSession().block(); + String sessionId = toSave.getId(); + + this.repository.save(toSave).block(); + + ReactiveRedisOperationsSessionRepository.RedisSession session = this.repository + .findById(sessionId).block(); + session.changeSessionId(); + session.setLastAccessedTime(Instant.now()); + this.repository.save(session).block(); + + toSave.setLastAccessedTime(Instant.now()); + this.repository.save(toSave).block(); + + assertThat(this.repository.findById(sessionId).block()).isNull(); + assertThat(this.repository.findById(session.getId()).block()).isNotNull(); + } + @Configuration @EnableRedisWebSession static class Config extends BaseConfig { diff --git a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepository.java b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepository.java index 2b922563..379ed29d 100644 --- a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepository.java +++ b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepository.java @@ -143,24 +143,39 @@ public class ReactiveRedisOperationsSessionRepository implements @Override public Mono save(RedisSession session) { - return session.saveDelta().and((s) -> { - if (session.isNew) { - session.setNew(false); - } - - s.onComplete(); - }); + Mono result = session.saveChangeSessionId().and(session.saveDelta()) + .and((s) -> { + session.isNew = false; + s.onComplete(); + }); + if (session.isNew) { + return result; + } + else if (session.hasChangedSessionId()) { + String sessionKey = getSessionKey(session.originalSessionId); + return this.sessionRedisOperations.hasKey(sessionKey) + .flatMap((exists) -> exists ? result : Mono.empty()); + } + else { + String sessionKey = getSessionKey(session.getId()); + return this.sessionRedisOperations.hasKey(sessionKey) + .flatMap((exists) -> exists ? result : Mono.empty()); + } } @Override public Mono findById(String id) { String sessionKey = getSessionKey(id); + // @formatter:off return this.sessionRedisOperations.opsForHash().entries(sessionKey) .collectMap((e) -> e.getKey().toString(), Map.Entry::getValue) - .filter((map) -> !map.isEmpty()).map(new SessionMapper(id)) - .filter((session) -> !session.isExpired()).map(RedisSession::new) + .filter((map) -> !map.isEmpty()) + .map(new SessionMapper(id)) + .filter((session) -> !session.isExpired()) + .map(RedisSession::new) .switchIfEmpty(Mono.defer(() -> deleteById(id).then(Mono.empty()))); + // @formatter:on } @Override @@ -285,12 +300,8 @@ public class ReactiveRedisOperationsSessionRepository implements return this.cached.isExpired(); } - public void setNew(boolean isNew) { - this.isNew = isNew; - } - - public boolean isNew() { - return this.isNew; + private boolean hasChangedSessionId() { + return !getId().equals(this.originalSessionId); } private void flushImmediateIfNecessary() { @@ -305,38 +316,35 @@ public class ReactiveRedisOperationsSessionRepository implements } private Mono saveDelta() { - String sessionId = getId(); - Mono changeSessionId = saveChangeSessionId(sessionId); - if (this.delta.isEmpty()) { - return changeSessionId.and(Mono.empty()); + return Mono.empty(); } - String sessionKey = getSessionKey(sessionId); - + String sessionKey = getSessionKey(getId()); Mono update = ReactiveRedisOperationsSessionRepository.this.sessionRedisOperations .opsForHash().putAll(sessionKey, this.delta); - Mono setTtl = ReactiveRedisOperationsSessionRepository.this.sessionRedisOperations .expire(sessionKey, getMaxInactiveInterval()); - return changeSessionId.and(update).and(setTtl).and((s) -> { + return update.and(setTtl).and((s) -> { this.delta.clear(); s.onComplete(); }).then(); } - private Mono saveChangeSessionId(String sessionId) { - if (sessionId.equals(this.originalSessionId)) { + private Mono saveChangeSessionId() { + if (!hasChangedSessionId()) { return Mono.empty(); } + String sessionId = getId(); + Publisher replaceSessionId = (s) -> { this.originalSessionId = sessionId; s.onComplete(); }; - if (isNew()) { + if (this.isNew) { return Mono.from(replaceSessionId); } else { diff --git a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryTests.java b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryTests.java index a4b52a1b..0530e435 100644 --- a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryTests.java +++ b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryTests.java @@ -183,6 +183,7 @@ public class ReactiveRedisOperationsSessionRepositoryTests { @Test public void saveSessionNothingChanged() { + given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true)); given(this.redisOperations.expire(anyString(), any())) .willReturn(Mono.just(true)); @@ -191,12 +192,14 @@ public class ReactiveRedisOperationsSessionRepositoryTests { StepVerifier.create(this.repository.save(session)).verifyComplete(); + verify(this.redisOperations).hasKey(anyString()); verifyZeroInteractions(this.redisOperations); verifyZeroInteractions(this.hashOperations); } @Test public void saveLastAccessChanged() { + given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true)); given(this.redisOperations.opsForHash()).willReturn(this.hashOperations); given(this.hashOperations.putAll(anyString(), any())).willReturn(Mono.just(true)); given(this.redisOperations.expire(anyString(), any())) @@ -206,6 +209,7 @@ public class ReactiveRedisOperationsSessionRepositoryTests { session.setLastAccessedTime(Instant.ofEpochMilli(12345678L)); Mono.just(session).subscribe(this.repository::save); + verify(this.redisOperations).hasKey(anyString()); verify(this.redisOperations).opsForHash(); verify(this.hashOperations).putAll(anyString(), this.delta.capture()); verify(this.redisOperations).expire(anyString(), any()); @@ -219,6 +223,7 @@ public class ReactiveRedisOperationsSessionRepositoryTests { @Test public void saveSetAttribute() { + given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true)); given(this.redisOperations.opsForHash()).willReturn(this.hashOperations); given(this.hashOperations.putAll(anyString(), any())).willReturn(Mono.just(true)); given(this.redisOperations.expire(anyString(), any())) @@ -229,6 +234,7 @@ public class ReactiveRedisOperationsSessionRepositoryTests { session.setAttribute(attrName, "attrValue"); Mono.just(session).subscribe(this.repository::save); + verify(this.redisOperations).hasKey(anyString()); verify(this.redisOperations).opsForHash(); verify(this.hashOperations).putAll(anyString(), this.delta.capture()); verify(this.redisOperations).expire(anyString(), any()); @@ -242,6 +248,7 @@ public class ReactiveRedisOperationsSessionRepositoryTests { @Test public void saveRemoveAttribute() { + given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true)); given(this.redisOperations.opsForHash()).willReturn(this.hashOperations); given(this.hashOperations.putAll(anyString(), any())).willReturn(Mono.just(true)); given(this.redisOperations.expire(anyString(), any())) @@ -252,6 +259,7 @@ public class ReactiveRedisOperationsSessionRepositoryTests { session.removeAttribute(attrName); Mono.just(session).subscribe(this.repository::save); + verify(this.redisOperations).hasKey(anyString()); verify(this.redisOperations).opsForHash(); verify(this.hashOperations).putAll(anyString(), this.delta.capture()); verify(this.redisOperations).expire(anyString(), any());