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

Changing ViewContent to ViewOwnContent #12752

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

MikeAlhayek
Copy link
Member

Fix #12751

@jtkech we worked on the previous PR for GraphQL together. The permission change in this PR is correct since this will check a lower level permission that is implied by ViewContents. But the ShouldReturnBlogsWithViewBlogContentPermission is failing. For some reason the authenticated user "in the test" has no claims which explain why the test will fail. Any idea on the fix? The change in this PR fixes the issue on hand but breaks the test.

@jtkech
Copy link
Member

jtkech commented Nov 3, 2022

Yes about the user claims, maybe that's why we are using fot tests a PermissionsContext and a custom PermissionContextAuthorizationHandler and its HandleRequirementAsync(). But then yes implied permissions will not be taken into account.

So to make the failing test working I needed to add to the PermissionsContext the ViewOwnContent which normally is implied by ViewContent (see below).

I think that's okay if you tried it and if it works as expected in a real context.

            .WithPermissionsContext(new PermissionsContext
            {
                UsePermissionsContext = true,
                AuthorizedPermissions = new[]
                {
                    GraphQLApi.Permissions.ExecuteGraphQL,
                    Contents.CommonPermissions.ViewContent,
                    Contents.CommonPermissions.ViewOwnContent,
                }
            });

@jtkech
Copy link
Member

jtkech commented Nov 3, 2022

The other way is to complete the testing authorization handler, I copy pasted the private method GetGrantingNamesInternal() from the DefaultPermissionGrantingService and used it in the testing handler, and it was working without having to add the implied permissions in the PermissionsContext.

    protected override Task HandleRequirementAsync(AuthorizationHandlerContext context, PermissionRequirement requirement)
    {
        var permissions = (_permissionsContext.AuthorizedPermissions ?? Enumerable.Empty<Permission>()).ToList();

        if (!_permissionsContext.UsePermissionsContext)
        {
            context.Succeed(requirement);
        }
        else if (permissions.Contains(requirement.Permission))
        {
            context.Succeed(requirement);
        }
        else
        {
            // Used here to retrieve ImpliedBy permissions names.
            var grantingNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
            GetGrantingNamesInternal(requirement.Permission, grantingNames);
            if (permissions.Any(p => grantingNames.Contains(p.Name)))
            {
                context.Succeed(requirement);
            }
            else
            {
                context.Fail();
            }
        }

        return Task.CompletedTask;
    }

@agriffard agriffard changed the title Changnig ViewContent toViewOwnContent Changing ViewContent to ViewOwnContent Nov 3, 2022
@MikeAlhayek
Copy link
Member Author

@jtkech Thank you for that info. I fixed the HandleRequirementAsync and the tests are now green.

I also added another test for ViewOwnContent so both permission

@MikeAlhayek MikeAlhayek added this to the 1.5 milestone Nov 3, 2022
@sebastienros sebastienros merged commit b75873b into OrchardCMS:main Nov 3, 2022
@MikeAlhayek MikeAlhayek deleted the FixGraphQLPermission branch December 28, 2022 01:44
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.

GraphQL is not honoring ViewOwnContent for a given content type
3 participants