-
Notifications
You must be signed in to change notification settings - Fork 91
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
ADD crypto-fields #160
base: master
Are you sure you want to change the base?
ADD crypto-fields #160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 93.02% 93.04% +0.01%
==========================================
Files 9 11 +2
Lines 789 949 +160
==========================================
+ Hits 734 883 +149
- Misses 55 66 +11
Continue to review full report at Codecov.
|
Hi @kamil1marczak, thank you for this detailed PR. We will review it and get back to you. |
drf_extra_fields/crypto_field.py
Outdated
Cryptography protocol used in mixin: Fernet (symmetric encryption) provided by Cryptography (pyca/cryptography) | ||
""" | ||
|
||
def __init__(self, salt_settings_env=None, password=None, *args, **kwargs): |
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.
salt_settings_env
sounds like an environment variable but it's not actually. Why not this is directly provided like password
? Why there is an inconsistency?
I suggest to have DEFAULT_CRYPTOGRAPHY_SALT and DEFAULT_CRYPTOGRAPHY_PASSWORD variables in settings.py. These must be defined to be able to use crypto fields. If salt or password is not provided along with field definition these will be used. What do you think?
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 an updated version, I use Django SECRET_KEY as salt but I kept the password as an optional parameter to make it more dynamic (the whole idea is to have something static - salt and dynamic - password as encryption ingredients)
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.
Thank you for your PR, we much appreciate it! However, I guess there is a misunderstanding here (and yes, I have realized it now, after reviewing your PR... 😓). This package includes extra fields for DRF (Django Rest Framework) Serializer fields, not for Django model fields (at least until now). I don't know what can/should we do about it. Any thoughts @alicertel?
README.md
Outdated
## CryptoField Package | ||
A set of generic common Djano Fields that automatically encrypt data for database amd encrypt for the purpose of usage in Django. | ||
* *CryptoFieldMixin* - Mixin that implement encrypt/decrypt methods. | ||
Example of how to use to create custom CryptoTextField |
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.
## CryptoField Package | |
A set of generic common Djano Fields that automatically encrypt data for database amd encrypt for the purpose of usage in Django. | |
* *CryptoFieldMixin* - Mixin that implement encrypt/decrypt methods. | |
Example of how to use to create custom CryptoTextField | |
## CryptoField Package | |
A set of generic common Django Fields that automatically encrypts data for the database and for the purpose of usage in Django. | |
* *CryptoFieldMixin* - A mixin that implements encrypt/decrypt methods. | |
Example of how to use it to create the CustomCryptoTextField |
README.md
Outdated
**Settings.** | ||
each CryptoField has 2 kwargs *salt_settings_env* and *password*. | ||
* *salt_settings_env* - name of variable stored in settings.py file, which will be used as cryptographic salt. default: salt_settings_env = 'SECRET_KEY' if settings not set or no SECRET_KEY in setting default: salt = "Salt123!!!" | ||
* *password* - password to be used in encryption process of given field (together with salt set globally) default = 'password' |
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.
**Settings.** | |
each CryptoField has 2 kwargs *salt_settings_env* and *password*. | |
* *salt_settings_env* - name of variable stored in settings.py file, which will be used as cryptographic salt. default: salt_settings_env = 'SECRET_KEY' if settings not set or no SECRET_KEY in setting default: salt = "Salt123!!!" | |
* *password* - password to be used in encryption process of given field (together with salt set globally) default = 'password' | |
### Settings | |
Each CryptoField has 2 kwargs: `salt_settings_env` and `password`. | |
* `salt_settings_env`: Name of the variable stored in the settings.py file, which will be used as cryptographic salt. | |
**Default**: `"SECRET_KEY"` | |
**Note**: If `salt_settings_env` does not exist in `settings.py` then `salt` will be `Salt123!!!` | |
* `password`: Password to be used in the encryption process of the given field (together with salt set globally). | |
**Default**: `"password"` |
README.md
Outdated
**Example.** | ||
|
||
```python | ||
# models.py | ||
|
||
from django.db import models | ||
|
||
from drf_extra_fields.crypto_field import * | ||
|
||
class TestCryptoEmail(models.Model): | ||
|
||
value = CryptoEmailField(salt_settings_env='NEW_SECRET_KEY', password='new_password') |
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.
**Example.** | |
```python | |
# models.py | |
from django.db import models | |
from drf_extra_fields.crypto_field import * | |
class TestCryptoEmail(models.Model): | |
value = CryptoEmailField(salt_settings_env='NEW_SECRET_KEY', password='new_password') | |
## Example | |
```python | |
# models.py | |
from django.db import models | |
from drf_extra_fields.crypto_field import CryptoEmailField | |
class User(models.Model): | |
encrypted_email = CryptoEmailField(salt_settings_env='NEW_SECRET_KEY', password='new_password') |
drf_extra_fields/crypto_field.py
Outdated
def to_bytes(_obj): | ||
if isinstance(_obj, bytes): | ||
return _obj | ||
elif isinstance(_obj, (bytearray, memoryview)): | ||
return force_bytes(_obj) | ||
else: | ||
return pickle.dumps(_obj) |
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 method doesn't work as expected for
bytearray
:
to_bytes(bytearray([1, 2, 3, 4, 5]))
# b"bytearray(b'\\x01\\x02\\x03\\x04\\x05')"
because force_bytes
casts bytearray
to str
and then convert it to bytes
. Here we don't need to use force_bytes
, we can cast the object to bytes
directly for bytearray
and memoryview
.
-
What about calling this method
_to_bytes()
, since this is a specific method forcrypto_field
? -
What is the purpose of the underscore in
_obj
here? I guess using_to_bytes(obj)
is fine.
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 the new version, I use validators rather than force casting types
drf_extra_fields/crypto_field.py
Outdated
try: | ||
from functools import cached_property as property_decorator | ||
except ImportError: | ||
from builtins import property as property_decorator | ||
import base64 | ||
import pickle | ||
from django.utils.translation import gettext_lazy as _ | ||
from cryptography.fernet import Fernet | ||
from cryptography.hazmat.primitives import hashes | ||
from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC | ||
from django.conf import settings | ||
from django.core.checks import Error | ||
from django.core.exceptions import ImproperlyConfigured | ||
from django.db import models | ||
from django.utils.encoding import force_bytes |
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.
try: | |
from functools import cached_property as property_decorator | |
except ImportError: | |
from builtins import property as property_decorator | |
import base64 | |
import pickle | |
from django.utils.translation import gettext_lazy as _ | |
from cryptography.fernet import Fernet | |
from cryptography.hazmat.primitives import hashes | |
from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC | |
from django.conf import settings | |
from django.core.checks import Error | |
from django.core.exceptions import ImproperlyConfigured | |
from django.db import models | |
from django.utils.encoding import force_bytes | |
try: | |
from functools import cached_property as property_decorator | |
except ImportError: | |
from builtins import property as property_decorator | |
import base64 | |
import pickle | |
from cryptography.fernet import Fernet | |
from cryptography.hazmat.primitives import hashes | |
from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC | |
from django.conf import settings | |
from django.core.checks import Error | |
from django.core.exceptions import ImproperlyConfigured | |
from django.db import models | |
from django.utils.encoding import force_bytes | |
from django.utils.translation import gettext_lazy as _ |
drf_extra_fields/crypto_field.py
Outdated
def get_db_prep_save(self, value, connection, prepared=False): | ||
if self.empty_strings_allowed and value == bytes(): | ||
value = "" | ||
value = super().get_db_prep_value(value, connection, prepared=False) | ||
if value is not None: | ||
encrypted_value = self.encrypt(value) | ||
return encrypted_value | ||
# return connection.Database.Binary(encrypted_value) |
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.
What about something like that?
def get_db_prep_save(self, value, connection, prepared=False): | |
if self.empty_strings_allowed and value == bytes(): | |
value = "" | |
value = super().get_db_prep_value(value, connection, prepared=False) | |
if value is not None: | |
encrypted_value = self.encrypt(value) | |
return encrypted_value | |
# return connection.Database.Binary(encrypted_value) | |
def get_db_prep_save(self, value, connection, prepared=False): | |
if value: | |
value = self.encrypt(value) | |
return super().get_db_prep_value(value, connection, prepared) |
requirements.txt
Outdated
@@ -1,2 +1,3 @@ | |||
Django >= 2.2 | |||
djangorestframework >= 3.9.2 | |||
cryptography |
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.
Could you please pin the version?
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.
version 3.2.1 is the last with python 3.5 support therefore I have pined that one
tests/test_crypto.py
Outdated
) | ||
|
||
|
||
class PersonTest(TestCase): |
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.
class PersonTest(TestCase): | |
class CryptoFieldsTests(TestCase): |
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 what about creating a model consisting of all crypto fields and making some insert & update & delete operations to make sure that the whole integration is working?
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 tested in that matter in updated version
tests/test_crypto.py
Outdated
@@ -0,0 +1,100 @@ | |||
import datetime |
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.
Could you please rename this file to test_crypto_fields.py
?
drf_extra_fields/crypto_field.py
Outdated
@@ -0,0 +1,211 @@ | |||
try: |
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.
Could you please rename this file to crypto_fields.py
? It makes more sense while importing:
from drf_extra_fields.crypto_fields import CryptoTextField
Good catch @omerfarukabaci . I didn't realize that. You are right our package includes only DRF fields. I believe this pr is more suitable to https://django-extensions.readthedocs.io/en/latest/field_extensions.html |
I am really sorry for the obvious mistake, I will rewrite it to operate as
serialization and include all of your comments from the review. I have few
ideas in that matter
Can I write you directly in case of seeking consultation on how to get the
most utility out of it?
Kamil
wt., 11 maj 2021 o 18:15 alicertel ***@***.***> napisał(a):
… Thank you for your PR, we much appreciate it! However, I guess there is a
misunderstanding here (and yes, I have realized it now, after reviewing
your PR... 😓). This package includes extra fields for DRF (Django Rest
Framework) Serializer fields, not for Django model fields (at least until
now). I don't know what can/should we do about it. Any thoughts @alicertel
<https://github.com/alicertel>?
Good catch @omerfarukabaci <https://github.com/omerfarukabaci> . I didn't
realize that. You are right our package includes only DRF fields. I believe
this pr is more suitable to
https://django-extensions.readthedocs.io/en/latest/field_extensions.html
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#160 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANBYAGQUJPPUJFYADSVB7LDTNFJ2TANCNFSM43QRZHIA>
.
|
@omerfarukabaci @alicertel everything has been rewritten to drf fields. I will be grateful for another review |
Thanks @kamil1marczak I will review this within this week. |
@alicertel hey any progress? |
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 @kamil1marczak I will review this within this week.
@alicertel hey any progress?
Apologies for the late review. I've been reviewing your PR but it's not finished. I will send it anyway and continue reviewing this week.
@omerfarukabaci Could you please help me review this PR?
requirements.txt
Outdated
@@ -1,2 +1,3 @@ | |||
Django >= 2.2 | |||
djangorestframework >= 3.9.2 | |||
cryptography==3.2.1 |
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.
How about we make cryptography an optional dependency? Please see https://github.com/Hipo/drf-extra-fields/blob/master/setup.py#L19
Pillow is an optional dependency and it's only required if Base64ImageField is used.
We can raise an error if the cryptography is not installed when the one of the crypto fields are initialized. How that sounds?
drf_extra_fields/crypto_fields.py
Outdated
except InvalidToken: | ||
return None |
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.
Why this is failing silently? There is something wrong here and this code block is hiding it. I think we should raise the error. What do you think?
value = super(CryptoCharField, self).to_internal_value(value) | ||
if value: | ||
return value.decode("utf-8") | ||
self.fail("invalid") |
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.
It's possible value to be None https://github.com/Hipo/drf-extra-fields/pull/160/files#diff-d6824db8ef32e9fad7a3e9a7ba84738d5a9c233da178e29aea2479e13e40f8fbR92
It will fail in that case but it shouldn't
README.md
Outdated
## CryptoBinaryField and CryptoCharField | ||
|
||
+ A django-rest-framework fields for handling encryption through serialisation. Inputs are String object and internal | ||
python representation is Binary object for CryptoBinaryField and String object for CryptoCharField | ||
|
||
+ It takes the optional parameter `salt` (Django SECRET_KEY imported from setting as default). If set it use custom | ||
cryptographic salt | ||
+ It takes the optional parameter `password` ("Non_nobis1solum?nati!sumus" as default). If set it use a custom password | ||
in encryption. **It is highly recommended to use custom one!!** | ||
+ It takes the optional parameter `ttl` (None as default). If set it manage the number of seconds old a message may be | ||
for it to be valid. If the message is older than ttl seconds (from the time it was originally created) field will | ||
return None and encrypted message will not be enabled for decryption. | ||
|
||
**Example** | ||
|
||
```python | ||
from rest_framework import serializers | ||
from drf_extra_fields.crypto_fields import CryptoCharField | ||
|
||
|
||
class CryptoSerializer(serializers.Serializer): | ||
crypto_char = CryptoCharField() | ||
|
||
``` | ||
**Example with parameters** | ||
+ It takes custom salt and password. Once saved it will be aviable for decryption for 1000 seconds. | ||
|
||
```python | ||
from rest_framework import serializers | ||
from drf_extra_fields.crypto_fields import CryptoCharField | ||
|
||
|
||
class CryptoSerializer(serializers.Serializer): | ||
crypto_char = CryptoCharField(salt="custom salt", password="custom password", ttl=1000) | ||
|
||
``` | ||
|
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 there are bunch of changes in the README file. Could you please revert them back and only have the documentation of the newly added fields.
I assume these changes caused by the IDE/Editor you are using.
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.
Agreed, it might be because of auto formatting or something.
return decrypted_message.decode("utf-8") | ||
|
||
|
||
class CryptoBinaryField(serializers.Field): |
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.
Could you please tell me your use case for this field? I am curious about why you want to store binary data in DB (judging by your previous PR I am assuming these data will go to db)?
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.
Thank you for the changes! 🚀 I have made some change requests.
README.md
Outdated
## CryptoBinaryField and CryptoCharField | ||
|
||
+ A django-rest-framework fields for handling encryption through serialisation. Inputs are String object and internal | ||
python representation is Binary object for CryptoBinaryField and String object for CryptoCharField | ||
|
||
+ It takes the optional parameter `salt` (Django SECRET_KEY imported from setting as default). If set it use custom | ||
cryptographic salt | ||
+ It takes the optional parameter `password` ("Non_nobis1solum?nati!sumus" as default). If set it use a custom password | ||
in encryption. **It is highly recommended to use custom one!!** | ||
+ It takes the optional parameter `ttl` (None as default). If set it manage the number of seconds old a message may be | ||
for it to be valid. If the message is older than ttl seconds (from the time it was originally created) field will | ||
return None and encrypted message will not be enabled for decryption. | ||
|
||
**Example** | ||
|
||
```python | ||
from rest_framework import serializers | ||
from drf_extra_fields.crypto_fields import CryptoCharField | ||
|
||
|
||
class CryptoSerializer(serializers.Serializer): | ||
crypto_char = CryptoCharField() | ||
|
||
``` | ||
**Example with parameters** | ||
+ It takes custom salt and password. Once saved it will be aviable for decryption for 1000 seconds. | ||
|
||
```python | ||
from rest_framework import serializers | ||
from drf_extra_fields.crypto_fields import CryptoCharField | ||
|
||
|
||
class CryptoSerializer(serializers.Serializer): | ||
crypto_char = CryptoCharField(salt="custom salt", password="custom password", ttl=1000) | ||
|
||
``` | ||
|
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.
Agreed, it might be because of auto formatting or something.
class CryptoSerializer(serializers.Serializer): | ||
message = CryptoBinaryField(required=False) |
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.
Let's call this BaseCryptoSerializer
and create other serializers in the test methods using this:
def test_serialization_with_salt(self):
class CryptoSerializer(BaseCryptoSerializer):
message = CryptoBinaryField(password=..., salt=...)
encrypted_message = _encrypt(token, message) | ||
model_data = SaveCrypto(message=encrypted_message, created=now) | ||
serializer = TtlCryptoSerializerSerializer(model_data) | ||
time.sleep(3) |
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.
We cannot use time.sleep
in tests, we should mock time.time
.
tests/test_crypto_fields.py
Outdated
created = serializers.DateTimeField() | ||
|
||
|
||
class PointSerializerTest(TestCase): |
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.
Let's make sure that every AssertionErrror and ValidationError is tested.
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Co-authored-by: Ömer Faruk Abacı <[email protected]>
Hi I have added package of encrypted fields,