-
Notifications
You must be signed in to change notification settings - Fork 4
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
3060 active user session #3182
3060 active user session #3182
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3182 +/- ##
========================================
Coverage 92.66% 92.66%
========================================
Files 47 47
Lines 1009 1009
Branches 169 169
========================================
Hits 935 935
Misses 42 42
Partials 32 32
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
backend is using |
We are using signed-cookies to keep the session information, which is using SessionBase. |
SESSION_COOKIE_AGE = 15 * 60 # 15 minutes | ||
#SESSION_EXPIRE_AT_BROWSER_CLOSE = True | ||
SESSION_SAVE_EVERY_REQUEST = True | ||
SESSION_COOKIE_AGE = 10 # 15 minutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For testing only: revert back to previous setting
@@ -1,7 +1,6 @@ | |||
"""Admin class for DataFile objects.""" | |||
from django.contrib import admin | |||
from tdpservice.core.utils import ReadOnlyAdminMixin | |||
# from tdpservice.core.filters import custom_filter_title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated to this PR but I saw this is commented and not deleted
…app into 3060-active-user-session
@@ -281,10 +281,12 @@ class Common(Configuration): | |||
) | |||
|
|||
# Sessions | |||
SESSION_ENGINE = "django.contrib.sessions.backends.signed_cookies" | |||
SESSION_ENGINE = "tdpservice.core.custom_session_engine" | |||
SIGNED_COOKIE_EXPIRES = 60 * 60 * 24 # 24 hours |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raftmsohani is this set to expire 24 hours from the initial session? if so, i wonder if it would be better to shorten this to 12 hours, which is on the very high end of an average work day. i'm also reaching out to our security officer about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raftmsohani TDP's ISSO's feedback:
- this change is related to AC-2-5. We'll need to update this documentation as part of this work to capture how the session refresh/expiration works for ACF users.
- let's also reduce the expiration to 12 hours
- we'll likely need to also complete risk-based decision documentation when the security team provides it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ADPennington we shouldn't need to update the AC-2-5, since we are still following same time out concept. In fact, this PR makes sure we are exactly following this time out setting.
The only difference is: with new changes, even if the user is active, system will log the user out after 12hrs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirming that the AC-2-5 seems aligned with the behavior in this PR, particularly this part: If the user chooses to extend the session, TDP will refresh the session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feedback included in comments.
SESSION_COOKIE_HTTPONLY = True | ||
SESSION_SAVE_EVERY_REQUEST = True | ||
#SESSION_EXPIRE_AT_BROWSER_CLOSE = True | ||
SESSION_COOKIE_AGE = 10 # 15 minutes | ||
SESSION_COOKIE_AGE = 60 * 30 # 30 minutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@raftmsohani can confirm that I received the warning for inactivity while logged in via AMS. im gonna wait another 30 minutes, because Im trying to understand the relationship between cookie_age and cookie_expires. max age set to 30 minutes when looking at devtools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 thanks @raftmsohani
Summary of Changes
Pull request closes #3060 _
How to Test
List the steps to test the PR
These steps are generic, please adjust as necessary.
Deliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
the session ends when the browser is closedlfrohlich
and/oradpennington
confirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):