Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TMVCEngine.GetCurrentSession ignores TWebSession.IsExpired #355

Closed
fastbike opened this issue Apr 6, 2020 · 7 comments
Closed

TMVCEngine.GetCurrentSession ignores TWebSession.IsExpired #355

fastbike opened this issue Apr 6, 2020 · 7 comments
Assignees
Labels
accepted Issue has been accepted and inserted in a future milestone
Milestone

Comments

@fastbike
Copy link
Contributor

fastbike commented Apr 6, 2020

The class function TMVCEngine.GetCurrentSession ignores the implementation of the IsExpired virtual function in the actual web session item.
Current code at line 2465 is

      IsExpired := True;
      if List.TryGetValue(ASessionId, Result) then
        if (ASessionTimeout = 0) then
          IsExpired := MinutesBetween(Now, Result.LastAccess) > DEFAULT_SESSION_INACTIVITY
        else
          IsExpired := MinutesBetween(Now, Result.LastAccess) > ASessionTimeout;

But this should be

      IsExpired := True;
      if List.TryGetValue(ASessionId, Result) then
        IsExpired  := Result.IsExpired;

This allows the web session item to determine if the timeout has expired. E.g. when using an OAuth token to determine session lifetime

@fastbike fastbike closed this as completed Apr 6, 2020
@fastbike fastbike reopened this Apr 6, 2020
@fastbike
Copy link
Contributor Author

fastbike commented Apr 7, 2020

SessionTimeout is a fixed value to be added to the time the session was last accessed, whereas with an OAuth controlled session, the "expiry time" is coded into the token and is not related to the last access time.

@danieleteti
Copy link
Owner

The session is just the good-old session mantained with the session cookie. OAuth or JWT "session" is just recreated from the info contained in the token after the token validation. Did I miss something?

@fastbike
Copy link
Contributor Author

fastbike commented Apr 9, 2020

I'm using a TWebSession descendant which stores the OAuth token expiry time and uses that to provide the result in the overridden IsExpired session function. Most places in the code call the IsExpired function but the code I have highlighted does not, so any custom implementation of session expiry is by passed.

You mention JWT and OAuth, is there an existing implementation I have missed ?

@danieleteti
Copy link
Owner

DMVCFramework provides a fully compatible JWT implementation out of the box. If you have all the OAuth machinery in place, creating a middleware to support OAuth is a matter of hours using the implementation for JWT provided in the unit sources\MVCFramework.Middleware.JWT.pas

@fastbike
Copy link
Contributor Author

I do have a custom middleware that supports OAuth.
My JWT token is being issued by a third party Identity Provider (IP). That IP controls the session expiry time, not the session lifetime I have configured in the DMVC configuration. My session descendant class compares the current date/time against the token's "exp" claim to return IsExpired.
So the MVC Engine code needs to read the IsExpired from this class rather than the current implementation that uses a hard coded algorithm which ignores the algorithmin coded into the session descendant.

@danieleteti
Copy link
Owner

It should be fixed (all unit tests pass). Please let me know if works in your case.

@danieleteti danieleteti added the accepted Issue has been accepted and inserted in a future milestone label Apr 12, 2020
@danieleteti danieleteti added this to the 3.2.0-boron milestone Apr 12, 2020
@danieleteti danieleteti self-assigned this Apr 12, 2020
@fastbike
Copy link
Contributor Author

Very nice improvement, many thanks for working with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue has been accepted and inserted in a future milestone
Projects
None yet
Development

No branches or pull requests

2 participants