Jetty 12 - Added a core Session abstraction #9223

gregw · 2023-01-30T05:56:20Z

Sessions were already a core mechanism, but there was no API for them.

There is now a new Session interface that is available via the Request API. It is intended only for users of sessions.

The previous concrete class Session has been renamed to ManagedSession and it is used by classes that extend AbstractSessionManager.

Sessions were already a core mechanism, but there was no API for them. There is now a new Session interface that is available via the Request API. It is intended only for users of sessions. The previous concrete class Session has been renamed to ManagedSession and it is used by classes that extend AbstractSessionManager.

…core-sessions # Conflicts: # jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/SessionManager.java # jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/AbstractSessionManagerTest.java # jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/SimpleSessionHandler.java # jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/TestableSessionManager.java # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextResponse.java # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/SessionHandler.java # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/security/authentication/LoginAuthenticator.java # jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java # jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Response.java # jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/SessionHandler.java # jetty-ee9/jetty-ee9-security/src/main/java/org/eclipse/jetty/ee9/security/authentication/LoginAuthenticator.java

janbartel

Couple of questions/suggestions.

janbartel · 2023-01-31T05:13:53Z

jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/ManagedSession.java

@@ -339,6 +335,7 @@ public boolean isInvalid()
    {
        try (AutoLock l = _lock.lock())
        {
+            // TODO this is not the inverse of isValue???? what about CHANGING


VALID is self evident. INVALID also. CHANGING means in the process of changing the session id and INVALIDATING means in the process of becoming invalid. The spec says that when session callbacks happen during invalidation, the session must still be readable, but is mute about what should happen when the session is changing its id. See checkValidForRead and checkValidForWrite which try to implement what the spec wants wrt to session validity. We could probably re-evaluate how isValid and isInvalid are used and see if it can be simplified - after this PR is done with.

I have removed a few usages of it and then renamed to isInvalidOrInvalidating

janbartel · 2023-01-31T05:19:35Z

jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/SessionManager.java

@@ -75,33 +76,33 @@ public interface SessionManager extends LifeCycle, SessionConfig
     */
    String __MaxAgeProperty = "org.eclipse.jetty.servlet.MaxAge";

-    Session getSession(String id) throws Exception;
+    ManagedSession getManagedSession(String id) throws Exception;


Should these be Session and not ManagedSession?

No. Users and/or extenders of SessionManager are using the concrete ManagedSession. For example, soon after calling getManagedSession(id), the access method may be called on the returned session. This is not available in the public Session API as an application should not call access.

janbartel · 2023-01-31T05:23:13Z

...0/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java

+        return httpSession.getId();
+    }
+
+    public ManagedSession getManagedSession()


Maybe the signature should be Session?

No. The callers of this method are doing ManagedSession stuff like:

resetting cookies on response.reset() (and cookies are not a known part of the application Session API. Cookies are implementation details as we can track sessions with other mechanisms.

committing the response so they need to flush the session to any datastore. This is an implementation detail.

completing the response... ditto.

janbartel · 2023-02-01T04:27:36Z

jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/ManagedSession.java

-        return null;
+        _api = _manager.newSessionAPIWrapper(this);
+        if (_api != null && _api.getSession() != this)
+            throw new IllegalStateException("APISession must wrap this session");


Suggested change

throw new IllegalStateException("APISession must wrap this session");

throw new IllegalStateException("Session.API must wrap this session");

janbartel · 2023-02-01T04:28:16Z

jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/ManagedSession.java

-    }
-
-    public void setAPISession(Object o)
+    public <T extends API> T getAPISession()


Suggested change

public <T extends API> T getAPISession()

public <T extends API> T getAPI()

janbartel · 2023-02-01T04:40:56Z

...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java

    private Authentication _authentication;
    private String _method;
    private ServletMultiPartFormData.Parts _parts;
-    private ServletPathMapping _servletPathMapping;
    private boolean _asyncSupported = true;

    protected ServletApiRequest(ServletContextRequest servletContextRequest)
    {
        this._request = servletContextRequest;


Suggested change

this._request = servletContextRequest;

_request = servletContextRequest;

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Session.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java

lachlan-roberts · 2023-02-01T05:38:30Z

jetty-core/jetty-session/src/main/java/org/eclipse/jetty/session/AbstractSessionCache.java

@@ -107,7 +108,7 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
     * @return a new Session object
     */
    @Override
-    public abstract Session newSession(SessionData data);
+    public abstract ManagedSession newSession(SessionData data);


Why do these all need to use the ManagedSession class and not the Session interface?

Applications use Session.
Implementations of Session use SessionManagers and SessionHandlers and SessionCaches and SessionData to implement a ManagedSession, thus they communicate within themselves with ManagedSession.

If you are just a handler/servlet that wants to use a session, you just see/use Session interface. But if you are implementing/extending our style of concrete session you see/use ManagedSession

...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java

Improved javadoc used util Attributes interface.

lachlan-roberts · 2023-02-02T02:43:22Z

...-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiRequest.java

-
-        return _coreSession.getAPISession();
+        if (session.isNew() && getAuthentication() instanceof Authentication.User)
+            session.setAttribute(ManagedSession.SESSION_CREATED_SECURE, Boolean.TRUE);


You said you would move this attribute onto Session interface instead of ManagedSession?

It's a security feature. When you move ee10 security to core then I'll move this... or better yet do away with it. For now it has no good home and it's a bit of leaked implementation, so ManagedSession is probably the right place.

lachlan-roberts · 2023-02-02T02:50:24Z

...ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletApiResponse.java

        ServletApiRequest servletApiRequest = _response.getServletContextRequest().getServletApiRequest();
-        Session session = servletApiRequest.getCoreSession();
+        ManagedSession session = servletApiRequest.getServletContextRequest().getManagedSession();


I don't like that the servlet layer is dependent on a particular implementation of the Session interface.

Is it possible to generalize this so that it can all be done through the jetty-server Session interface so it would work if you used a different implementation of Session.

Maybe SessionManager should be a jetty-server interface as well?

It is probably possible. But currently ManagedSession contains servlet specific features like passivation and listeners. It would need to be split to move those boys into some servlet common module.... That we don't yet have. So a good thing to look at, but that's a different PR i think.

lachlan-roberts

I still don't like the design that jetty-server has the Session interface, but any implementation you can use must be based off the ManagedSession from jetty-session module.

Id rather a more complete set of interfaces in jetty-server, or have jetty-session use an attribute to store session instead of having a direct getter on the jetty-server request.

But hopefully this can be improved in further PRs.

gregw · 2023-02-02T09:40:28Z

I still don't like the design that jetty-server has the Session interface, but any implementation you can use must be based off the ManagedSession from jetty-session module.

Again this is because all our current usages are based on servlet API, which has a lot of it's implementation in ManagedSession.

When we (you) move security to core, it should just use the session interface and nothing else.

We can also eventually split the servlet stuff out of ManagedSession and put in a servlet common package.

Id rather a more complete set of interfaces in jetty-server, or have jetty-session use an attribute to store session instead of having a direct getter on the jetty-server request.

I don't see how using an attribute would help?

gregw requested review from janbartel and lachlan-roberts January 30, 2023 05:56

gregw added 4 commits January 31, 2023 09:28

Merge branch 'jetty-12.0.x' into jetty-12.0.x-core-sessions

ab4c1b2

Fixed tests

4fee6af

Use static method

b2592d0

gregw marked this pull request as ready for review January 31, 2023 01:54

joakime assigned gregw Jan 31, 2023

joakime added the Jetty 12 label Jan 31, 2023

joakime added this to the 12.0.x milestone Jan 31, 2023

joakime changed the title ~~Added a core Session abstraction~~ Jetty 12 - Added a core Session abstraction Jan 31, 2023

janbartel approved these changes Feb 1, 2023

View reviewed changes

Merge branch 'jetty-12.0.x' into jetty-12.0.x-core-sessions

3871b23

lachlan-roberts requested changes Feb 1, 2023

View reviewed changes

gregw added 2 commits February 1, 2023 17:20

Updates from review

0addf14

Updates from review

b0a1012

Improved javadoc used util Attributes interface.

gregw requested a review from lachlan-roberts February 1, 2023 20:57

lachlan-roberts reviewed Feb 2, 2023

View reviewed changes

gregw requested a review from lachlan-roberts February 2, 2023 05:06

lachlan-roberts approved these changes Feb 2, 2023

View reviewed changes

gregw merged commit ebc6cca into jetty-12.0.x Feb 2, 2023

gregw deleted the jetty-12.0.x-core-sessions branch February 2, 2023 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetty 12 - Added a core Session abstraction #9223

Jetty 12 - Added a core Session abstraction #9223

gregw commented Jan 30, 2023

janbartel left a comment

janbartel Jan 31, 2023

gregw Feb 1, 2023

janbartel Jan 31, 2023

gregw Feb 1, 2023

janbartel Jan 31, 2023

gregw Feb 1, 2023

janbartel Feb 1, 2023

janbartel Feb 1, 2023

janbartel Feb 1, 2023

lachlan-roberts Feb 1, 2023

gregw Feb 1, 2023

lachlan-roberts Feb 2, 2023

gregw Feb 2, 2023

lachlan-roberts Feb 2, 2023

gregw Feb 2, 2023

lachlan-roberts left a comment

gregw commented Feb 2, 2023

	throw new IllegalStateException("APISession must wrap this session");
	throw new IllegalStateException("Session.API must wrap this session");

	public <T extends API> T getAPISession()
	public <T extends API> T getAPI()

	this._request = servletContextRequest;
	_request = servletContextRequest;

Jetty 12 - Added a core Session abstraction #9223

Jetty 12 - Added a core Session abstraction #9223

Conversation

gregw commented Jan 30, 2023

janbartel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lachlan-roberts left a comment

Choose a reason for hiding this comment

gregw commented Feb 2, 2023