Handle users' permissions when pulling data via pull-through distributions

[lubosmj]

closes #1657
closes #1623
closes #1624

[ipanova]

can you add that anonymous users could still pull 'cached' content?

[ipanova]

I did not read through the code, but how this scenario will be handled:
Anonymos user pulls content (from existing repo) but there is an updated content remotely? We should not be allowing this, because as a result of such operation a new repo version will be created and more bits will be pulled down into pulp

[lubosmj]

Anonymos user pulls content (from existing repo) but there is an updated content remotely?

Good catch! I may want to introduce another check in

pulp_container/pulp_container/app/registry_api.py

Line 1031 in da8a70f

if distribution.remote and distribution.pull_through_distribution_id:

and

pulp_container/pulp_container/app/registry.py

Line 132 in da8a70f

if distribution.remote_id and distribution.pull_through_distribution_id:

. Is there a guarantee that the same headers are passed during the redirect? With BasicAuth, we would want to be sure that the user headers remain the same inside content-app to prevent pulling.

can you add that anonymous users could still pull 'cached' content?

Is "new content" not explicit enough?

[lubosmj]

I am still trying to figure out how to restrict the access to content-app for anonymous users. Checking the basic auth header is not enough.

[lubosmj]

Actually, we do not need to restrict the access to content-app. Similarly, we do not pass a token to content-app and check the access scope. Everything is tested inside api-app. So, I am making this PR ready for review.

[mdellweg]

I do not see anything out of the odinary.

Only looked at: "Enforce permission checks while pulling from pull-through distributions"

[mdellweg]

So this never triggered?

[lubosmj]

Correct. And we do not have such a model.

[mdellweg]

This (and a similar one below) is the core of the change, right?

[lubosmj]

Yes, once we pass through the registry token steps and a user can access the particular endpoint, we should finally ensure that users without valid permissions cannot pull new content into a repository. But, they can still pull existing content.

[lubosmj]

This granularity cannot be achieved purely with token authentication. That is why I had to introduce this check.

[git-hyagi]

When I was testing this change, I was receiving errors like: "Repository not found", but the remote repository was available. Then, I realized the error was because of the lack of authentication.
What if we return 401 for
https://github.com/pulp/pulp_container/pull/1659/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR315 (not self.request.user.is_authenticated)
https://github.com/pulp/pulp_container/pull/1659/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR1046 (not permission_checker.has_pull_through_permissions(distribution))
https://github.com/pulp/pulp_container/pull/1659/files#diff-1b944e15118f82233aa03a435e9e05c188ad0d371b71be43260506b4a5eeddffR1101 (not permission_checker.has_pull_through_permissions(distribution))
?

for example:

    if (
        distribution.remote
        and distribution.pull_through_distribution_id
        and request.user.is_authenticated
    ):
        <logic block>
    elif not request.user.is_authenticated:
        raise UnauthorizedException()
    else:
        raise ManifestNotFound(reference=pk)

Trying to pull from a private pull-through distribution, it returned:

requested access to the resource is denied

instead of "repository not found".

[lubosmj]

The idea is to not change the default behaviour. Once there is no such a repository existing in Pulp, we should return 404 in case an unauthorized user is trying to pull it. If the repository exists, and it is private, we should return 401 when the unauthorized user is accessing it. This whole workflow should be transparent to pull-through caching. I think more granular error handling is not needed in this case.