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

sessions: addition of KVSession interface #292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drjova
Copy link
Member

@drjova drjova commented Nov 21, 2019

Signed-off-by: Harris Tzovanakis [email protected]

Closes #289.

@drjova drjova force-pushed the create-session-interface branch 2 times, most recently from 89991aa to 0bdc7e7 Compare November 21, 2019 15:43
@drjova drjova changed the title sessions: stop persist anonymous sessions: addition of KVSession interface Nov 21, 2019
@drjova drjova force-pushed the create-session-interface branch from 0bdc7e7 to df00af1 Compare November 22, 2019 14:27
@drjova
Copy link
Member Author

drjova commented Nov 22, 2019

@slint updated

@kpsherva kpsherva self-assigned this Mar 3, 2020
@@ -152,3 +154,14 @@ def default_session_store_factory(app):
accounts_session_redis_url))
from simplekv.memory import DictStore
return DictStore()


class KVSessionInterfaceWithAnonymousSessions(KVSessionInterface):
Copy link
Contributor

@kpsherva kpsherva Mar 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small impact: IMO the name indicates that we store sessions for anonymous. I would suggest KVSessionInterfaceStoreAuthenticated, also is it possible to test it?

@kpsherva kpsherva removed their assignment Mar 3, 2020
@diegodelemos diegodelemos force-pushed the create-session-interface branch from df00af1 to 382f8b8 Compare May 8, 2020 15:08
@diegodelemos
Copy link
Member

diegodelemos commented May 8, 2020

Working using latest cookiecutter-invenio-instance:

~/invenio-accounts $ git diff
diff --git a/setup.py b/setup.py
index 0eed598..81e41f9 100644
--- a/setup.py
+++ b/setup.py
@@ -75,7 +75,7 @@ install_requires = [
      'email-validator>=1.0.5',
      'future>=0.16.0',
      'invenio-base>=1.2.2',
-    'invenio-i18n>=1.2.0',
+    'invenio-i18n<1.2.0,>=1.1.1',
      'invenio-celery>=1.1.2',
      'invenio-rest>=1.1.3',
      'maxminddb-geolite2>=2017.404',
~/invenio-accounts $ cd ../ && cookiecutter cookiecutter-invenio-instance
$ cd my-site
$ cat my_site/config.py
...
# Sessions
# ========
#: Whether the instance should use SameSite cookies to store client sessions.
SESSION_COOKIE_SAMESITE = 'Strict'
...
$ ./scripts/bootstrap
$ ./scripts/setup
$ pipenv install ../invenio-accounts # the dev version
$ pip env shell
$ pip uninstall flask-kvsession -y
$ pip show flask-kvsession-invenio 
Name: Flask-KVSession-Invenio
Version: 0.6.3
...
$ ./scripts/server

Screenshot 2020-05-08 at 16 41 05

Note: Invenio-i18n version needs to be changed because Invenio 3.2.1 doesn't support it, but this doesn't change anything regarding Flask-KVSession integration


We need to:

  • Decide what are we going to put as the default value for SameSite cookies on sessions, in the past, we have been always choosing the most secure defaults but I am not sure for this case.
  • Address Karolina's message:
    • Regarding the naming, I agree with her.
    • Regarding the tests, should I go ahead and write them?

CC @ppanero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

global: fork or integrate Flask-KVSession
4 participants