Skip to content

Improve GraphQL content item authorization handling#19098

Merged
gvkries merged 5 commits intomainfrom
gvkries/fix-graphql-authorization
Apr 24, 2026
Merged

Improve GraphQL content item authorization handling#19098
gvkries merged 5 commits intomainfrom
gvkries/fix-graphql-authorization

Conversation

@gvkries
Copy link
Copy Markdown
Member

@gvkries gvkries commented Apr 1, 2026

Refactored ContentItemQuery to remove IHttpContextAccessor and use context.RequestServices for service resolution. Added user authorization checks before returning content items in GraphQL queries, ensuring only authorized users can access content. Passed context.User into GraphQL execution options for resolver access. Cleaned up constructor and removed unnecessary usings. Enhances security and modernizes dependency injection usage.

/cc @MikeAlhayek @Piedone

Refactored ContentItemQuery to remove IHttpContextAccessor and use context.RequestServices for service resolution. Added user authorization checks before returning content items in GraphQL queries, ensuring only authorized users can access content. Passed context.User into GraphQL execution options for resolver access. Cleaned up constructor and removed unnecessary usings. Enhances security and modernizes dependency injection usage.
@gvkries gvkries requested review from MikeAlhayek and Piedone April 1, 2026 09:45
gvkries added 2 commits April 2, 2026 10:26
Enforce ExecuteGraphQL permission on the contentItem GraphQL field and ensure null is returned if the content item does not exist. Add tests to verify access control for users with and without ViewContent permission.
@gvkries
Copy link
Copy Markdown
Member Author

gvkries commented Apr 2, 2026

This contains the same fix as #19102, but for v3 I slightly changed the implementation. There is no need to use the IHttpContextAccessor in GraphQL.

This PR also sets the User property correctly in all GraphQL contexts for use in future.

@Piedone
Copy link
Copy Markdown
Member

Piedone commented Apr 2, 2026

@MikeAlhayek please merge if OK.

@sebastienros
Copy link
Copy Markdown
Member

Let's merge it once 2.2 is merged back to main

@gvkries gvkries added this to the 3.0 milestone Apr 7, 2026
@tkononowiczsolwit tkononowiczsolwit mentioned this pull request Apr 22, 2026
30 tasks
@github-actions
Copy link
Copy Markdown
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@gvkries
Copy link
Copy Markdown
Member Author

gvkries commented Apr 24, 2026

@copilot resolve the merge conflicts in this pull request

…uthorization

# Conflicts:
#	src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/ContentItemQuery.cs
#	test/OrchardCore.Tests/Apis/GraphQL/ContentManagement/ContentItemQueryTests.cs

Co-authored-by: gvkries <7062785+gvkries@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 24, 2026

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts in commit 4390468.

The two conflicts were:

  1. ContentItemQuery.cs (content conflict): Kept our v3 approach using context.RequestServices and context.User without IHttpContextAccessor, which is the cleaner implementation for v3.
  2. ContentItemQueryTests.cs (add/add conflict): Both this PR and the 2.2 backport added this test file. Merged the proper using directives from main's version while keeping our test logic, and removed the duplicate class body.

@gvkries
Copy link
Copy Markdown
Member Author

gvkries commented Apr 24, 2026

@copilot Please fix the build error:

Error: /home/runner/work/OrchardCore/OrchardCore/test/OrchardCore.Tests/Apis/GraphQL/ContentManagement/ContentItemQueryTests.cs(71,68): error CS0104: 'ISession' is an ambiguous reference between 'Microsoft.AspNetCore.Http.ISession' and 'YesSql.ISession' [/home/runner/work/OrchardCore/OrchardCore/test/OrchardCore.Tests/OrchardCore.Tests.csproj::TargetFramework=net10.0]

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 24, 2026

@copilot Please fix the build error:

Error: /home/runner/work/OrchardCore/OrchardCore/test/OrchardCore.Tests/Apis/GraphQL/ContentManagement/ContentItemQueryTests.cs(71,68): error CS0104: 'ISession...

Fixed in commit 058ec65. Changed ISession to global::YesSql.ISession to disambiguate from Microsoft.AspNetCore.Http.ISession.

@gvkries gvkries merged commit 6793dcd into main Apr 24, 2026
6 checks passed
@gvkries gvkries deleted the gvkries/fix-graphql-authorization branch April 24, 2026 11:07
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

Successfully merging this pull request may close these issues.

4 participants