strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
391 stars 115 forks source link

The right way to protect and filter information #523

Open DephPhascow opened 2 months ago

DephPhascow commented 2 months ago

Hello, I can say that I am new to this issue and I would like to clarify how to implement such a design correctly: There are 4 models available:

class UserModel(AbstractBaseUser, PermissionsMixin):
    tg_id = models.CharField(max_length=100, unique=True)
    ...

class DoctorModel(models.Model):
    user = models.ForeignKey(UserModel, on_delete=models.CASCADE)

class PatientModel(models.Model):
    user = models.ForeignKey(UserModel, on_delete=models.CASCADE)

class BookingModel(models.Model):
    doctor = models.ForeignKey(DoctorModel, on_delete=models.CASCADE)
    patient = models.ForeignKey(PatientModel, on_delete=models.CASCADE)

There are already entries in the BookingModel model:

[
 {
    doctor: 1,
    patient: 1,
 },
 {
    doctor: 1,
    patient: 2,
 }, 
 {
    doctor: 1,
    patient: 3,
 }, 
 {
    doctor: 1,
    patient: 4,
 }, 
 {
    doctor: 2,
    patient: 4,
 },  
 {
    doctor: 2,
    patient: 5,
 },  
]

query:

bookings: List[types.BookingModelType] = strawberry_django.field(permission_classes=[IsAuthenticated], )

class IsAuthenticated(BasePermission):
    message = "No access"
    def has_permission(self, source: typing.Any, info: Info, **kwargs) -> bool:
        user = info.context["request"].user
        return user and user.is_authenticated and user.is_active

I do authorization of requests via strawberry-django-auth using the JWT token The essence of the gap is that if I make a request

query getMyBookings {
  bookings {
    patinet
  }
}

Then I get a list of all patients, including other doctors, when I need to get only "my patients" (If I logged in as doctor id: 2, then I have patients only id 4 and 5 Of course, I can add filters like:

query getMyBookings {
  bookings (
    filters: {
        doctor: {
            id: {
                exact: 2
            }
        }
    }
  ) {
    patinet
  }
}

Then I really only get ids 4 and 5. But what prevents me from faking the request? Through the browser developer console, I will see where the request is being sent and which request. I'll copy and paste, but instead of id 2, I'll specify id 1 and now I see all the records of the first doctor, which violates confidentiality. Therefore, tell me, maybe I'm not doing it right or I'm confusing something.

PS: This is the first time I'm writing questions on github, so I might not have written/framed the question correctly, I'm sorry

Upvote & Fund

Fund with Polar

sdobbelaere commented 2 months ago

Sounds you want to hammer that out in the backend to ensure you cannot fetch non-related info from the frontend.

In Onesila.com we wanted to ensure all information is attached to an owner, which in this case is company.

To do this, we auto-filter on company and every mutation will auto-assign a new entry to company via the backend. This way all information is safe as the filtering doesn't happen in the frontend and therefore cannot be fiddled with.

For queries, this was done with a custom node: https://github.com/OneSila/OneSilaHeadless/blob/c32bffbfe730e2bb350a86c6ea9bad9091bc1ba2/OneSila/core/schema/core/queries.py#L12

For the mutations, this was done with custom Create, Update and Delete mutations by subclassing the original mutations. https://github.com/OneSila/OneSilaHeadless/blob/c32bffbfe730e2bb350a86c6ea9bad9091bc1ba2/OneSila/core/schema/core/mutations.py#L19

This approach does assume that the user-model is reachable through the patient-model as it's the logged-in user that will determine which data is shown.

The example I linked here, has a slightly different structure as we have users and multi-tenant-owners since it's a Saas application. But it sounds like the code from OneSila can be easily adjusted to do what you want it do.

DephPhascow commented 2 months ago

Sounds you want to hammer that out in the backend to ensure you cannot fetch non-related info from the frontend.

In Onesila.com we wanted to ensure all information is attached to an owner, which in this case is company.

To do this, we auto-filter on company and every mutation will auto-assign a new entry to company via the backend. This way all information is safe as the filtering doesn't happen in the frontend and therefore cannot be fiddled with.

For queries, this was done with a custom node: https://github.com/OneSila/OneSilaHeadless/blob/c32bffbfe730e2bb350a86c6ea9bad9091bc1ba2/OneSila/core/schema/core/queries.py#L12

For the mutations, this was done with custom Create, Update and Delete mutations by subclassing the original mutations. https://github.com/OneSila/OneSilaHeadless/blob/c32bffbfe730e2bb350a86c6ea9bad9091bc1ba2/OneSila/core/schema/core/mutations.py#L19

This approach does assume that the user-model is reachable through the patient-model as it's the logged-in user that will determine which data is shown.

The example I linked here, has a slightly different structure as we have users and multi-tenant-owners since it's a Saas application. But it sounds like the code from OneSila can be easily adjusted to do what you want it do.

Thank you for your reply, I will study and try it in practice

DephPhascow commented 2 months ago

I've found one solution that works for me, but unfortunately it only works if filtering is present in the query. If there is no filtering, then it does not work and the person sees all the recordings

@strawberry_django.filter(models.AppoitmentModel, lookups=True)
class AppoitmentModelFilter:
    id: auto
    worker: typing.Optional[DoctorModel]
    patient: typing.Optional[PatientModelFilter]

    @strawberry_django.filter_field
    def filter(
        self,
        info: Info,
        queryset: QuerySet,
        prefix: str,
    ):
        user = info.context["request"].user
        if user.groups.filter(name='Doctor').exists():
            queryset = queryset.filter(worker__doctor__user=user)
        elif user.groups.filter(name='Patient').exists():
            queryset = queryset.filter(patient__user=user)
        elif not user.is_superuser:
            raise Exception('No access')
        return strawberry_django.process_filters(
            self,
            info=info,
            queryset=queryset,
            prefix=prefix,
            skip_object_filter_method=True,
        )
sdobbelaere commented 2 months ago

You really want to go more upstream and filter it at the deepest level where you have access to both the data and logged in user. That's why in the linked project it's done on the actual query and mutation level.

bellini666 commented 1 month ago

One option here is to use the permissions extension: https://strawberry-graphql.github.io/strawberry-django/guide/permissions/

You can create a subclass of DjangoPermissionExtension and define your own logic on resolve_for_user

You can see in the HasPerm class some examples. Feel free to ask here or ping me on discord if you need any help with that