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

Ignore Other Global Query Filters during OnModelCreating #34986

Closed
brandonryan opened this issue Oct 25, 2024 · 7 comments
Closed

Ignore Other Global Query Filters during OnModelCreating #34986

brandonryan opened this issue Oct 25, 2024 · 7 comments

Comments

@brandonryan
Copy link

brandonryan commented Oct 25, 2024

Context:

I have an OData application where the user can pass filters in the url query that get applied to the IQueryable. My initial attempt I just used an IQueryable with a filter applied from an internal class. The problem is as soon as the request uses $expand's, it .Include()'s data that is not filtered.

To solve this, I'm using .HasQueryFilter because it applies the .Where() no matter how the entity is accessed.
Example:

public class TestContext : DbContext
{
    //In my actual application this is populated per request based on user
    private List<int> AllowedPeopleIds = [1, 2, 3];

    public DbSet<Person> People { get; set; }
    public DbSet<Car> Cars { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        if (optionsBuilder.IsConfigured) return;

        optionsBuilder.LogTo(Console.WriteLine);
        optionsBuilder.EnableSensitiveDataLogging();
        optionsBuilder.UseSqlServer("data source=localhost;initial catalog=sandbox;TrustServerCertificate=True;integrated security=True;MultipleActiveResultSets=True;App=EntityFramework;");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Person>().HasQueryFilter(p =>
            AllowedPeopleIds.Contains(p.Id)
        );
        
        modelBuilder.Entity<Car>().HasQueryFilter(c =>
            //I know I could just use c.OwnerId but im going through a navigation to show the issue
            //In my real application im walking up to 3 parent relationships to get to the permissioned item
            AllowedPeopleIds.Contains(c.Owner.Id)
        );
    }
}

Very simple code for this test:

public static class Program
{
    public static async Task Main()
    {
        var db = new ExpContext();
        db.Cars.ToList();
    }
}

Here is the resulting SQL that is executed:

      SELECT [c].[Id], [c].[Color], [c].[Make], [c].[Model], [c].[PersonId]
      FROM [Cars] AS [c]
      LEFT JOIN (
          SELECT [p].[Id]
          FROM [People] AS [p]
          WHERE [p].[Id] IN (
              SELECT [e].[value]
              FROM OPENJSON(@__ef_filter__AllowedPeopleIds_0) WITH ([value] int '$') AS [e]
          )
      ) AS [t] ON [c].[PersonId] = [t].[Id]
      WHERE [t].[Id] IN (
          SELECT [e0].[value]
          FROM OPENJSON(@__ef_filter__AllowedPeopleIds_0) WITH ([value] int '$') AS [e0]
      )

Problem:

Because im walking through the Owner navigation of the car to check if their ID is in the list, the query condition get applied twice. Once for the Car QueryFilter, and once for the joined Person QueryFilter. For some use cases this probably makes sense, but for mine it just causes performance issues. This gets much worse as I have deeply related entities, and more complex permission scenarios.

Proposed Solution:

Maybe add a flag to disable EFCore applying the filters for .HasQueryFilter expressions. Either through a options parameter, or global configuration.
Something like:

modelBuilder.Entity<Car>().HasQueryFilter(c => AllowedPeopleIds.Contains(c.Owner.Id), new QueryFilterOptions { IgnoreEntityFilters = true });

If you feel that im misusing the QueryFilter let me know, I just couldn't find a better place to put a layer for the permission checking.

If this is out of scope of EFCore, then I can look into a solution a layer above EFCore. Pretty sure that requires me to make a custom IQueryProvider so I can tack on the permissions filters for includes there. I'm just trying to avoid such a large implementation lift if possible.

@brandonryan
Copy link
Author

brandonryan commented Oct 25, 2024

Forgot to post the models. Very basic though.

public class Car
{
    [Key]
    public required int Id { get; set; }
    public required string Make { get; set; }
    public required string Model { get; set; }
    public string? Color { get; set; }
    public int OwnerId { get; set; }

    // Navigations

    [ForeignKey(nameof(OwnerId))]
    public Person? Owner { get; set; }
}

public class Person
{
    [Key]
    public int Id { get; set; }
    public required string FirstName { get; set; }
    public required string LastName { get; set; }

    public List<Car>? Cars { get; set; }
}

@stevendarby
Copy link
Contributor

stevendarby commented Oct 25, 2024

Although this is a different use case, I think this is slightly related to #22914 - note in particular #22914 (comment). I think the request boils down to: the ability to only apply global query filters to the root set(s) in a query

@brandonryan
Copy link
Author

brandonryan commented Oct 25, 2024

