-
Notifications
You must be signed in to change notification settings - Fork 238
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
implemented DynamoDBSessionInterface and tests. #214
Changes from all commits
0b60f0c
1f8acb3
37bff6e
7cea169
3928290
1ad3eb0
68415e3
1f5d5d3
14d6a15
ab9a756
a92914e
1736952
5a3413b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,4 +15,6 @@ redis | |
pymemcache | ||
Flask-SQLAlchemy | ||
pymongo | ||
boto3 | ||
mypy_boto3_dynamodb | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,4 +7,6 @@ redis | |
cachelib | ||
pymongo | ||
flask_sqlalchemy | ||
pymemcache | ||
pymemcache | ||
boto3 | ||
mypy_boto3_dynamodb |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
from .dynamodb import DynamoDBSession, DynamoDBSessionInterface # noqa: F401 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
import warnings | ||
from datetime import datetime | ||
from datetime import timedelta as TimeDelta | ||
from decimal import Decimal | ||
from typing import Optional | ||
|
||
import boto3 | ||
from mypy_boto3_dynamodb.service_resource import DynamoDBServiceResource | ||
from flask import Flask | ||
from itsdangerous import want_bytes | ||
|
||
from ..base import ServerSideSession, ServerSideSessionInterface | ||
from ..defaults import Defaults | ||
|
||
|
||
class DynamoDBSession(ServerSideSession): | ||
pass | ||
|
||
|
||
class DynamoDBSessionInterface(ServerSideSessionInterface): | ||
"""A Session interface that uses dynamodb as backend. (`boto3` required) | ||
|
||
:param client: A ``DynamoDBServiceResource`` instance. | ||
:param key_prefix: A prefix that is added to all DynamoDB store keys. | ||
:param use_signer: Whether to sign the session id cookie or not. | ||
:param permanent: Whether to use permanent session or not. | ||
:param sid_length: The length of the generated session id in bytes. | ||
:param table_name: DynamoDB table name to store the session. | ||
|
||
.. versionadded:: 0.6 | ||
The `sid_length` parameter was added. | ||
|
||
.. versionadded:: 0.2 | ||
The `use_signer` parameter was added. | ||
""" | ||
|
||
session_class = DynamoDBSession | ||
|
||
def __init__( | ||
self, | ||
app: Flask, | ||
client: Optional[DynamoDBServiceResource] = Defaults.SESSION_DYNAMODB, | ||
key_prefix: str = Defaults.SESSION_KEY_PREFIX, | ||
use_signer: bool = Defaults.SESSION_USE_SIGNER, | ||
permanent: bool = Defaults.SESSION_PERMANENT, | ||
sid_length: int = Defaults.SESSION_ID_LENGTH, | ||
serialization_format: str = Defaults.SESSION_SERIALIZATION_FORMAT, | ||
table_name: str = Defaults.SESSION_DYNAMODB_TABLE, | ||
): | ||
|
||
if client is None: | ||
warnings.warn( | ||
"No valid DynamoDBServiceResource instance provided, attempting to create a new instance on localhost:8000.", | ||
RuntimeWarning, | ||
stacklevel=1, | ||
) | ||
client = boto3.resource( | ||
"dynamodb", | ||
endpoint_url="http://localhost:8000", | ||
region_name="us-west-2", | ||
aws_access_key_id="dummy", | ||
aws_secret_access_key="dummy", | ||
) | ||
|
||
try: | ||
client.create_table( | ||
AttributeDefinitions=[ | ||
{"AttributeName": "id", "AttributeType": "S"}, | ||
], | ||
TableName=table_name, | ||
KeySchema=[ | ||
{"AttributeName": "id", "KeyType": "HASH"}, | ||
], | ||
BillingMode="PAY_PER_REQUEST", | ||
) | ||
client.meta.client.get_waiter("table_exists").wait(TableName=table_name) | ||
client.meta.client.update_time_to_live( | ||
TableName=self.table_name, | ||
TimeToLiveSpecification={ | ||
"Enabled": True, | ||
"AttributeName": "expiration", | ||
}, | ||
) | ||
except (AttributeError, client.meta.client.exceptions.ResourceInUseException): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there may be a problem here: Scenario:
This means we'd have to give whichever resource is running this permission to use the CreateTable etc. APIs even though we want to use an existing table. Not ideal from a security perspective. (Purely theoretical at this point, but I'm fairly certain this is what would happen) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, seems more reasonable to check if the table exists before the try, is that what you would suggest? Could you add a PR to development? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a security perspective I'd prefer that the implementation can work with minimal permissions, in this case:
This would be sufficient for read and write access to a properly configured table. All this table and TTL management is something that we usually like to handle through Infrastructure as Code. Many companies also require additional configuration regarding backups, etc. Having the option of doing that through flask-session is nice for convenience, but IMO a bit too much magic that happens by default. I'd prefer an Got distracted and forgot the middle part :D I'd prefer to handle the non-existence of a table when we try to write to it/read from it. The ask for forgiveness instead of approval approach - that requires fewer API calls and AWS permissions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean. I think the first step would be to factor out the creation into a single method on each interface. Similar to what is done here https://github.com/kroketio/quart-session/blob/master/quart_session/sessions.py.This could then be easily subclassed to skip the method (maybe like 3 lines of code). The second step would be adding the config setting for all interfaces, which I am less sure of (adding more settings) unless there is a bit of demand or discussing with the team. I think if the first was done neatly it would be merged quickly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm currently looking into this, but I'm not super familiar with the codebase yet. My approach would be to:
This would result in there being two DynamoDB varieties for the I'm considering another optional flag to the constructor of Not sure what is a better approach, any thoughts @Lxstr ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can simplify a bit Step 1 is good. This is already done for postgresql (impending PR) but would also need to be done for SQLAlchemy and the base class Steps 2 and 3 can be instead adding the flag SESSION_TABLE_EXISTS Base class should look something like:
Tests would be great Does that make sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, looks good to me, I've added a variation of that as a PR: #237 |
||
# TTL already exists, or table already exists | ||
pass | ||
|
||
self.client = client | ||
self.store = client.Table(table_name) | ||
super().__init__( | ||
app, | ||
key_prefix, | ||
use_signer, | ||
permanent, | ||
sid_length, | ||
serialization_format, | ||
) | ||
|
||
def _retrieve_session_data(self, store_id: str) -> Optional[dict]: | ||
# Get the saved session (document) from the database | ||
document = self.store.get_item(Key={"id": store_id}).get("Item") | ||
if document: | ||
serialized_session_data = want_bytes(document.get("val").value) | ||
return self.serializer.decode(serialized_session_data) | ||
return None | ||
|
||
def _delete_session(self, store_id: str) -> None: | ||
self.store.delete_item(Key={"id": store_id}) | ||
|
||
def _upsert_session( | ||
self, session_lifetime: TimeDelta, session: ServerSideSession, store_id: str | ||
) -> None: | ||
storage_expiration_datetime = datetime.utcnow() + session_lifetime | ||
# Serialize the session data | ||
serialized_session_data = self.serializer.encode(session) | ||
|
||
self.store.update_item( | ||
Key={ | ||
"id": store_id, | ||
}, | ||
UpdateExpression="SET val = :value, expiration = :exp", | ||
ExpressionAttributeValues={ | ||
":value": serialized_session_data, | ||
":exp": Decimal(storage_expiration_datetime.timestamp()), | ||
}, | ||
) |
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.
Nice