Implement warnings #688

hectorgomezv · 2022-10-25T08:50:30Z

Relates to #675

This PR aims to create a new warning field for each Chain. This new warning field is nullable and should be updatable by superusers, and also for support collaborators. That's why a new support group is created. The idea is to make all Chain fields read-only for support collaborators, except warning, so this group can only edit the warning field.

Using this approach we avoid the usage of third party libraries or external dependencies. It's implemented by just using Django features. Feedback is welcomed! 🙂

coveralls · 2022-10-25T08:52:44Z

Pull Request Test Coverage Report for Build 3446239714

23 of 28 (82.14%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.5%) to 99.227%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/chains/admin.py	3	8	37.5%

Totals
Change from base Build 3417671914:	-0.5%
Covered Lines:	899
Relevant Lines:	906

💛 - Coveralls

Uxio0 · 2022-10-25T08:56:26Z

src/chains/admin.py

+    def get_readonly_fields(
+        self, request: HttpRequest, obj: Optional[Chain] = None
+    ) -> List[str]:
+        if request.user.groups.filter(name="support"):  # type: ignore


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

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?

I would prefer not if it's possible to do it in another way, I like having a normalized database if possible

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).

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 like can_change_all_chain_fields and when a user doesn't have it, show all the fields as read-only except warning. Do you see a simpler way here?

I think I'm missing something, but why not using if request.user.has_perm("app.permission_name")?

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?

I see it more like:

By default users don't have any permission unless you make them superusers or give them permissions to every table individually.

support group will grant the edit chains and change_only_warning to all his members.

To edit the chains table users will need the edit chains permission

People with change_only_warning permission will be able to modify only warning field, so if request.user.has_perm(change_only_warning): every field but warning will be read_only

@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 to read_only for the superusers as well (except warning of course). 🤔 I see two options here:

Rely on the group.

Have an additional check to verify the user is not a superadmin before marking the fields as read-only.

if ( request.user.has_perm("change_only_warning") and not request.user.is_superuser ):

I think the second option is better, what do you think?

I implemented the second option, of course if you see any concerns with it let me know 🙂

Uxio0 · 2022-10-25T09:00:15Z

src/chains/models.py

@@ -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)


Maybe a TextField?

Yes, thanks! This is something I forgot to annotate in the PR, but I was doubting about it as well 👍

Changed in 02e17b2

Uxio0 · 2022-10-25T09:03:08Z

src/chains/migrations/0038_auto_20221024_1543.py

+
+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")


Where is this permission defined? It should be on Meta for the Chain model: https://docs.djangoproject.com/en/4.1/topics/auth/customizing/#custom-permissions

Uxio0 · 2022-10-25T09:03:41Z

src/chains/migrations/0038_auto_20221024_1543.py

+    support_group, created = Group.objects.get_or_create(name="support")
+    permission = Permission.objects.filter(codename="change_chain")
+    if permission:
+        support_group.permissions.add(permission[0])


If you only expect to get one permission you can do Permission.objects.get(codename="change_chain")

Uxio0 · 2022-10-25T09:04:15Z

src/chains/migrations/0038_auto_20221024_1543.py

+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")
+    if permission:


Permission must exist we shouldn't check this

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 Meta as you said in the previous comment...

That's it. Permission should be created in a migration when adding it to Meta, and you can use that migration to add it to the group. Also, if possible, do everything in the same migration

@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 Permission.objects.get_or_create here instead of simply Permission.objects.get is because the permissions data is not inferred/written to the database until all migrations are executed (using the post_migrate signal if I'm not wrong).

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).

src/chains/migrations/0037_chain_warning.py

fmrsabino · 2022-11-09T10:55:06Z

src/chains/models.py

@@ -129,6 +130,9 @@ def get_disabled_wallets(self) -> QuerySet["Wallet"]:
    def __str__(self) -> str:
        return f"{self.name} | chain_id={self.id}"

+    class Meta:
+        permissions = [("change_only_warning", "Can change only warning")]


I would name it change_warning with the description: Can change the chain warning.

I think "only" might be redundant if the permission is tied to this field.

only is included in the name because of the nature of this permission. This permission will imply two things for an user:

The user can edit the warning field.

The user cannot edit any other Chain field, i.e. the rest of the fields will be marked as read-only.

I know it is not very intuitive, but we agreed on taking this approach to avoid using a third party dependency which would let us define field-level permissions. Even using that dependency, I suspect the outcome wouldn't be very intuitive either, because our goal is not to grant new permissions, but preventing some users to access some fields.

fmrsabino · 2022-11-09T10:56:46Z

src/chains/serializers.py

@@ -118,6 +118,7 @@ class ChainSerializer(serializers.ModelSerializer[Chain]):
    ens_registry_address = EthereumAddressField()
    disabled_wallets = serializers.SerializerMethodField()
    features = serializers.SerializerMethodField()
+    warning = serializers.CharField()


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?

It is named warning because of the issue this change is related: New "warning" text field in chain configs 🙂 But for sure we can sync about of this.

fmrsabino · 2022-11-09T11:11:18Z

