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

The right way to protect and filter information #523

Open
DephPhascow opened this issue Apr 26, 2024 · 5 comments
Open

The right way to protect and filter information #523

DephPhascow opened this issue Apr 26, 2024 · 5 comments

Comments

@DephPhascow
Copy link

DephPhascow commented Apr 26, 2024

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

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@sdobbelaere
Copy link
Contributor

sdobbelaere commented Apr 26, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Member

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

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

No branches or pull requests

3 participants