Authorization edge case handling

[Smtih]

When setting up entities for authorization using nestjs-query I've had a few issues that have tripped me up. It would be good to get some confirmation if there's already a known way to get these working or what if what I want conflicts with overall vision for the library. If these are just missing functionalities, I'll see If I can address any of them with a PR.

Issue 1 - Creation authorization.

I was hoping that I could prevent users creating a resource in a configuration that conflicts with the authorization filter.
I have managed to triage this in my own project by extending the query service to use the authorization filters with the inbuilt applyFilter on the create-dto. I think an option could be passed to the query-service to enable this behaviour (or the default behaviour)

Issue 2 - Related Creation Authorization
If we accept that the above is desired, I'd love an inbuilt way to prevent things being related to items that you don't have permissions to, I'm not sure what the implementation of this would be and at the moment I just allow this to happen which wouldn't be too big of a problem except for the next issue.

Issue 3 - Inferring data through relation filters
Once you have a filterable relation set up it's possible to construct queries that leak information about the items even if you don't have permission to that item but have permissions to a related item.
As per my example you can ask for subtasks where task meets condition X and it will not consider your permission to the tasks so you can find out that tasks meeting condition X exist in the system even if you don't have permission to any of them.
When joining relationships, I believe we need to apply the authorization filters to the join.

Here's a repo to demonstrate the issue: https://github.com/Smtih/netjs-query-auth-issues/blob/main/test/issue-3.e2e-spec.ts

[TriPSs]

Issue 1:
In my own projects I tend to do this in the before-create hook, a better solution is always welcome but I don't quite understand yours?

Issue 3:
I don't quite understand this one either, you say "no access" but the auth filter says complete: false access, so if I'm not mistaken the query applied to todoItem should be the merged auth filter and the filter provided? Is that not the case?

[Smtih]

Sorry I was trying to produce an example that was close to the boilerplate example used throughout the code, but I think I've made it too unrealistic so perhaps it's too hard to understand.

Issue 1:
In my app users belong to one or more groups all authorization is provided at the group level and my jwt contains the array of groups you belong to.

This means most of my apis have you specify which groupId you are creating the resource against and my auth filters look like { groupId: { in: context.req.user.groups } }

Here's a code diff of what I was thinking generalizing what I've done in my service would look like.
Smtih#1

[TriPSs]

Interesting, this looks pretty good but I think we should make that behavior then opt-in as I don't think this wil work when filtering relations.

[Smtih]

I'll start a PR for this then, I'm reworking my example to highlight what I think is wrong with issue 3 with a bit more of a realistic setup.

[Smtih]

@TriPSs Here's a more complete example to illustrate Isssue 3.
https://github.com/Smtih/netjs-query-auth-issues/blob/main/test/issue-3.e2e-spec.ts

Permission setup:
Groups - authed if my token says i belong to it.
https://github.com/Smtih/netjs-query-auth-issues/blob/82589a9e15bedac45e2daac4ee54e1b8635f3161/src/group/group.dto.ts#L11

Tasks - authed if my token says I belong to the group it's in and I'm assigned.
https://github.com/Smtih/netjs-query-auth-issues/blob/82589a9e15bedac45e2daac4ee54e1b8635f3161/src/task/task.dto.ts#L9

We can then expose filters that allow the FE to get something like Groups I have uncompleted tasks in.
Groups will implicitly be filtered to my groups thanks to the baked in auth filtering, however once we start asking questions about connected tasks in the nested filters we don't get the same behaviour, so we need to start adding explicit filters to ensure we're only filtering by my tasks.

The fact we get the auth filtering at the top level, but not at nested levels I think is a bit unintuitive, but more problematically, it also allows for sensitive information to leak.
I can now maliciously ask for all my groups where others have not completed their tasks, even though I have no access to other people's task. This is what I mean by leaking information in my original post.

[TriPSs]

I understand now, thanks for the explanation, I'm going to checkout your code to see why this is happening as normally (or at-least how I thought it was) the auth filter on the relation should also be applied.

Edit: after my first check I now completely understand the issue, the parent is still being returned because someone else (as described in the provided filter) still has the group, but if the auth filter would also be applied that would not have been the case. Currently the auth filters of the relation is only applied when you actually retrieve the tasks instead of filtering on it.

So to summarise: when we filter on groups and pass the relation tasks in that filter, the auth filter of tasks is not added to it and this is expected to be the case, right?

[Smtih]

Yeah, that's correct, resolving the related entities will trigger the auth filter, but not simply filtering by information on related entities and I think whether it's expected or not is questionable?

I would rather authorization be stricter by default with options to loosen it if the behaviour isn't desirable but for the sake of backwards compatibility, I think we'd need some sort of option which when enabled would allow the auth filter to be applied when we do the left joins on the relation to apply the filter.

If you think this is a sane behaviour for the library, I'll try and find time to raise another PR but probably need some pointers as to where this relation filtering/joining logic is happening in the library to get started.

[TriPSs]

I agree that it's sane behavior, will also try to find some time the coming days to look into this.

Do think we need to release it as breaking change, just so everyone is aware default behavior has been changed.

[Smtih]

I think it would need to be, as asking the question give me groups where any tasks are incomplete is a sane question to ask and people may rely on it even with a similar permission model.

I think other than a flag that restores the current behaviour globally we'd also want to consider passing information to the authorizer (e.g. through the Operation type/name?) that specifies where evaluating it for the purpose of relation filtering as opposed to querying?

[TriPSs]

I think other than a flag that restores the current behaviour globally we'd also want to consider passing information to the authorizer (e.g. through the Operation type/name?) that specifies where evaluating it for the purpose of relation filtering as opposed to querying?

Wanted to check it out tomorrow but my idea was to get it to work the same way as when you retrieve a relation, that also calls the authoriser with a different operation name.