Adding permission check when querying content using GraphQL

[MikeAlhayek]

Fix #12693

Prior this PR, a user with permission to GraphQL can see all content regardless of their permissions. With the new added filter, we honor their permissions.

Also, GraphQLContentOptions.ConfigureContentType(...) had no effect. Like if the user configure contentType as hidden, we still show it anyway. This was also fixed.

[MikeAlhayek]

@jtkech are you able to help with the failing test case?

The problem is that the role "Anonymous" has permission to view all contents. So when the test ShouldNotReturnBlogsWithoutViewBlogContentPermission runs, it fails because it inherits all claims assigned to "Anonymous".

I think all these GraphQl tests should run with "Anonymous" and "Authenticated" that has NO "ViewContents" permissions assigned to them. Is there a better approach for this beside creating a recipe for GraphQL and maybe GraphQLContext?

[jtkech]

Okay, I think I found the issue, I will comment it.

[MikeAlhayek]

Okay, I think I found the issue, I will comment it.

@jtkech is the issue different than what I described above?

[jtkech]

So, I saw that when removing in ContentTypeQuery this new added line the test works.

query.RequirePermission(new Permission("ExecuteGraphQL"));

Then I looked at RequiresPermissionValidationRule.AuthorizeNodePermissionAsync() where it checks if there are more than one permissions, in that case it checks the following.

if (authorizationResults.Any(x => !true))

Which I think is wrong, so I replaced it by

if (authorizationResults.Any(x => !x))

Then the test was working because a validation error was added.

---

That said maybe you don't need to add the ExecuteGraphQL permission as it is checked in the GraphQLMiddleware.

[MikeAlhayek]

yeah that works but I still think in all the GraphQL tests we should remove ViewContents permission from the "Anonymous" and "Authenticated" roles. These tests should be adding their permission is each test. the only reason what you did works is because you enforce 2 permissions not just the 1 as we had.

[jtkech]

yeah that works

What did you do? Just removing query.RequirePermission(new Permission("ExecuteGraphQL")); or by applying the fix if (authorizationResults.Any(x => !x)).

These tests should be adding their permission is each test

In this test it adds explicitly the ExecuteGraphQL then it fails (not the test) because it doesn't have the permission, which is intended to make the test successful.

the only reason what you did works is because you enforce 2 permissions not just the 1 as we had.

I don't understand

[jtkech]

About ConfigureContentType(), yes I saw it, when you said in a free form I thought it could be provided e.g. by an API call, but it's a configuration by code only, so I changed my mind about the case sensitivity.

I think that's okay to keep it case sensitive, to force people to provide by code the exact type name, as it is automatically done when using the generic ConfigurePart<TContentPart>().

[jtkech]

the only reason what you did works is because you enforce 2 permissions not just the 1 as we had.

Just to say that I did nothing, at the end just applied the mentioned fix.

[jtkech]

Hmm, just saw that it doesn't check if permissions.Count() == 0 ;)

[MikeAlhayek]

@jtkech I changed that. but one of the tests is still failing. I also changed the string check to == instead

[jtkech]

Okay I'm going to look at it

[jtkech]

Okay looks like we need to reference the same static permission instance added in the test.

GraphQLApi.Permissions.ExecuteGraphQL,

So I could make it working by using the following in ContentTypeQuery

query.RequirePermission(Apis.GraphQL.Permissions.ExecuteGraphQL);

But needed to reference the OC.Apis.GraphQL module project, would need to move the Permissions to a core/abstractions project.

But it shows that our fix is okay as it still works when having more than one permission, then for now I suggest to not add the RequirePermission(Apis.GraphQL.Permissions.ExecuteGraphQL) as it is already checked in the GraphQLMiddleware line 62.

[MikeAlhayek]

@jtkech the tests are pass now. Let me know if you see something else before an approval.