Not exactly. I want global query filters to apply for includes. I just want it to not be applied for joins caused by navigation properties in the HasQueryFilter expressions in OnModelCreating. Or, perhaps thats what your saying and im misunderstanding?

Unlike your tenant id example, My actual model has entities that have children that are also individually permissioned, so I need the permission filter to be applied for includes.

@stevendarby
Copy link
Contributor

stevendarby commented Oct 25, 2024

Hmm, I could well be misunderstanding myself! But I'm thinking: Includes are joins. If you query on Cars and Include Owners, the filter on the root set Cars would already be filtering based on the related Owners being allowed, thus the join to Owner would only include the allowed Owners, without needing an explicit global filter on Owners. But you need the global filter on Owners because Owners might be the root set in another query.

However, I could be misunderstanding based on the limited example. Sorry if I've clouded the issue. Take it as an expression of support, I'd like to see query filters improved in lots of ways :D

@ajcvickers
Copy link
Member

Note for query folks: as @stevendarby says, it looks to me like this is covered by #22914, possibly with consideration for where the join comes from.

@brandonryan
Copy link
Author

brandonryan commented Oct 30, 2024

I'm good with that. I just don't want the complexity to be underestimated. If the solution ends up being "only apply global filter at the first level" then that wont work for me.

This is an example of one of my permission filters:

modelBuilder.Entity<TrainingStatus>().HasQueryFilter(ts =>
    (
        //access to view the vendor
        resolver.ViewAllVendors ||
        resolver.ViewVendorIds.Contains(ts.Employee.VendorId)
    ) || (
        (
            //has access to view the associated contract through the requirement
            resolver.ViewAllContracts ||
            resolver.ViewContractIds.Contains(ts.Requirement.ContractId)
        ) && (
            //has access to any of the line items (via td permission) on any of the employee's work assignments
            resolver.ViewAllTds ||
            ts.Employee.WorkAssignments.Any(wa =>
                wa.LineItem.TechnicalDirectionId == null ||
                resolver.ViewTdIds.Contains(wa.LineItem.TechnicalDirectionId.Value)
            )
        )
    )
);

Without adding query filters for the other entities a select looks like this:

SELECT [t].[EmployeeId], [t].[RequirementId], [t].[WorkAssignmentId], [t].[CompletionId], [t].[ExceptionId], CASE
          WHEN [t1].[Id] IS NOT NULL THEN 1
          WHEN [t2].[Id] IS NOT NULL THEN CASE
              WHEN [t2].[ExplicitRequire] = CAST(1 AS bit) THEN 0
              ELSE 3
          END
          ELSE 0
      END AS [CompletionStatus]
      FROM [torch].[TrainingStatusesView] AS [t]
      LEFT JOIN [torch].[Employees] AS [e] ON [t].[EmployeeId] = [e].[Id]
      LEFT JOIN [torch].[TrainingRequirements] AS [t0] ON [t].[RequirementId] = [t0].[Id]
      LEFT JOIN [torch].[TrainingCompletions] AS [t1] ON [t].[CompletionId] = [t1].[Id]
      LEFT JOIN [torch].[TrainingExceptions] AS [t2] ON [t].[ExceptionId] = [t2].[Id]
      WHERE @__ef_filter__ViewAllVendors_0 = CAST(1 AS bit) OR [e].[VendorId] IN (
          SELECT [e0].[value]
          FROM OPENJSON(@__ef_filter__ViewVendorIds_1) WITH ([value] int '$') AS [e0]
      ) OR ((@__ef_filter__ViewAllContracts_2 = CAST(1 AS bit) OR [t0].[ContractId] IN (
          SELECT [e1].[value]
          FROM OPENJSON(@__ef_filter__ViewContractIds_3) WITH ([value] int '$') AS [e1]
      )) AND (@__ef_filter__ViewAllTds_4 = CAST(1 AS bit) OR EXISTS (
          SELECT 1
          FROM [torch].[WorkAssignments] AS [w]
          LEFT JOIN [torch].[LineItems] AS [l] ON [w].[LineItemId] = [l].[Id]
          WHERE [e].[Id] IS NOT NULL AND [e].[Id] = [w].[EmployeeId] AND ([l].[TechnicalDirectionId] IS NULL OR [l].[TechnicalDirectionId] IN (
              SELECT [e2].[value]
              FROM OPENJSON(@__ef_filter__ViewTdIds_5) WITH ([value] int '$') AS [e2]
          )))))

