-
Notifications
You must be signed in to change notification settings - Fork 106
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
Implement warnings #688
Implement warnings #688
Changes from 4 commits
b4e6d0d
de02df9
a701b04
6675e12
e0b4d66
b8d1332
c42f7c2
a091308
27c8008
02e17b2
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 |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Generated by Django 4.0.5 on 2022-10-24 14:29 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("chains", "0036_alter_chain_transaction_service_uri_and_more"), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name="chain", | ||
name="warning", | ||
field=models.CharField(blank=True, max_length=255, null=True), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Generated by Django 4.0.5 on 2022-10-24 15:43 | ||
|
||
from django.apps.registry import Apps | ||
from django.contrib.auth.models import Group, Permission | ||
from django.db import migrations | ||
from django.db.backends.base.schema import BaseDatabaseSchemaEditor | ||
|
||
|
||
def create_support_group(apps: Apps, schema_editor: BaseDatabaseSchemaEditor) -> None: | ||
support_group, created = Group.objects.get_or_create(name="support") | ||
permission = Permission.objects.filter(codename="change_chain") | ||
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. Where is this permission defined? It should be on |
||
if permission: | ||
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. Permission must exist we shouldn't check this 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 was getting an error when running tests due to this, that's why I included the condition. Maybe it's related with not having the permission defined in 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. That's it. Permission should be created in a migration when adding it to 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. @Uxio0 I was testing a lot trying to understand why the migration was failing (because the permission wasn't found), I think the reason why we need 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 this issue is related. This also explains why test were failing in my local setup (I understand the whole list of migrations gets executed against a test database when running the tests). |
||
support_group.permissions.add(permission[0]) | ||
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. If you only expect to get one permission you can do |
||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
("chains", "0037_chain_warning"), | ||
] | ||
|
||
operations = [migrations.RunPython(create_support_group, migrations.RunPython.noop)] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -119,6 +119,7 @@ class RpcAuthentication(models.TextChoices): | |
recommended_master_copy_version = models.CharField( | ||
max_length=255, validators=[sem_ver_validator] | ||
) | ||
warning = models.CharField(max_length=255, blank=True, null=True) | ||
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. Maybe a 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. Yes, thanks! This is something I forgot to annotate in the PR, but I was doubting about it as well 👍 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. Changed in 02e17b2 |
||
|
||
def get_disabled_wallets(self) -> QuerySet["Wallet"]: | ||
all_wallets = Wallet.objects.all() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,7 @@ class ChainSerializer(serializers.ModelSerializer[Chain]): | |
ens_registry_address = EthereumAddressField() | ||
disabled_wallets = serializers.SerializerMethodField() | ||
features = serializers.SerializerMethodField() | ||
warning = serializers.CharField() | ||
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. Was there any decision on the name of this field? We should be aligned with the clients regarding this. Can this field be used to have just some information out (which is not a warning) for example? 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. It is named |
||
|
||
class Meta: | ||
model = Chain | ||
|
@@ -140,6 +141,7 @@ class Meta: | |
"recommended_master_copy_version", | ||
"disabled_wallets", | ||
"features", | ||
"warning", | ||
] | ||
|
||
@staticmethod | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,7 @@ def test_json_payload_format(self) -> None: | |
"recommendedMasterCopyVersion": chain.recommended_master_copy_version, | ||
"disabledWallets": [], | ||
"features": [], | ||
"warning": None, | ||
} | ||
], | ||
} | ||
|
@@ -195,6 +196,7 @@ def test_json_payload_format(self) -> None: | |
"recommendedMasterCopyVersion": chain.recommended_master_copy_version, | ||
"disabledWallets": [], | ||
"features": [], | ||
"warning": None, | ||
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. We are missing a test if the warning is set. The factory method should also be updated 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. Added in 02e17b2 👍🏻 |
||
} | ||
|
||
response = self.client.get(path=url, data=None, format="json") | ||
|
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 would prefer to use custom permissions instead of relying on the group, as in Django a group is only an aggregation of permissions: https://docs.djangoproject.com/en/4.1/topics/auth/customizing/#custom-permissions
Example: https://github.com/safe-global/safe-transaction-service/blob/6230a980bad24a0997c334f206279f96bc3465c1/safe_transaction_service/history/serializers.py#L246
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 having a new table for the warning messages? ChainId + Message.
The endpoint can return the message from this table. The permission then can be done on the table level. Maybe it's easier and simpler this way wdyt?
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 would prefer not if it's possible to do it in another way, I like having a normalized database if possible
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 was thinking more about this field and in a way it seems that it's not really part of the chain metadata so that's why I thought about a separate table. Even though it's possible in a different way the Chain model itself already has quite some fields. But just an idea (nothing that would block the feature).
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 was thinking in having a separated table as well, it could be nice to keep an history of the changes as well, but I initially discarded it because it seems a bit overkill for the feature.
About the custom permission... it was my initial approach as well, but in this case we need to still grant edit permission over all Chain fields to superusers, and only over
warning
field for a selected group of users. I thinking in creating an additional permission, something likecan_change_all_chain_fields
and when a user doesn't have it, show all the fields as read-only exceptwarning
. Do you see a simpler way here?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 think I'm missing something, but why not using
if request.user.has_perm("app.permission_name")
?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.
Maybe it's me the one missing something, but we need to avoid support group from modifying any field except for
warning
right? So it's kind of negative permission. I mean, all the other fields are the ones to restrict to some users (support), right?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 see it more like:
support
group will grant theedit chains
andchange_only_warning
to all his members.edit chains
permissionchange_only_warning
permission will be able to modify onlywarning
field, soif request.user.has_perm(
change_only_warning):
every field but warning will be read_onlyThere 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.
@Uxio0 thanks for your feedback and guidance on this 🙂. I was doing some tests and it seems that when creating a new permission (
change_only_warning
in this case), that permission is automatically inherited by superuser(s). So, in this case, all fields are converted toread_only
for the superusers as well (exceptwarning
of course). 🤔 I see two options here:superadmin
before marking the fields as read-only.I think the second option is better, 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.
I implemented the second option, of course if you see any concerns with it let me know 🙂