Skip to content

FilterList model and endpoints#371

Merged
lemonsaurus merged 33 commits into
masterfrom
whitelist_system
Aug 3, 2020
Merged

FilterList model and endpoints#371
lemonsaurus merged 33 commits into
masterfrom
whitelist_system

Conversation

@lemonsaurus
Copy link
Copy Markdown
Contributor

@lemonsaurus lemonsaurus commented Jul 16, 2020

This pull request introduces a new model, FilterList, and the prerequisite viewset, URL endpoint, migrations, serializer and tests.

The intention of this model is to store things that we will either whitelist or blacklist. For example, we'll be storing file formats, guild invite IDs, and other things that are currently in the bot config file, but that we want to be able to modify during runtime.

Eventually, we may set up automation where this table is synchronized against stuff like the Awesome Discord Programming List.

Review style: commit-by-commit.

Closes #305.

Routes

GET /bot/filter-lists

Returns all filterlist items in the database.

Response format

[
    {
        'id': "2309268224",
        'created_at': "01-01-2020 ...",
        'updated_at': "01-01-2020 ...",
        'type': "file_format",
        'allowed': 'true',
        'content': ".jpeg",
        'comment': "Popular image format.",
    },
    ...
]

Status codes

  • 200: returned on success
  • 401: returned if unauthenticated

GET /bot/filter-lists/<id:int>

Returns a specific FilterList item from the database.

Response format

{
    'id': "2309268224",
    'created_at': "01-01-2020 ...",
    'updated_at': "01-01-2020 ...",
    'type': "file_format",
    'allowed': 'true',
    'content': ".jpeg",
    'comment': "Popular image format.",
}

Status codes

  • 200: returned on success
  • 404: returned if the id was not found.

GET /bot/filter-lists/get-types

Returns a list of valid list types that can be used in POST requests.

Response format

[
    ["GUILD_INVITE","Guild Invite"],
    ["FILE_FORMAT","File Format"],
    ["DOMAIN_NAME","Domain Name"],
    ["FILTER_TOKEN","Filter Token"]
]

Status codes

  • 200: returned on success

POST /bot/filter-lists

Adds a single FilterList item to the database.

Request body

{
   'type': str,
   'allowed': bool,
   'content': str,
   'comment': Optional[str],
}

Status codes

  • 201: returned on success
  • 400: if one of the given fields is invalid

DELETE /bot/filter-lists/<id:int>

Deletes the FilterList item with the given id.

Status codes

  • 204: returned on success
  • 404: if a tag with the given id does not exist

@lemonsaurus lemonsaurus requested a review from a team as a code owner July 16, 2020 11:22
@ghost ghost added the needs 2 approvals label Jul 16, 2020
Leon Sandøy added 4 commits July 16, 2020 13:27
The test_utils_account.py tests were never running, because the folder
they were in had no __init__.py file.

The test_models.py file was failing because it had an outdated import of
the ModelReprMixin, which has moved to a new file.

#305
@lemonsaurus lemonsaurus changed the title AllowList model and endpoints AllowDenyList model and endpoints Jul 16, 2020
@MarkKoz MarkKoz added area: API Related to or causes API changes language: python Involves Python code priority: 2 - normal Normal Priority type: feature New feature or request labels Jul 16, 2020
@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Jul 19, 2020

An allowed field doesn't make sense to me because it facilitates the simultaneous maintenance of both a blacklist and whitelist for the same type. This means every possible value would have to exist in the database as explicitly allowed or denied, which is of course infeasible.

It makes more sense to enforce all records of a certain type to all be whitelist or all be a blacklist. Each type should have a default state (i.e. allowed or denied), and if an item of that type is present in the DB, then assume the opposite state. It can be up to the client to decide what that default state is. If you really need to have it in the DB, then it should be stored in a separate table as a mapping between type and allowed/denied rather than stored as a field for individual items in the list.

@lemonsaurus
Copy link
Copy Markdown
Contributor Author

lemonsaurus commented Jul 19, 2020

@MarkKoz

An allowed field doesn't make sense to me because it facilitates the simultaneous maintenance of both a blacklist and whitelist for the same type.

Some types need both a blacklist and a whitelist, for example the Guild Invite IDs. Here the idea is that some guild invites will be blacklisted and will never be permitted, some will be whitelisted and will always be permitted, and everything else will be permitted based on certain criteria (whether they are Partnered or Verified, in this case).

The UniqueConstraint makes it impossible for a blacklist and a whitelist for the same item to simultaneously exist, so every item is always exclusively on one of the two lists.

@lemonsaurus lemonsaurus dismissed MarkKoz’s stale review July 24, 2020 07:41

All requested changes fulfilled

@lemonsaurus lemonsaurus changed the title AllowDenyList model and endpoints FilterList model and endpoints Jul 27, 2020
Leon Sandøy added 5 commits July 29, 2020 13:55
I was handling this in a Django vanilla kind of way, which was causing
the constraint to return a 500 instead of a 400. This changes the
approach to use the DRF way, and makes it return 400.

It doesn't actually change the way anything behaves, other than
returning the right status code.
This really should've been handled automatically by DRF, and in the
future, it will be. But for now, we need to have constraints both on the
serializer (to get status code 400), and on the model (to prevent direct
database constraint violations).

See encode/django-rest-framework#7173
Copy link
Copy Markdown
Contributor

@SebastiaanZ SebastiaanZ left a comment

Choose a reason for hiding this comment

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

Alright, that's quite a lot of commits and files, but it looks solid. I've also ran some manual tests locally by polling the endpoints; looks good. One requested change to prevent us from using a wrong default value somewhere.

Comment thread pydis_site/apps/api/migrations/0025_allow_custom_inserted_at_infraction_field.py Outdated
@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 1 approval labels Jul 29, 2020
Co-authored-by: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
@ghost ghost added needs 1 approval and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 30, 2020
@lemonsaurus lemonsaurus dismissed SebastiaanZ’s stale review July 30, 2020 08:40

changes addressed

Copy link
Copy Markdown
Contributor

@SebastiaanZ SebastiaanZ left a comment

Choose a reason for hiding this comment

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

Nice work. Adding in the rework/fix/kaizen of the social account tests made it a slightly less focused PR, but I doubt fixing that would have otherwise been a major priority. Better to get it done.

@ghost ghost removed the needs 1 approval label Jul 30, 2020
@lemonsaurus
Copy link
Copy Markdown
Contributor Author

Now that we're gonna merge this pretty soon, I'll write a migration to get all the data ready.

Leon Sandøy added 2 commits July 30, 2020 11:29
This will populate the table with domain names, guild invites, filter
tokens and file formats.
Now that we have a migration that adds data, we can no longer have tests
that operate on the assumption that the database is going to be empty.

So, we're now clearing that table before these tests run.
@SebastiaanZ
Copy link
Copy Markdown
Contributor

Is this now ready for merge?

@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Aug 1, 2020

Best to wait until the bot PR is ready too, in case a review there requires changes here.

@SebastiaanZ SebastiaanZ added the status: deferred Will be done at a later time label Aug 1, 2020
@lemonsaurus lemonsaurus merged commit 685214c into master Aug 3, 2020
@lemonsaurus lemonsaurus deleted the whitelist_system branch August 3, 2020 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Related to or causes API changes language: python Involves Python code priority: 2 - normal Normal Priority status: deferred Will be done at a later time type: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Manage whitelisted guilds and file formats in DB.

3 participants