Skip to content

Commit 7e8f500

Browse files
vpavicvpavic
committed
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-1185
1 parent b27742c commit 7e8f500

File tree

3 files changed

+66
-26
lines changed

3 files changed

+66
-26
lines changed

‎spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryITests.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package org.springframework.session.data.redis;
1818

19+
import java.time.Instant;
20+
1921
import org.junit.Test;
2022
import org.junit.runner.RunWith;
2123

@@ -191,6 +193,28 @@ public void changeSessionIdSaveTwice() {
191193
assertThat(this.repository.findById(originalId).block()).isNull();
192194
}
193195

196+
// gh-1111
197+
@Test
198+
public void changeSessionSaveOldSessionInstance() {
199+
ReactiveRedisOperationsSessionRepository.RedisSession toSave = this.repository
200+
.createSession().block();
201+
String sessionId = toSave.getId();
202+
203+
this.repository.save(toSave).block();
204+
205+
ReactiveRedisOperationsSessionRepository.RedisSession session = this.repository
206+
.findById(sessionId).block();
207+
session.changeSessionId();
208+
session.setLastAccessedTime(Instant.now());
209+
this.repository.save(session).block();
210+
211+
toSave.setLastAccessedTime(Instant.now());
212+
this.repository.save(toSave).block();
213+
214+
assertThat(this.repository.findById(sessionId).block()).isNull();
215+
assertThat(this.repository.findById(session.getId()).block()).isNotNull();
216+
}
217+
194218
@Configuration
195219
@EnableRedisWebSession
196220
static class Config extends BaseConfig {

‎spring-session-data-redis/src/main/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepository.java

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -134,24 +134,39 @@ public Mono<RedisSession> createSession() {
134134

135135
@Override
136136
public Mono<Void> save(RedisSession session) {
137-
return session.saveDelta().and((s) -> {
138-
if (session.isNew) {
139-
session.setNew(false);
140-
}
141-
142-
s.onComplete();
143-
});
137+
Mono<Void> result = session.saveChangeSessionId().and(session.saveDelta())
138+
.and((s) -> {
139+
session.isNew = false;
140+
s.onComplete();
141+
});
142+
if (session.isNew) {
143+
return result;
144+
}
145+
else if (session.hasChangedSessionId()) {
146+
String sessionKey = getSessionKey(session.originalSessionId);
147+
return this.sessionRedisOperations.hasKey(sessionKey)
148+
.flatMap((exists) -> exists ? result : Mono.empty());
149+
}
150+
else {
151+
String sessionKey = getSessionKey(session.getId());
152+
return this.sessionRedisOperations.hasKey(sessionKey)
153+
.flatMap((exists) -> exists ? result : Mono.empty());
154+
}
144155
}
145156

146157
@Override
147158
public Mono<RedisSession> findById(String id) {
148159
String sessionKey = getSessionKey(id);
149160

161+
// @formatter:off
150162
return this.sessionRedisOperations.opsForHash().entries(sessionKey)
151163
.collectMap((e) -> e.getKey().toString(), Map.Entry::getValue)
152-
.filter((map) -> !map.isEmpty()).map(new SessionMapper(id))
153-
.filter((session) -> !session.isExpired()).map(RedisSession::new)
164+
.filter((map) -> !map.isEmpty())
165+
.map(new SessionMapper(id))
166+
.filter((session) -> !session.isExpired())
167+
.map(RedisSession::new)
154168
.switchIfEmpty(Mono.defer(() -> deleteById(id).then(Mono.empty())));
169+
// @formatter:on
155170
}
156171

157172
@Override
@@ -276,12 +291,8 @@ public boolean isExpired() {
276291
return this.cached.isExpired();
277292
}
278293

279-
public void setNew(boolean isNew) {
280-
this.isNew = isNew;
281-
}
282-
283-
public boolean isNew() {
284-
return this.isNew;
294+
private boolean hasChangedSessionId() {
295+
return !getId().equals(this.originalSessionId);
285296
}
286297

287298
private void flushImmediateIfNecessary() {
@@ -296,38 +307,35 @@ private void putAndFlush(String a, Object v) {
296307
}
297308

298309
private Mono<Void> saveDelta() {
299-
String sessionId = getId();
300-
Mono<Void> changeSessionId = saveChangeSessionId(sessionId);
301-
302310
if (this.delta.isEmpty()) {
303-
return changeSessionId.and(Mono.empty());
311+
return Mono.empty();
304312
}
305313

306-
String sessionKey = getSessionKey(sessionId);
307-
314+
String sessionKey = getSessionKey(getId());
308315
Mono<Boolean> update = ReactiveRedisOperationsSessionRepository.this.sessionRedisOperations
309316
.opsForHash().putAll(sessionKey, this.delta);
310-
311317
Mono<Boolean> setTtl = ReactiveRedisOperationsSessionRepository.this.sessionRedisOperations
312318
.expire(sessionKey, getMaxInactiveInterval());
313319

314-
return changeSessionId.and(update).and(setTtl).and((s) -> {
320+
return update.and(setTtl).and((s) -> {
315321
this.delta.clear();
316322
s.onComplete();
317323
}).then();
318324
}
319325

320-
private Mono<Void> saveChangeSessionId(String sessionId) {
321-
if (sessionId.equals(this.originalSessionId)) {
326+
private Mono<Void> saveChangeSessionId() {
327+
if (!hasChangedSessionId()) {
322328
return Mono.empty();
323329
}
324330

331+
String sessionId = getId();
332+
325333
Publisher<Void> replaceSessionId = (s) -> {
326334
this.originalSessionId = sessionId;
327335
s.onComplete();
328336
};
329337

330-
if (isNew()) {
338+
if (this.isNew) {
331339
return Mono.from(replaceSessionId);
332340
}
333341
else {

‎spring-session-data-redis/src/test/java/org/springframework/session/data/redis/ReactiveRedisOperationsSessionRepositoryTests.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ public void saveNewSession() {
183183

184184
@Test
185185
public void saveSessionNothingChanged() {
186+
given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true));
186187
given(this.redisOperations.expire(anyString(), any()))
187188
.willReturn(Mono.just(true));
188189

@@ -191,12 +192,14 @@ public void saveSessionNothingChanged() {
191192

192193
StepVerifier.create(this.repository.save(session)).verifyComplete();
193194

195+
verify(this.redisOperations).hasKey(anyString());
194196
verifyZeroInteractions(this.redisOperations);
195197
verifyZeroInteractions(this.hashOperations);
196198
}
197199

198200
@Test
199201
public void saveLastAccessChanged() {
202+
given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true));
200203
given(this.redisOperations.opsForHash()).willReturn(this.hashOperations);
201204
given(this.hashOperations.putAll(anyString(), any())).willReturn(Mono.just(true));
202205
given(this.redisOperations.expire(anyString(), any()))
@@ -206,6 +209,7 @@ public void saveLastAccessChanged() {
206209
session.setLastAccessedTime(Instant.ofEpochMilli(12345678L));
207210
Mono.just(session).subscribe(this.repository::save);
208211

212+
verify(this.redisOperations).hasKey(anyString());
209213
verify(this.redisOperations).opsForHash();
210214
verify(this.hashOperations).putAll(anyString(), this.delta.capture());
211215
verify(this.redisOperations).expire(anyString(), any());
@@ -219,6 +223,7 @@ public void saveLastAccessChanged() {
219223

220224
@Test
221225
public void saveSetAttribute() {
226+
given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true));
222227
given(this.redisOperations.opsForHash()).willReturn(this.hashOperations);
223228
given(this.hashOperations.putAll(anyString(), any())).willReturn(Mono.just(true));
224229
given(this.redisOperations.expire(anyString(), any()))
@@ -229,6 +234,7 @@ public void saveSetAttribute() {
229234
session.setAttribute(attrName, "attrValue");
230235
Mono.just(session).subscribe(this.repository::save);
231236

237+
verify(this.redisOperations).hasKey(anyString());
232238
verify(this.redisOperations).opsForHash();
233239
verify(this.hashOperations).putAll(anyString(), this.delta.capture());
234240
verify(this.redisOperations).expire(anyString(), any());
@@ -242,6 +248,7 @@ public void saveSetAttribute() {
242248

243249
@Test
244250
public void saveRemoveAttribute() {
251+
given(this.redisOperations.hasKey(anyString())).willReturn(Mono.just(true));
245252
given(this.redisOperations.opsForHash()).willReturn(this.hashOperations);
246253
given(this.hashOperations.putAll(anyString(), any())).willReturn(Mono.just(true));
247254
given(this.redisOperations.expire(anyString(), any()))
@@ -252,6 +259,7 @@ public void saveRemoveAttribute() {
252259
session.removeAttribute(attrName);
253260
Mono.just(session).subscribe(this.repository::save);
254261

262+
verify(this.redisOperations).hasKey(anyString());
255263
verify(this.redisOperations).opsForHash();
256264
verify(this.hashOperations).putAll(anyString(), this.delta.capture());
257265
verify(this.redisOperations).expire(anyString(), any());

0 commit comments

Comments
 (0)