Skip to content

Commit 5d38823

Browse files
at055612at055612
committed
Refactor getSession and get/set attr calls
1 parent 7c2647e commit 5d38823

File tree

9 files changed

+186
-67
lines changed

9 files changed

+186
-67
lines changed

‎stroom-activity/stroom-activity-impl/src/main/java/stroom/activity/impl/CurrentActivityImpl.java‎

Lines changed: 3 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@
1919
import stroom.activity.api.CurrentActivity;
2020
import stroom.activity.shared.Activity;
2121
import stroom.security.api.SecurityContext;
22+
import stroom.util.servlet.SessionUtil;
2223

2324
import com.google.inject.Inject;
2425
import jakarta.inject.Provider;
2526
import jakarta.servlet.http.HttpServletRequest;
26-
import jakarta.servlet.http.HttpSession;
2727
import org.slf4j.Logger;
2828
import org.slf4j.LoggerFactory;
2929

@@ -49,17 +49,7 @@ public Activity getActivity() {
4949

5050
try {
5151
final HttpServletRequest request = httpServletRequestProvider.get();
52-
if (request != null) {
53-
final HttpSession session = request.getSession(false);
54-
if (session != null) {
55-
final Object object = session.getAttribute(NAME);
56-
if (object instanceof Activity) {
57-
activity = (Activity) object;
58-
}
59-
} else {
60-
LOGGER.debug("No Session");
61-
}
62-
}
52+
activity = SessionUtil.getAttribute(request, NAME, Activity.class, false);
6353
} catch (final RuntimeException e) {
6454
LOGGER.debug(e.getMessage(), e);
6555
}
@@ -72,14 +62,7 @@ public Activity getActivity() {
7262
public void setActivity(final Activity activity) {
7363
securityContext.secure(() -> {
7464
final HttpServletRequest request = httpServletRequestProvider.get();
75-
if (request != null) {
76-
final HttpSession session = request.getSession(false);
77-
if (session != null) {
78-
session.setAttribute(NAME, activity);
79-
} else {
80-
LOGGER.debug("No Session");
81-
}
82-
}
65+
SessionUtil.setAttribute(request, NAME, activity, false);
8366
});
8467
}
8568
}

‎stroom-resource/stroom-resource-impl/src/main/java/stroom/resource/impl/SessionResourceStoreImpl.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,11 @@
1717
package stroom.resource.impl;
1818

1919
import stroom.resource.api.ResourceStore;
20+
import stroom.util.logging.LambdaLogger;
21+
import stroom.util.logging.LambdaLoggerFactory;
22+
import stroom.util.logging.LogUtil;
2023
import stroom.util.servlet.HttpServletRequestHolder;
24+
import stroom.util.servlet.SessionUtil;
2125
import stroom.util.shared.IsServlet;
2226
import stroom.util.shared.ResourceKey;
2327

@@ -39,6 +43,8 @@
3943
*/
4044
public class SessionResourceStoreImpl extends HttpServlet implements ResourceStore, IsServlet {
4145

46+
private static final LambdaLogger LOGGER = LambdaLoggerFactory.getLogger(SessionResourceStoreImpl.class);
47+
4248
private static final Set<String> PATH_SPECS = Set.of("/resourcestore/*");
4349
private static final String UUID_ARG = "uuid";
4450
private static final String ZIP_EXTENSION = ".zip";
@@ -94,7 +100,9 @@ private ResourceMap getMap() {
94100
}
95101

96102
final String name = "SESSION_RESOURCE_STORE";
97-
final HttpSession session = request.getSession();
103+
final HttpSession session = SessionUtil.getOrCreateSession(request, newSession ->
104+
LOGGER.debug(() -> LogUtil.message("getMap() - Created session {}",
105+
SessionUtil.getSessionId(newSession))));
98106
final Object object = session.getAttribute(name);
99107
if (object == null) {
100108
resourceMap = new ResourceMap();

‎stroom-security/stroom-security-common-impl/src/main/java/stroom/security/common/impl/UserIdentitySessionUtil.java‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import stroom.util.logging.LambdaLogger;
55
import stroom.util.logging.LambdaLoggerFactory;
66
import stroom.util.logging.LogUtil;
7+
import stroom.util.servlet.SessionUtil;
78
import stroom.util.shared.NullSafe;
89

910
import jakarta.servlet.http.HttpSession;
@@ -24,7 +25,7 @@ private UserIdentitySessionUtil() {
2425
*/
2526
public static void set(final HttpSession session, final UserIdentity userIdentity) {
2627
LOGGER.debug(() -> LogUtil.message("Setting userIdentity of type {} in session {}, userIdentity: {}",
27-
NullSafe.get(userIdentity, UserIdentity::getClass, Class::getSimpleName),
28+
LogUtil.getSimpleClassName(userIdentity),
2829
NullSafe.get(session, HttpSession::getId),
2930
userIdentity));
3031
session.setAttribute(SESSION_USER_IDENTITY, userIdentity);
@@ -34,16 +35,16 @@ public static Optional<UserIdentity> get(final HttpSession session) {
3435
final Optional<UserIdentity> optUserIdentity = Optional.ofNullable(session)
3536
.map(session2 ->
3637
(UserIdentity) session2.getAttribute(SESSION_USER_IDENTITY));
38+
3739
if (LOGGER.isTraceEnabled()) {
3840
optUserIdentity.ifPresentOrElse(userIdentity -> {
3941
LOGGER.trace(() -> LogUtil.message("Got userIdentity of type {} in session {}, userIdentity: {}",
4042
NullSafe.get(userIdentity, UserIdentity::getClass, Class::getSimpleName),
41-
NullSafe.get(session, HttpSession::getId),
43+
SessionUtil.getSessionId(session),
4244
userIdentity));
43-
}, () -> {
44-
LOGGER.trace(() -> LogUtil.message("No userIdentity in session {}",
45-
NullSafe.get(session, HttpSession::getId)));
46-
});
45+
}, () ->
46+
LOGGER.trace(() ->
47+
LogUtil.message("No userIdentity in session {}", SessionUtil.getSessionId(session))));
4748
}
4849
return optUserIdentity;
4950
}

‎stroom-security/stroom-security-identity/src/main/java/stroom/security/identity/authenticate/AuthenticationServiceImpl.java‎

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import stroom.util.logging.LambdaLogger;
4040
import stroom.util.logging.LambdaLoggerFactory;
4141
import stroom.util.logging.LogUtil;
42+
import stroom.util.servlet.SessionUtil;
4243

4344
import event.logging.AuthenticateOutcomeReason;
4445
import jakarta.inject.Inject;
@@ -59,7 +60,7 @@ class AuthenticationServiceImpl implements AuthenticationService {
5960
private static final LambdaLogger LOGGER = LambdaLoggerFactory.getLogger(AuthenticationServiceImpl.class);
6061

6162
private static final long MIN_CREDENTIAL_CONFIRMATION_INTERVAL = 600000;
62-
private static final String AUTH_STATE = "AUTH_STATE";
63+
private static final String AUTH_STATE_ATTRIBUTE_NAME = "AUTH_STATE";
6364

6465
private final UriFactory uriFactory;
6566
private final IdentityConfig config;
@@ -93,23 +94,19 @@ public AuthenticationServiceImpl(
9394
this.certificateExtractor = certificateExtractor;
9495
}
9596

96-
private void setAuthState(final HttpSession session, final AuthStateImpl authStateImpl) {
97-
if (session != null) {
98-
session.setAttribute(AUTH_STATE, authStateImpl);
99-
}
97+
private void setAuthState(final HttpServletRequest request,
98+
final AuthStateImpl authStateImpl,
99+
final boolean createSession) {
100+
SessionUtil.setAttribute(request, AUTH_STATE_ATTRIBUTE_NAME, authStateImpl, createSession);
101+
100102
}
101103

102104
private AuthStateImpl getAuthState(final HttpServletRequest request) {
103-
if (request == null) {
104-
return null;
105-
}
106-
107-
final HttpSession session = request.getSession(false);
108-
if (session == null) {
109-
return null;
110-
}
111-
112-
return (AuthStateImpl) session.getAttribute(AUTH_STATE);
105+
return SessionUtil.getAttribute(
106+
request,
107+
AUTH_STATE_ATTRIBUTE_NAME,
108+
AuthStateImpl.class,
109+
false);
113110
}
114111

115112
@Override
@@ -183,7 +180,7 @@ private AuthStatus loginWithCertificate(final HttpServletRequest request) {
183180
account,
184181
false,
185182
System.currentTimeMillis());
186-
setAuthState(request.getSession(true), newState);
183+
setAuthState(request, newState, true);
187184

188185
// Reset last access, login failures, etc...
189186
accountDao.recordSuccessfulLogin(userId);
@@ -253,10 +250,9 @@ public ConfirmPasswordResponse confirmPassword(final HttpServletRequest request,
253250
final String message = result.toString();
254251
if (result.isAllOk()) {
255252
// Update tha last credential confirmation time.
256-
setAuthState(request.getSession(true),
257-
new AuthStateImpl(authState.getAccount(),
258-
authState.isRequirePasswordChange(),
259-
System.currentTimeMillis()));
253+
254+
final AuthStateImpl newAuthState = authState.withLastCredentialCheckMs(System.currentTimeMillis());
255+
setAuthState(request, newAuthState, true);
260256
return new ConfirmPasswordResponse(true, message);
261257
}
262258

@@ -302,8 +298,11 @@ public ChangePasswordResponse changePassword(final HttpServletRequest request,
302298
accountDao.changePassword(userId, changePasswordRequest.getNewPassword());
303299

304300
if (authState.getSubject().equals(userId)) {
305-
setAuthState(request.getSession(true),
306-
new AuthStateImpl(authState.getAccount(), false, System.currentTimeMillis()));
301+
final AuthStateImpl newAuthState = new AuthStateImpl(
302+
authState.getAccount(),
303+
false,
304+
System.currentTimeMillis());
305+
setAuthState(request, newAuthState, true);
307306
}
308307

309308
return new ChangePasswordResponse(true, null, false);
@@ -344,8 +343,11 @@ private LoginResponse processSuccessfulLogin(final HttpServletRequest request,
344343

345344
// Reset last access, login failures, etc...
346345
accountDao.recordSuccessfulLogin(userId);
347-
setAuthState(request.getSession(true),
348-
new AuthStateImpl(account, userNeedsToChangePassword, System.currentTimeMillis()));
346+
final AuthStateImpl newAuthState = new AuthStateImpl(
347+
account,
348+
userNeedsToChangePassword,
349+
System.currentTimeMillis());
350+
setAuthState(request, newAuthState, true);
349351

350352
return new LoginResponse(true, message, userNeedsToChangePassword);
351353
}
@@ -392,7 +394,7 @@ private Optional<String> getIdFromCertificate(final String cn) {
392394
}
393395

394396
private void clearSession(final HttpServletRequest request) {
395-
setAuthState(request.getSession(false), null);
397+
setAuthState(request, null, false);
396398
}
397399

398400
public PasswordPolicyConfig getPasswordPolicy() {
@@ -466,8 +468,16 @@ public boolean isRequirePasswordChange() {
466468
public long getLastCredentialCheckMs() {
467469
return lastCredentialCheckMs;
468470
}
471+
472+
public AuthStateImpl withLastCredentialCheckMs(final long lastCredentialCheckMs) {
473+
return new AuthStateImpl(account, requirePasswordChange, lastCredentialCheckMs);
474+
}
469475
}
470476

477+
478+
// --------------------------------------------------------------------------------
479+
480+
471481
private static class AuthStatusImpl implements AuthStatus {
472482

473483
private final AuthState state;

‎stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/OpenIdManager.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,6 @@ private RedirectUrl backChannelOIDC(final HttpServletRequest request,
125125
if (optionalUserIdentity.isPresent()) {
126126
// Set the token in the session so that when we re-direct to the initiating page (i.e. '/')
127127
// we will have the identity in session so won't go back round the code flow loop
128-
// UserIdentitySessionUtil.set(request.getSession(true), optionalUserIdentity.get());
129128

130129
// Successful login, so redirect to the original URL held in the state.
131130
final String uri = state.getInitiatingUri();

‎stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/SecurityFilter.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -143,9 +143,7 @@ private void filter(final HttpServletRequest request,
143143
chain.doFilter(request, response);
144144
} else {
145145
LOGGER.debug(() -> LogUtil.message("Session ID {}, request URI {}",
146-
Optional.ofNullable(request.getSession(false))
147-
.map(HttpSession::getId)
148-
.orElse("-"),
146+
SessionUtil.getSessionId(request),
149147
request.getRequestURI() + Optional.ofNullable(request.getQueryString())
150148
.map(str -> "/" + str)
151149
.orElse("")));
@@ -174,7 +172,7 @@ private void filter(final HttpServletRequest request,
174172

175173
// If no user from header token, see if we have one in session already.
176174
if (optUserIdentity.isEmpty()) {
177-
optUserIdentity = UserIdentitySessionUtil.get(request.getSession(false));
175+
optUserIdentity = UserIdentitySessionUtil.get(SessionUtil.getExistingSession(request));
178176
if (LOGGER.isDebugEnabled()) {
179177
logUserIdentityToDebug(optUserIdentity, fullPath, "from session");
180178
}
@@ -363,7 +361,10 @@ public void destroy() {
363361

364362
private Optional<HttpSession> ensureSessionIfCookiePresent(final HttpServletRequest request) {
365363
if (SessionUtil.requestHasSessionCookie(request)) {
366-
return Optional.of(request.getSession(true));
364+
final HttpSession session = SessionUtil.getOrCreateSession(request, newSession ->
365+
LOGGER.debug(() -> LogUtil.message("ensureSessionIfCookiePresent() - Created new session {}",
366+
SessionUtil.getSessionId(newSession))));
367+
return Optional.of(session);
367368
}
368369
return Optional.empty();
369370
}

‎stroom-security/stroom-security-impl/src/main/java/stroom/security/impl/StroomUserIdentityFactory.java‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import stroom.util.logging.LambdaLogger;
3737
import stroom.util.logging.LambdaLoggerFactory;
3838
import stroom.util.logging.LogUtil;
39+
import stroom.util.servlet.SessionUtil;
3940
import stroom.util.shared.Clearable;
4041
import stroom.util.shared.NullSafe;
4142
import stroom.util.shared.PermissionException;
@@ -322,7 +323,10 @@ private UserIdentity createAuthFlowUserIdentity(final JwtClaims jwtClaims,
322323
// At this point we have authenticated the User so need to ensure
323324
// we have a session as that gets attached to the userIdentity object to
324325
// allow us to refresh it's token.
325-
final HttpSession session = request.getSession(true);
326+
final HttpSession session = SessionUtil.getOrCreateSession(request, newSession -> {
327+
LOGGER.debug(() -> LogUtil.message("createAuthFlowUserIdentity() - Created new session {}",
328+
newSession.getId()));
329+
});
326330

327331
// Make a token object that we can update as/when we do a token refresh
328332
final UpdatableToken updatableToken = new UpdatableToken(
@@ -348,11 +352,11 @@ private UserIdentity createAuthFlowUserIdentity(final JwtClaims jwtClaims,
348352

349353
LOGGER.debug(() -> LogUtil.message(
350354
"createAuthFlowUserIdentity() - Authenticated user - session: {}, userIdentity: {}",
351-
NullSafe.get(session, HttpSession::getId),
355+
SessionUtil.getSessionId(session),
352356
userIdentity));
353357

354358
LOGGER.info(() -> "createAuthFlowUserIdentity() - Authenticated user " + userIdentity
355-
+ " for sessionId " + NullSafe.get(session, HttpSession::getId));
359+
+ " for sessionId " + SessionUtil.getSessionId(session));
356360
return userIdentity;
357361
}
358362

@@ -403,15 +407,11 @@ private static ApiUserIdentity createApiUserIdentity(final JwtContext jwtContext
403407
final String displayName,
404408
final String userUuid,
405409
final HttpServletRequest request) {
406-
Objects.requireNonNull(userId);
407-
408-
final HttpSession session = request.getSession(false);
409-
410410
return new ApiUserIdentity(
411411
userUuid,
412-
userId,
412+
Objects.requireNonNull(userId),
413413
displayName,
414-
NullSafe.get(session, HttpSession::getId),
414+
SessionUtil.getSessionId(request),
415415
jwtContext);
416416
}
417417

@@ -431,6 +431,7 @@ private Optional<UserIdentity> getProcessingUser(final JwtContext jwtContext) {
431431
public boolean isServiceUser(final String subject,
432432
final String issuer,
433433
final UserIdentity serviceUser) {
434+
434435
if (serviceUser instanceof final HasJwtClaims hasJwtClaims) {
435436
return Optional.ofNullable(hasJwtClaims.getJwtClaims())
436437
.map(ThrowingFunction.unchecked(jwtClaims -> {

‎stroom-util/src/main/java/stroom/util/logging/LogUtil.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,4 +492,12 @@ public static DurationTimer startTimerIfTraceEnabled(final Logger logger) {
492492
? DurationTimer.start()
493493
: null;
494494
}
495+
496+
public static String getSimpleClassName(final Object obj) {
497+
return NullSafe.get(obj, Object::getClass, Class::getSimpleName);
498+
}
499+
500+
public static String getClassName(final Object obj) {
501+
return NullSafe.get(obj, Object::getClass, Class::getName);
502+
}
495503
}

0 commit comments

Comments
 (0)