Once I add the filters for all the other entities it looks like this:
Note: I'm still just .ToList() on the TrainingStatuses.

      SELECT [t].[EmployeeId], [t].[RequirementId], [t].[WorkAssignmentId], [t].[CompletionId], [t].[ExceptionId], CASE
          WHEN [t2].[Id] IS NOT NULL THEN 1
          WHEN [t4].[Id] IS NOT NULL THEN CASE
              WHEN [t4].[ExplicitRequire] = CAST(1 AS bit) THEN 0
              ELSE 3
          END
          ELSE 0
      END AS [CompletionStatus]
      FROM [torch].[TrainingStatusesView] AS [t]
      LEFT JOIN [torch].[Employees] AS [e] ON [t].[EmployeeId] = [e].[Id]
      LEFT JOIN (
          SELECT [t1].[Id], [t1].[ContractId]
          FROM [torch].[TrainingRequirements] AS [t1]
          WHERE @__ef_filter__ViewAllContracts_2 = CAST(1 AS bit) OR [t1].[ContractId] IN (
              SELECT [e0].[value]
              FROM OPENJSON(@__ef_filter__ViewContractIds_3) WITH ([value] int '$') AS [e0]
          )
      ) AS [t0] ON [t].[RequirementId] = [t0].[Id]
      LEFT JOIN (
          SELECT [t3].[Id]
          FROM [torch].[TrainingCompletions] AS [t3]
          LEFT JOIN [torch].[People] AS [p] ON [t3].[TraineeId] = [p].[Id]
          WHERE @__ef_filter__ViewAllVendors_0 = CAST(1 AS bit) OR EXISTS (
              SELECT 1
              FROM [torch].[Employees] AS [e4]
              WHERE [p].[Id] IS NOT NULL AND [p].[Id] = [e4].[PersonnelId] AND [e4].[VendorId] IN (
                  SELECT [e5].[value]
                  FROM OPENJSON(@__ef_filter__ViewVendorIds_1) WITH ([value] int '$') AS [e5]
              ))
      ) AS [t2] ON [t].[CompletionId] = [t2].[Id]
      LEFT JOIN (
          SELECT [t5].[Id], [t5].[ExplicitRequire]
          FROM [torch].[TrainingExceptions] AS [t5]
          LEFT JOIN (
              SELECT [t7].[Id], [t7].[ContractId]
              FROM [torch].[TrainingRequirements] AS [t7]
              WHERE @__ef_filter__ViewAllContracts_2 = CAST(1 AS bit) OR [t7].[ContractId] IN (
                  SELECT [e6].[value]
                  FROM OPENJSON(@__ef_filter__ViewContractIds_3) WITH ([value] int '$') AS [e6]
              )
          ) AS [t6] ON [t5].[RequirementId] = [t6].[Id]
          WHERE @__ef_filter__ViewAllContracts_2 = CAST(1 AS bit) OR [t6].[ContractId] IN (
              SELECT [e7].[value]
              FROM OPENJSON(@__ef_filter__ViewContractIds_3) WITH ([value] int '$') AS [e7]
          )
      ) AS [t4] ON [t].[ExceptionId] = [t4].[Id]
      WHERE @__ef_filter__ViewAllVendors_0 = CAST(1 AS bit) OR [e].[VendorId] IN (
          SELECT [e1].[value]
          FROM OPENJSON(@__ef_filter__ViewVendorIds_1) WITH ([value] int '$') AS [e1]
      ) OR ((@__ef_filter__ViewAllContracts_2 = CAST(1 AS bit) OR [t0].[ContractId] IN (
          SELECT [e2].[value]
          FROM OPENJSON(@__ef_filter__ViewContractIds_3) WITH ([value] int '$') AS [e2]
      )) AND (@__ef_filter__ViewAllTds_4 = CAST(1 AS bit) OR EXISTS (
          SELECT 1
          FROM [torch].[WorkAssignments] AS [w]
          LEFT JOIN [torch].[LineItems] AS [l] ON [w].[LineItemId] = [l].[Id]
          WHERE [e].[Id] IS NOT NULL AND [e].[Id] = [w].[EmployeeId] AND ([l].[TechnicalDirectionId] IS NULL OR [l].[TechnicalDirectionId] IN (
              SELECT [e3].[value]
              FROM OPENJSON(@__ef_filter__ViewTdIds_5) WITH ([value] int '$') AS [e3]
          )))))

Combine that with OData $expand and things get messier. I definitely think the filters can be rationalized / de-duplicated. That would actually be better that what im asking for.

I have two concerns specifically:

  • covering compound conditions (&& / || ) where portions can be deduplicated
  • covering .Any conditions

However, if this cant be done then adding a "ignore other global query filters" flag would be the next best solution for me. Thanks for the consideration.

@maumar
Copy link
Contributor

maumar commented Nov 1, 2024

closing as dupe of #22914

@maumar maumar closed this as not planned Won't fix, can't repro, duplicate, stale Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants