ENH: Add logic for refreshing sessions and handling RMS sessions separately#325
ENH: Add logic for refreshing sessions and handling RMS sessions separately#325slangeveld wants to merge 1 commit intoequinor:mainfrom
Conversation
e2a1284 to
992a803
Compare
| API_V1_PREFIX: str = Field(default="/api/v1", frozen=True) | ||
| SESSION_COOKIE_KEY: str = Field(default="fmu_settings_session", frozen=True) | ||
| SESSION_EXPIRE_SECONDS: int = Field(default=1200, frozen=True) # 20 minutes | ||
| SESSION_EXPIRE_SECONDS: int = Field(default=31556926, frozen=True) # 1 year |
There was a problem hiding this comment.
In case we want to change the session expiration time later (not keep it valid forever) I still kept that as an option and just used 1 year as a measure of "infinite".
There was a problem hiding this comment.
maybe better to use this for readability:
SESSION_EXPIRE_SECONDS: int = Field(
default=int(timedelta(days=365).total_seconds()),
frozen=True,
)| return await session_manager.get_session( | ||
| fmu_settings_session, extend_expiration=False | ||
| ) | ||
| return await get_fmu_session(fmu_settings_session) |
There was a problem hiding this comment.
The functionality for removing/destroying sessions where kindoff hidden inside the session_manager.get_session() function. This has now been moved out to a separate dependency DestroySessionIfExpiredDep which is injected into the get_session dependency.
|
|
||
|
|
||
| async def get_project_session( | ||
| session: SessionDep, |
There was a problem hiding this comment.
Rather inject the depedency then call the get_session function directly
| DestroySessionIfExpiredDep = Annotated[None, Depends(destroy_session_if_expired)] | ||
|
|
||
|
|
||
| async def get_session_no_extend( |
There was a problem hiding this comment.
All these no_extend functions could now be removed as session_manager.get_session()does no longer extend the session expiration time.
| ) | ||
| async def post_lock_refresh( | ||
| session_service: ProjectSessionServiceNoExtendDep, | ||
| session_service: ProjectSessionServiceDep, |
There was a problem hiding this comment.
No need to distinguish between ProjectSessionServiceNoExtendDep and ProjectSessionServiceDep anymore.
| old_session = await session_manager.get_session( | ||
| fmu_settings_session, extend_expiration=False | ||
| ) | ||
| try: |
There was a problem hiding this comment.
This endpoint has been updated to not transfer old session data to new sessions. Instead, new sessions are created if and only if there does not already exist a session for the session_id in the cookie.
If it exists but is expired, the injected dependency DestroySessionIfExpiredDep will destroy it before the code in post_session is executed so a new session can be created.
For these case where a valid session exists, but has no project attached to it, a project will be added to it if a project can be found in the parents path.
| try: | ||
| existing_session = await get_fmu_session(fmu_settings_session) | ||
| if isinstance(existing_session, ProjectSession): | ||
| response.set_cookie( |
There was a problem hiding this comment.
Me might not have to set the cookie here for this case where dont make any changes to the existing session as this means the client making the reqeust already has a cookie with a valid session set. But it depends a bit on the logic in the GUI and how it handles the response that comes from the API, so added it here just to be safe.
There was a problem hiding this comment.
we don't need to set the cookie for an existing ProjectSession. The client sends credentials automatically and doesn’t read the session cookie or response id itself, so in this branch it already has the valid cookie it needs.
suggest to refactor this endpoint for readability:
session_id: str | None = None
if fmu_settings_session:
try:
existing_session = await get_fmu_session(fmu_settings_session)
except SessionNotFoundError:
pass
else:
if isinstance(existing_session, ProjectSession):
return SessionResponse(
id=existing_session.id,
created_at=existing_session.created_at,
expires_at=existing_session.expires_at,
rms_expires_at=await get_rms_session_expiration(
existing_session.id
),
last_accessed=existing_session.last_accessed,
)
session_id = existing_session.id
if session_id is None:
session_id = await create_fmu_session(user_fmu_dir)
response.set_cookie(
key=settings.SESSION_COOKIE_KEY,
value=session_id,
httponly=True,
secure=False,
samesite="lax",
)
with contextlib.suppress(FileNotFoundError, LockError):
path = Path.cwd()
project_fmu_dir = find_nearest_fmu_directory(path)
await add_fmu_project_to_session(session_id, project_fmu_dir)
session = await get_fmu_session(session_id)
return SessionResponse(
id=session.id,
created_at=session.created_at,
expires_at=session.expires_at,
rms_expires_at=None,
last_accessed=session.last_accessed,
)There was a problem hiding this comment.
It feels like this could be simplified even more now when a session in theory doesn't expire..
Could we make this into a pure create session endpoint?
Currently, the GUI uses this endpoint to create an initial session and also calls it again whenever the API returns a "session expired" response. However, with the changes introduced this PR to extend a session lifetime this automatic session creation can be removed from the GUI - then there will be only one call to this endpoint.
The dependency added will destroy an existing session if it has expired, could this endpoint if run multiple times raise SessionAlreadyExistError, or something simple?
We already have the get_session endpoint for getting a session response for an existing session. and we have another endpoint for adding a project to the session. So we don't really need to run this endpoint more than the initial creation of the session.
if fmu_settings_session:
try:
existing_session = await get_fmu_session(fmu_settings_session)
return Message("Session already exists")
except SessionNotFoundError:
pass
session_id = await create_fmu_session(user_fmu_dir)
with contextlib.suppress(FileNotFoundError, LockError):
path = Path.cwd()
project_fmu_dir = find_nearest_fmu_directory(path)
await add_fmu_project_to_session(session_id, project_fmu_dir)
session = await session_manager.get_session(session_id)
return SessionResponse(
id=session.id,
created_at=session.created_at,
expires_at=session.expires_at,
rms_expires_at=None
last_accessed=session.last_accessed,
)|
|
||
| session = await session_manager.get_session(session_id) | ||
| @router.patch( | ||
| "/", |
There was a problem hiding this comment.
Extending the expiration time of a session now has to be done through this PATCH /session endpoint.
Calling this endpoint will extend both the RMS session and the user session. In practice there is no need to extend the user session at the moment (as it is set to 1 year), but with this logic it is a bit more flexible in case we want to reduce lifetime of the user session again at one point.
The GUI will need to keep track of the expiration time for the RMS session and do a refresh call before it expires.
If a refresh is requested for an expired RMS session, the injected dependency SessionDep will remove the RMS session from the user session (through DestroySessionIfExpiredDep) which in effect will close the RMS project.
There was a problem hiding this comment.
should we just make this a dependency injection instead of endpoint to refresh RMS session, and inject this to all call to rms endpoints? just like the RefreshLockDep, so the GUI will not have to do anything, any call to rms endpoints mean the user is still actively working on rms project and rms session expiry will be automatically refreshed
There was a problem hiding this comment.
so i think making this refresh rms session as a dependency is better and no changes needed in the GUI because the GUI does not need to keep track of expiry time.
currently we keep track the lock expiration and show a notification when the lock is about to expire. this is needed because having a project lock will block someone else to access the project, so extending the expiry time should be a user decision. but asking the user to refresh rms session is not necessary because they are the only one using it.
a small change in the gui is also needed for now because setIsRmsProjectOpen(false) is only set when failing to open rms project and closing rms project, setIsRmsProjectOpen(false) should also be set when we failed to call the GET rms endpoints when we don't have the rms project opened (because it expired and the proxy shutdown). this is why we observed the bug as Kristin mentioned.
a better long term solution maybe to set the api to hold rms data after fetching it, as this data is unlikely to change in one session, so an expired rms session is not a problem. but this requires more changes in the api and gui.
what do you think @tnatt?
There was a problem hiding this comment.
Fair enough. The only problem I see with this is that the RMS project can be closed for a user without them knowing. Which might lead to a somewhat frustrating user experience as they would then need to open it again. Maybe this is not a big problem, but the overall user experience should be taken into consideration
There was a problem hiding this comment.
yes that's a problem, but the gui needs to handle it by showing a dialog/toast telling the rms is closed and they need to open it again if they want to.
There was a problem hiding this comment.
I like the idea of extending the rms session automatically for every call to an rms endpoint.
That way we reduce the amount of dialog pop-ups the user encounter. But still think it can be valuable to give the users a way of refreshing the rms session, without them having to reopen the project. I envision that in the GUI when working there is a dialog asking the users if they want to continue working.
If rms session expires before they could react to it, the dialog could stay open informing users that rms closed after inactivity asking them if they want to reopen it to resume editing. ...not sure how that will work in combination with the .fmu turning read-only though - we don't want one dialog saying the lock expired and one dialog on top of there saying the rms expired 🤔 need to think a bit of the ux on that scenario
There was a problem hiding this comment.
maybe the cleanest option is for the api to save the result from rms in memory and the gui will use that saved data, that way the data is available as long as the session and we can just close rms whenever so expiry time is not a problem
There was a problem hiding this comment.
The class methods and wrapper functions in this file has been refactored such that only the wrapper functions are using the SessionManager singleton directly. All other calls from outside this file will use the wrapper functions to do operations on the Sessions manager.
| async def get_session( | ||
| self: Self, session_id: str, extend_expiration: bool = True | ||
| ) -> Session | ProjectSession: | ||
| async def get_session(self: Self, session_id: str) -> Session | ProjectSession: |
There was a problem hiding this comment.
The get_session class method has been updated to only do what you would expect from its method name: getting the session from the session storage. The logic for extending and destroying the session has been moved out from here.
| async def destroy_fmu_session_if_expired(session_id: str) -> None: | ||
| """Destroys the expired sessions in the session manager for the given session_id. | ||
|
|
||
| Cheks the user and rms session for the given session_id and destroy the ones |
There was a problem hiding this comment.
This method removes an expired RMS session from a session or destroys the whole session if the user session itself has expired (not likely to happen atm with expiration time set to 1 year)
There was a problem hiding this comment.
typo: Cheks should be Checks
| await session_manager.destroy_session(session_id) | ||
|
|
||
|
|
||
| async def refresh_fmu_session(session_id: str) -> Session | ProjectSession: |
There was a problem hiding this comment.
This method refreshes both the RMS session (if present) and the user session.
| expires_at: datetime | ||
| """Timestamp when the session will expire.""" | ||
|
|
||
| rms_expires_at: datetime | None |
There was a problem hiding this comment.
Need to also return this to the client so that it can keep track of when the RMS session will expire.
| """The RMS API executor controlling the worker lifetime.""" | ||
| project: RmsApiProxy | ||
| """An opened RMS project that close() can be called against.""" | ||
| expires_at: datetime |
There was a problem hiding this comment.
This is what will keep track of when the RMS session has expired.
992a803 to
60b122b
Compare
GibranAlfa
left a comment
There was a problem hiding this comment.
looks good overall, just a comment on the new patch endpoint. was also thinking about using SESSION_EXPIRE_SECONDS: int | None = Field(default=None, frozen=True) to signal the session never expires, but that will require some changes in the GUI to handle null for session expiry time, so current implementation is OK with 1 year expiry time.
| After migrating the state, the old session is destroyed. | ||
| If a session already exists when POSTing to this route, a project will be added | ||
| to the existing session if a project can be found. In case a session with a | ||
| project already exists, the existing session will kept as-is and |
| async def destroy_fmu_session_if_expired(session_id: str) -> None: | ||
| """Destroys the expired sessions in the session manager for the given session_id. | ||
|
|
||
| Cheks the user and rms session for the given session_id and destroy the ones |
There was a problem hiding this comment.
typo: Cheks should be Checks
| """Timestamp when the session will expire.""" | ||
|
|
||
| rms_expires_at: datetime | None | ||
| """Timestamp when the rms session will expire.""" |
There was a problem hiding this comment.
| """Timestamp when the rms session will expire.""" | |
| """Timestamp when the RMS session will expire.""" |
|
|
||
| session = await session_manager.get_session(session_id) | ||
| @router.patch( | ||
| "/", |
There was a problem hiding this comment.
should we just make this a dependency injection instead of endpoint to refresh RMS session, and inject this to all call to rms endpoints? just like the RefreshLockDep, so the GUI will not have to do anything, any call to rms endpoints mean the user is still actively working on rms project and rms session expiry will be automatically refreshed
|
|
||
| async def get_project_session( | ||
| session: SessionDep, | ||
| fmu_settings_session: str | None = Cookie(None), |
There was a problem hiding this comment.
this can be removed as it's not used anymore
|
|
||
| async def get_smda_session( | ||
| session: SessionDep, | ||
| fmu_settings_session: str | None = Cookie(None), |
There was a problem hiding this comment.
this can be removed as it's not used anymore
|
|
||
| async def get_project_smda_session( | ||
| session: ProjectSessionDep, | ||
| fmu_settings_session: str | None = Cookie(None), |
There was a problem hiding this comment.
this can be removed as it's not used anymore
| API_V1_PREFIX: str = Field(default="/api/v1", frozen=True) | ||
| SESSION_COOKIE_KEY: str = Field(default="fmu_settings_session", frozen=True) | ||
| SESSION_EXPIRE_SECONDS: int = Field(default=1200, frozen=True) # 20 minutes | ||
| SESSION_EXPIRE_SECONDS: int = Field(default=31556926, frozen=True) # 1 year |
There was a problem hiding this comment.
maybe better to use this for readability:
SESSION_EXPIRE_SECONDS: int = Field(
default=int(timedelta(days=365).total_seconds()),
frozen=True,
)|
Here's a suggestion of an alternative way to handle the API session expiration and renewal: Suppose that the session is kept internally in the API indefinitely, and not destroyed when it has expired. A session is created the first time, with an expiry date, but when this expiry date arrives the session object is kept, instead of being destroyed as it is done today. The "existing session dependency" will return a failure due to the expiry date having passed, but the data is still there. The endpoint will return a 401 Unauthorized, and the GUI will call the session creation endpoint. When this endpoint is called, the Such a solution would preserve data between sessions, since the actual session object persists internally and is not destroyed - the problem of transferring data from the expired session to the new session would be irrelevant. Whenever the client calls the create session endpoint, only the create/expiry/cookie values are renewed, and the context (data) for the session is preserved. It would be good to consider this solution, as I think it would solve the existing problem in a better way. This solution would be instead of changing the session duration from 20 minutes to 1 year. Compared to OAuth, the API token would be similar to a refresh token (for getting a new access token), with the cookie and short lived session similar to what the access token gives. |
|
Great suggestion from Joar! This issue summarized what needs to be changed to implement that suggestion. For this PR, as a minimal fix as discussed in the meeting, it will be updated to:
|
Resolves #235
Add logic for refreshing sessions and handling RMS sessions separately.
The session logic has been refactored to only accept refreshing of sessions through the new session refresh endpoint and not through internal logic in
get_session. The RMS session has been updated with a fieldexpires_atwhich allows for handling the RMS session and user session separately.In addition the logic for destroying a session is moved to a new dependency DestroySessionIfExpiredDep that is injected into the relevant places.
Both the RMS session and user session are treated as sessions that can expire and be refreshed. At the moment the user session expiration time is set to 1 year so in practice we will not need that there, but it gives the flexibility to reduce this later if needed.
As the session logic affect a big part of the code (especially tests), the PR is quite large (mostly tests) so have tried to add comments for explanation and context at the relevant places. Let me know if anything is unclear.
The new flow for creating, refreshing and destroying sessions will be:
POSTendpointfmu-settings-apiendpoints that requires a session, the DestroySessionIfExpiredDep is injected, making sure any expired RMS sessions are removed or user sessions are destroyed.fmu-settings-api.Checklist
--cov=src/ --cov-report term-missing)