Skip to content
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

feat: support union operator on BasePermission #3315

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

replygirl
Copy link

@replygirl replygirl commented Jan 3, 2024

Description

👉 tl;dr: Lets you use | between permission instances, e.g. is_cool_color = IsBlue() | IsGreen()

  • Added: BasePermission.__or__ instance method, which returns an OrPermission instance
  • Added: OrPermission, inheriting from BasePermission
    • __init__ takes a tuple of BasePermission instances to be evaluated
    • has_permission returns True as long as has_permission returns True from any of its input permissions
    • message and on_unauthorized refer to the last failed permission

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).
    • Also fully tested on the job 👍

@botberry
Copy link
Member

botberry commented Jan 3, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Permissions can now be combined with the | operator to require the user pass either of the has_permission checks:

import strawberry
from strawberry.permission import PermissionExtension


@strawberry.type
class User:
    @strawberry.field(
        extensions=[
            PermissionExtension(
                # require auth AND (node is current user OR current user is staff)
                permissions=[IsAuthenticated(), IsExactUser() | IsStaff()]
            )
        ]
    )
    def ssn(self) -> str:
        return "555-55-5555"

Here's the tweet text:

🆕 Release (next) is out! Thanks to replygirl for the PR 👏

Get it here 👉 https://beta.strawberry.rocks/release/(next)

Copy link
Member

@erikwrede erikwrede left a comment

Choose a reason for hiding this comment

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

This looks like a great usability addition. I will conduct more thorough testing later, but for now, can you please add the following test cases so we don't accidentally break something later:

  • two resolvers use the same OR filter -> directive duplication doesn't cause an error
  • permutation of entries in the OR filter in two separate resolve (My preference is to use the same directive in this case and not create two separate directives, i.e. always AorB instead of AorB and BorA. We can sort this out by ordering the permission names lexicographically using sort)

How should we handle multiple ORs that are chained? Should the or-filter flat map them into a single filter ,or should there be nested OR-filters? In this case, there should also be test cases on schema generation

  • More than 2 permissions disjuncted, (AorBorC)

@replygirl
Copy link
Author

replygirl commented Jan 3, 2024

@erikwrede all sounds good! i'll have time to come back to this in the next few days

Should the or-filter flat map them into a single filter (my preferred option),or should there be nested OR-filters?

nesting is my instinct, because it seems supporting other operators would be more straightforward, but i'll think about it more and look at DRF +others again

@erikwrede
Copy link
Member

erikwrede commented Jan 3, 2024

@replygirl the only problem I see with nesting is the handling of schema directives. Maybe they need to be flatMapped in this case, but this should be easy to handle since only the outermost OR-extension will be respected. I'm open to any solutions, so just go with what works best for the next draft 😊

Copy link

codspeed-hq bot commented Jan 3, 2024

CodSpeed Performance Report

Merging #3315 will not alter performance

Comparing replygirl:or-permission (c275f12) with main (6c7b11f)

Summary

✅ 11 untouched benchmarks

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Merging #3315 (c275f12) into main (6c7b11f) will decrease coverage by 1.82%.
The diff coverage is 93.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3315      +/-   ##
==========================================
- Coverage   96.61%   94.80%   -1.82%     
==========================================
  Files         483      482       -1     
  Lines       30161    30187      +26     
  Branches     3726     3728       +2     
==========================================
- Hits        29140    28618     -522     
- Misses        832     1364     +532     
- Partials      189      205      +16     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combining permissions with -OR-
3 participants