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

Add content guard #976

Closed
wants to merge 7 commits into from
Closed

Conversation

bmclaughlin
Copy link
Contributor

@bmclaughlin bmclaughlin commented Sep 20, 2021

  • Add content guard to check authentication before a collection can be downloaded.
  • Create a migration to add content guard to existing Distributions
  • Create a signal to add content guard to new Distributions

Issue: AAH-923

AnsibleDistribution = apps.get_model('ansible', 'AnsibleDistribution')
ContentGuard = apps.get_model('galaxy', 'CollectionDownloadContentGuard')

cg = ContentGuard(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use get_or_create to initialize the object if it doesn't exist.


view = CollectionArtifactDownloadView()
setattr(view, "get_object", lambda: self)
setattr(view, "action", "download")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the action on this view is already set to "download"

from galaxy_ng.app.api.v3.viewsets import CollectionArtifactDownloadView

view = CollectionArtifactDownloadView()
setattr(view, "get_object", lambda: self)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overriding the get_object function so that it always returns the content guard instance. What we need to do is make it so that get_object returns the collection that is being accessed.

However, looking through the access policy, it looks like the download action doesn't take any conditions, so we can probably get away without defining a get_object method on the viewset until we get to a point where we need to add conditions to the download function.

        {
            "action": ["download"],
            "principal": 'authenticated',
            "effect": "allow",
        },

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, DRF access policy gets passed the view that is being accessed and can optionally use 'get_object' to load the object that is being accessed and check permissions on it. Here's one example where we do that: https://github.com/ansible/galaxy_ng/blob/master/galaxy_ng/app/access_control/access_policy.py#L75

@bmclaughlin bmclaughlin marked this pull request as ready for review September 21, 2021 17:17
@bmclaughlin
Copy link
Contributor Author

@chouseknecht, @rochacbruno updated migration and signal.

@rochacbruno
Copy link
Member

ci erroring on

django.core.exceptions.ImproperlyConfigured: Field name `content-guard` is not valid for model `AnsibleDistribution`.

Issue: AAH-923
…RK__DEFAULT_AUTHENTICATION_CLASSES

Issue: AAH-923
@bmclaughlin
Copy link
Contributor Author

/retest

@@ -11,11 +11,13 @@ def permit(self, request):
"""
Authorize the specified request based on if the request is authenticated.
"""
if not (drequest := request.get("drf_request", None)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Walrus := for the first time on our codebase :) 💯

@rochacbruno
Copy link
Member

@bmclaughlin can you provide a testing workflow for this PR? here in comments, something like

  1. Login to hub
  2. Try to click on x, y z.
  3. Verify that something happened

I want to help testing, but I checked out to this PR and I don't know what to check during my manual testing.

@bmclaughlin
Copy link
Contributor Author

@rochacbruno The manual testing that I've done for this PR started as:

  1. Make sure new migrations have been run
  2. Verify our core distros in core_distribution have the same content_guard_id (empty prior to this PR)
  3. Upload (or make api/create-test-collections), verify content_guard_id is populated again, same pulp_id

That is working as expected, I attempted to download an uploaded collection which now results in a 403 from the content-app. The content app is being re-enabled in #977, so I assume downloads should continue to work as they did prior to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants