-
Notifications
You must be signed in to change notification settings - Fork 685
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
Refactor scrypt hashing and source app session mgmt pt 3/3 #5695
Refactor scrypt hashing and source app session mgmt pt 3/3 #5695
Conversation
d969087
to
c209b4d
Compare
fee40dd
to
2de49c4
Compare
2de49c4
to
62fb13e
Compare
securedrop/tests/conftest.py
Outdated
) | ||
journalist_app.crypto_util.genkeypair(source_user) | ||
source = source_user.get_db_record() | ||
return {'source_user': source_user, |
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.
This makes the source_user easy to access in the tests
62fb13e
to
9363869
Compare
9363869
to
8e10517
Compare
8e10517
to
5bba230
Compare
e94fa7a
to
c7bd749
Compare
c7bd749
to
362c5c6
Compare
pass | ||
self.SESSION_EXPIRATION_MINUTES: int = getattr( | ||
_config, "SESSION_EXPIRATION_MINUTES", 120 | ||
) |
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.
I moved the default value from the source app to here.
timedelta(minutes=getattr(config, | ||
'SESSION_EXPIRATION_MINUTES', | ||
120)) | ||
|
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.
This was moved to the SessionManager. I think this was the root cause of one of the bugs discussed by @zenmonkeykstop at the end of #5694 (but not 100% sure).
@@ -14,10 +16,10 @@ def login_required(f: Callable) -> Callable: | |||
@wraps(f) | |||
def decorated_function(*args: Any, **kwargs: Any) -> Any: | |||
try: | |||
logged_in_source = SessionManager.get_logged_in_user() | |||
logged_in_source = SessionManager.get_logged_in_user(db_session=db.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.
In this PR, I have removed the local cache (due to the problems encountered in #5694) so a DB session now has to be passed to the SessionManager in order to fetch the source record (instead of fetching it from the now-removed cache).
@@ -54,22 +55,30 @@ def generate() -> Union[str, werkzeug.Response]: | |||
codenames = session.get('codenames', {}) | |||
codenames[tab_id] = codename | |||
session['codenames'] = fit_codenames_into_cookie(codenames) | |||
session["codenames_expire"] = datetime.utcnow() + timedelta( | |||
minutes=config.SESSION_EXPIRATION_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.
I am not sure why the codenames from the /generate flow need to expire, but this brings the previous behavior back (so that they do expire).
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.
I don't know if this was the original motivation or not, but one reason I can see to expire codenames is that folks may have left their terminal on the "generate codename" page, then returned to it at a later point, increasing the risk that a third party observed the codename.
@@ -27,80 +28,76 @@ class UserHasBeenDeleted(_InvalidUserSession): | |||
|
|||
|
|||
class SessionManager: | |||
_CACHE = SimpleCache() # A mapping of passphrase -> SourceUser |
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.
I have removed the local cache here to address session timeout bugs discussed in #5694.
) | ||
# Save the session expiration date in the user's session cookie | ||
session_duration = timedelta(minutes=config.SESSION_EXPIRATION_MINUTES) | ||
session[cls._SESSION_COOKIE_KEY_FOR_EXPIRATION_DATE] = datetime.utcnow() + session_duration |
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.
I've put back the session expiration date in the session cookie; in my previous PR it was in the local cache.
@@ -118,6 +122,8 @@ def __init__( | |||
self._scrypt_p = scrypt_p | |||
self._backend = default_backend() | |||
|
|||
# Use @lru_cache to not recompute the same values over and over for the same user | |||
@lru_cache |
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.
This should help a lot with performance, by not having to recompute hashes on every authenticated request.
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.
Worked well in my staging. I will need a second pair of eyes before merging.
I've done some additional testing with packages built from this branch using the staging env, and so far have not encountered any issues, including by following the steps from #5694 (comment) . I'll do additional testing tomorrow 1) with multiple browser instances, 2) with a forced low session expiry setting. |
I've done additional testing of the following:
Let me know if you'd like me to test anything else in particular, but so far, the session behavior looks pretty solid in my testing on staging. The behavior also matches what I see in my current 2.0.2 prod environment. |
Also took a final spin through, via the prod upgrade scenario. Upgrade worked fine, testing with multiple sources worked fine, also loaded a bunch of test data, no issues that I could see. |
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.
Also took this for a spin, and am pleased to say I can't reproduce any of the session-related problems I was experiencing on develop. Also made sure to follow the clear STR in #5694 (comment) , and everything worked well.
Thanks, @zenmonkeykstop, for all your careful review of these changes. And thanks to @nabla-c0d3 as well, for your collaboration throughout review!
Thanks everyone for taking the time to review and test these changes in depth 😀 |
Status
Ready.
Description of Changes