src/chains/admin.py

+    ) -> List[str]:
+        if (
+            request.user.has_perm("chains.change_only_warning")
+            and not request.user.is_superuser


Why don't we allow a superuser to change this field? (even without the permission)

(Please refer to the previous comment for reference)

This function (get_readonly_fields) returns the read-only fields, so for an user which have the change_only_warning and is not a superuser, will return all the Chain fields except warning. For superusers, it will return [], so all the fields are editable. In short, we allow superusers to change this field.

(I know this code is not super intuitive to read, please let me know if you find a better approach).

fmrsabino · 2022-11-09T11:12:21Z

src/chains/tests/test_views.py

@@ -195,6 +196,7 @@ def test_json_payload_format(self) -> None:
            "recommendedMasterCopyVersion": chain.recommended_master_copy_version,
            "disabledWallets": [],
            "features": [],
+            "warning": None,


We are missing a test if the warning is set.

The factory method should also be updated

Added in 02e17b2 👍🏻

hectorgomezv · 2022-11-11T15:53:58Z

I'm drafting this PR since there is an ongoing discussion in Notion.

hectorgomezv · 2023-04-19T11:46:06Z

I'm closing this PR because the source issue is also closed now. Please reach out if you want to discuss this feature further 🙂

hectorgomezv added 4 commits October 25, 2022 09:51

Add support group and make readonly chain fields

b4e6d0d

Filter permissions to avoid testing failure

de02df9

Sort imports, add typing

a701b04

Add typings

6675e12

hectorgomezv requested a review from a team as a code owner October 25, 2022 08:50

hectorgomezv requested review from fmrsabino, moisses89 and luarx October 25, 2022 08:50

Uxio0 requested changes Oct 25, 2022

View reviewed changes

hectorgomezv added 2 commits October 31, 2022 17:31

Merge branch 'main' into implement-warnings

e0b4d66

Merge branch 'main' into implement-warnings

b8d1332

hectorgomezv requested a review from Uxio0 November 3, 2022 16:50

hectorgomezv added 2 commits November 3, 2022 17:54

Modify migration, add Meta data, change readonly condition

c42f7c2

Add permission migration

a091308

luarx removed their request for review November 3, 2022 21:54

fmrsabino suggested changes Nov 9, 2022

View reviewed changes

hectorgomezv added 2 commits November 11, 2022 16:28

Merge branch 'main' into implement-warnings

27c8008

Change field definition, add tests

02e17b2

hectorgomezv marked this pull request as draft November 11, 2022 15:54

hectorgomezv closed this Apr 19, 2023

github-actions bot locked and limited conversation to collaborators Apr 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement warnings #688

Implement warnings #688

hectorgomezv commented Oct 25, 2022

coveralls commented Oct 25, 2022 •

edited

Loading

Uxio0 Oct 25, 2022

fmrsabino Oct 25, 2022

Uxio0 Oct 25, 2022 •

edited

Loading

fmrsabino Oct 25, 2022

hectorgomezv Oct 25, 2022 •

edited

Loading

Uxio0 Oct 25, 2022

hectorgomezv Oct 25, 2022 •

edited

Loading

Uxio0 Oct 28, 2022

hectorgomezv Nov 2, 2022 •

edited

Loading

hectorgomezv Nov 3, 2022

Uxio0 Oct 25, 2022

hectorgomezv Oct 25, 2022 •

edited

Loading

hectorgomezv Nov 11, 2022

Uxio0 Oct 25, 2022

Uxio0 Oct 25, 2022

Uxio0 Oct 25, 2022

hectorgomezv Oct 25, 2022

Uxio0 Oct 25, 2022

hectorgomezv Nov 3, 2022 •

edited

Loading

hectorgomezv Nov 3, 2022

fmrsabino Nov 9, 2022

hectorgomezv Nov 9, 2022 •

edited

Loading

fmrsabino Nov 9, 2022

hectorgomezv Nov 9, 2022

fmrsabino Nov 9, 2022

hectorgomezv Nov 9, 2022 •

edited

Loading

fmrsabino Nov 9, 2022

hectorgomezv Nov 11, 2022

hectorgomezv commented Nov 11, 2022

hectorgomezv commented Apr 19, 2023

Implement warnings #688

Implement warnings #688

Conversation

hectorgomezv commented Oct 25, 2022

coveralls commented Oct 25, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3446239714

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uxio0 Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorgomezv commented Nov 11, 2022

hectorgomezv commented Apr 19, 2023

coveralls commented Oct 25, 2022 •

edited

Loading

Uxio0 Oct 25, 2022 •

edited

Loading

hectorgomezv Oct 25, 2022 •

edited

Loading

hectorgomezv Oct 25, 2022 •

edited

Loading

hectorgomezv Nov 2, 2022 •

edited

Loading

hectorgomezv Oct 25, 2022 •

edited

Loading

hectorgomezv Nov 3, 2022 •

edited

Loading

hectorgomezv Nov 9, 2022 •

edited

Loading

hectorgomezv Nov 9, 2022 •

edited

Loading