Copilot commented on code in PR #2711:
URL: https://github.com/apache/shiro/pull/2711#discussion_r3293734275
##########
core/src/main/java/org/apache/shiro/mgt/DefaultSecurityManager.java:
##########
@@ -302,7 +305,17 @@ public Subject login(Subject subject, AuthenticationToken
token) throws Authenti
* @param subject Subject
*/
protected void beforeSuccessfulLogin(Subject subject) {
- stopSession(subject);
+ Session session = subject.getSession(false);
+ if (session != null) {
+ Map<Object, Object> attributes = new HashMap<>();
+ session.getAttributeKeys().forEach(key -> attributes.put(key,
session.getAttribute(key)));
+ stopSession(subject);
Review Comment:
The attribute snapshot logic calls session.getAttribute(key) for every key
returned by getAttributeKeys(). When the Session is a DelegatingSession (the
default exposed session for native session management), each getAttribute call
re-loads the session via the SessionManager, resulting in an N+1 pattern during
login. Consider adding a bulk attribute snapshot mechanism (e.g., a
SessionManager method that returns a copy of the underlying attributes map in
one lookup) or otherwise avoiding repeated sessionManager lookups here.
##########
core/src/test/java/org/apache/shiro/subject/DelegatingSubjectTest.java:
##########
@@ -223,6 +226,36 @@ void testRunAs() {
LifecycleUtils.destroy(sm);
}
+ @Test
+ void sessionAttributesSurviveLoginSessionRotation() {
+ Ini ini = new Ini();
+ Ini.Section users = ini.addSection("users");
+ users.put("user1", "user1,role1");
+ users.put("user2", "user2,role2");
+ users.put("user3", "user3,role3");
+ SecurityManager sm = new BasicIniEnvironment(ini).getSecurityManager();
+ Subject subject = new Subject.Builder(sm).buildSubject();
+
+ subject.login(new UsernamePasswordToken("user1", "user1"));
+ subject.logout();
+
+ Session preLoginSession = subject.getSession(true);
+ preLoginSession.setAttribute("tenantId", "ACME");
+ Serializable preLoginSessionId = preLoginSession.getId();
+
+ subject.login(new UsernamePasswordToken("user1", "user1"));
+ assertThat(subject.isAuthenticated()).isTrue();
+
+ Session postLoginSession = subject.getSession(false);
+ assertThat(postLoginSession).isNotNull();
+
+ assertThat(preLoginSessionId).as("session ID should change on login
(session fixation protection)")
+ .isNotEqualTo(postLoginSession.getId());
+ assertThat(postLoginSession.getAttribute("tenantId"))
+ .as("session attributes set before login must survive session
rotation")
+ .isEqualTo("ACME");
Review Comment:
This test creates a SecurityManager via BasicIniEnvironment but does not
destroy it. Other tests in this class call LifecycleUtils.destroy(sm) to avoid
leaking resources across tests; consider adding similar cleanup (ideally in a
try/finally) here as well.
##########
core/src/main/java/org/apache/shiro/mgt/DefaultSecurityManager.java:
##########
@@ -302,7 +305,17 @@ public Subject login(Subject subject, AuthenticationToken
token) throws Authenti
* @param subject Subject
*/
protected void beforeSuccessfulLogin(Subject subject) {
- stopSession(subject);
+ Session session = subject.getSession(false);
+ if (session != null) {
+ Map<Object, Object> attributes = new HashMap<>();
+ session.getAttributeKeys().forEach(key -> attributes.put(key,
session.getAttribute(key)));
+ stopSession(subject);
+ var newSession = subject.getSession();
+ var keys = newSession.getAttributeKeys();
Review Comment:
beforeSuccessfulLogin() now calls subject.getSession() after stopping the
existing session. If the Subject has sessionCreationEnabled=false but still has
an existing session (e.g., resolved from an incoming session id),
DelegatingSubject.getSession(true) will throw DisabledSessionException and
break login. Consider guarding this by only recreating/restoring when session
creation is enabled (if detectable) or catching DisabledSessionException and
skipping the restore in that case.
##########
core/src/main/java/org/apache/shiro/mgt/DefaultSecurityManager.java:
##########
@@ -603,6 +616,9 @@ protected void stopSession(Subject subject) {
Session s = subject.getSession(false);
if (s != null) {
s.stop();
+ if (subject instanceof DelegatingSubject) {
+ ((DelegatingSubject) subject).sessionStopped();
+ }
Review Comment:
stopSession() now depends on DelegatingSubject.sessionStopped() and forces
that method to be public. DelegatingSubject already wraps sessions in
StoppingAwareProxiedSession, whose stop() implementation calls
owner.sessionStopped(), so this extra cast/call is redundant and expands the
public API surface. Consider removing the DelegatingSubject dependency here and
keeping sessionStopped non-public (or otherwise documenting/isolating it as an
internal hook).
##########
core/src/main/java/org/apache/shiro/subject/support/DelegatingSubject.java:
##########
@@ -379,7 +379,7 @@ public void logout() {
}
}
- private void sessionStopped() {
+ public void sessionStopped() {
Review Comment:
sessionStopped() was changed from private to public, but it appears to be an
internal lifecycle callback (it is invoked by the internal
StoppingAwareProxiedSession on Session.stop()). Exposing it publicly makes it
easy for callers to put a Subject into an inconsistent state. Consider keeping
it non-public and adjusting DefaultSecurityManager.stopSession() to avoid
needing direct access to this method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
