Enable token auth sync #326

fao89 · 2020-06-10T22:11:10Z

https://pulp.plan.io/issues/6540
closes #6540

pep8speaks · 2020-06-10T22:11:14Z

Hello @fao89! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-07-01 20:54:12 UTC

pulpbot · 2020-06-10T22:15:26Z

Attached issue: https://pulp.plan.io/issues/6540

fao89 · 2020-06-15T18:43:06Z

pulp_ansible/app/downloaders.py

+        # aiohttps does not allow to send auth argument and auth header together
+        self.session._default_auth = None
+
+        async with self.session.get(self.url, headers=headers, proxy=self.proxy) as response:


for stage env, I had to add ssl=False

Possible to add the full ca_chain for ci / stage will work as well.

fao89 · 2020-06-15T18:43:16Z

pulp_ansible/app/downloaders.py

+                "refresh_token": self.remote.token,
+            }
+            url = self.remote.auth_url
+            async with self.session.post(url, data=form_payload, raise_for_status=True) as response:


for stage env, I had to add ssl=False

fao89 · 2020-06-15T18:49:25Z

I had an issue here:
(building URL)
https://github.com/pulp/pulp_ansible/blob/master/pulp_ansible/app/tasks/collections.py#L340-L347

this endpoint https://ci.cloud.redhat.com/api/automation-hub/v3/collections/autohubtest2/collection_dep_a_hlrknkqw/ returns:

{
    "href": "/pulp_ansible/galaxy/automation-hub/api/v3/collections/autohubtest2/collection_dep_a_hlrknkqw/",
    "created_at": "2020-02-21T23:01:52.727205Z",
    "updated_at": "2020-02-21T23:01:52.727232Z",
    "namespace": "autohubtest2",
    "name": "collection_dep_a_hlrknkqw",
    "deprecated": false,
    "versions_url": "/pulp_ansible/galaxy/automation-hub/api/v3/collections/autohubtest2/collection_dep_a_hlrknkqw/versions/",
    "highest_version": {
        "href": "/pulp_ansible/galaxy/automation-hub/api/v3/collections/autohubtest2/collection_dep_a_hlrknkqw/versions/1.0.0/",
        "version": "1.0.0"
    }
}

so when getting versions_url it yields:
/pulp_ansible/galaxy/automation-hub/api/v3/collections/<namespace>/<name>/versions
https://ci.cloud.redhat.com/pulp_ansible/galaxy/automation-hub/api/v3/collections/autohubtest2/collection_dep_a_hlrknkqw/versions/

but the URL should be:
/api/automation-hub/v3/collections/<namespace>/<name>/versions
https://ci.cloud.redhat.com/api/automation-hub/v3/collections/autohubtest2/collection_dep_a_hlrknkqw/versions/

bmbouter · 2020-06-15T19:15:19Z

pulp_ansible/app/downloaders.py

+    """
+    Custom Downloader that automatically handles Token Based and Basic Authentication.
+
+    Additionally, use custom headers from DeclarativeArtifact.extra_data['headers']


Do these come from DeclarativeArtifact? I don't see them set anywhere.

I copied from pulp_container and forgot to remove it

bmbouter · 2020-06-15T19:19:02Z

Can some user facing docs be added? We already have a Collection section on syncing from galaxy.ansible.com, so I think this would be a "Syncing from cloud.redhat.com" as as an example.

@fao89 wdyt about adding this?

fao89 · 2020-06-15T19:50:34Z

Can some user facing docs be added? We already have a Collection section on syncing from galaxy.ansible.com, so I think this would be a "Syncing from cloud.redhat.com" as as an example.

@fao89 wdyt about adding this?

I agree, I'll add it

alikins

I think this may have issues against galaxy.ansible.com

alikins · 2020-06-23T13:47:59Z

pulp_ansible/app/models.py

@@ -227,6 +229,60 @@ class CollectionRemote(Remote):
    TYPE = "collection"

    requirements_file = models.TextField(null=True, max_length=255)
+    auth_url = models.CharField(null=True, max_length=255)


I was thinking this may need to be split info two Remote subclasses. One for the 'v2' and galaxy.ansible.com API, and another for Automation-hub / cloud.redhat.com / galaxy_ng.

Partly because "community" galaxy.ansible.com and cloud.redhat.com need different auth tokens.

cloud.redhat.com needs:

the 'auth_url' (pointing at https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token SSO server)

The value of "token" is a "refresh_token" in from SSO (the token a user gets from 'GET API TOKEN' on the cloud.redhat.com automation-hub ui).

The token received from SSO is used in 'Autorization: Bearer '

community / galaxy.ansible.com / maybe standalone galaxy_ng

No 'auth_url' is required. For ansible-galaxy client the presence of a auth_url is what determines if SSO style auth is used and what format the authorization token takes.

The value of "token" is a drf auth token

The token is used in an 'Authorization: Token ' header.

name wise, I was thinking more or less "CollectionRemote" for v2 / community galaxy.ansible.com remotes since it exists already.

And 'AutomationHubRemote' for Remotes for talking to automation-hub / cloud.redhat.com.

The differences in the technologies of the tokens (to me) motivates splitting up the Remote into two.

alikins · 2020-06-23T13:52:12Z

pulp_ansible/app/downloaders.py

+        # aiohttps does not allow to send auth argument and auth header together
+        self.session._default_auth = None
+
+        async with self.session.get(self.url, headers=headers, proxy=self.proxy) as response:


Possible to add the full ca_chain for ci / stage will work as well.

alikins · 2020-06-23T13:54:18Z

pulp_ansible/app/downloaders.py

+            return await super()._run(extra_data=extra_data)
+
+        token = await self.update_token()
+        headers = {"Authorization": "Bearer {token}".format(token=token)}


I think this will fail if the CollectionRemote is pointing at galaxy.ansible.com (and I believe, if pointed against a standalone galaxy_ng that is using drf style 'Authorization: Token ' style auth headers)

fao89 · 2020-06-24T01:06:30Z

I wasn't able to make the CI work, because it requires VPN to access https://ci.cloud.redhat.com/:

it was not about the token on travis CI

fao89 · 2020-06-29T19:43:40Z

pulp_ansible/app/downloaders.py

+        if not self.remote.auth_url:
+            headers = {"Authorization": self.remote.token}
+        else:
+            token = await self.update_token_from_auth_url()
+            headers = {"Authorization": "Bearer {token}".format(token=token)}


Without auth_url will be considered as community / galaxy.ansible.com / maybe standalone galaxy_ng
With auth_url will be considered as cloud.redhat.com

I guess this is simpler than splitting the remotes. The only reservation I have is if users will be confused about this. I guess we could make this clear in the serializer help text.

I'm just seeing this comment now, maybe we should leave it as one remote.

pulp_container does something similar:
https://github.com/pulp/pulp_container/blob/master/pulp_container/app/downloaders.py#L25

fao89 · 2020-06-30T22:20:05Z

pulp_ansible/tests/functional/cli/test_collection_install.py

+    @skip_if(bool, "AH_token", False)
+    def test_install_collection_with_token_from_automation_hub(self):


Tested locally and it is working! It will only run on pulp org, as travis does not set variables on PRs
@bmbouter @ironfroggy @chouseknecht

fao89 · 2020-06-30T22:21:58Z

pulp_ansible/tests/functional/cli/test_collection_install.py

+    @skip_if(bool, "GH_token", False)
+    def test_install_collection_with_token_from_galaxy(self):


It is more like a placeholder, as I don't know which collection requires token auth to be synced, I put pulp.pulp_installer here.
@bmbouter @ironfroggy @chouseknecht

bmbouter · 2020-07-01T16:23:52Z

CHANGES/6540.feature

@@ -0,0 +1 @@
+Enable token auth sync


Can this get more detail. As a user I would not understand this. Here are a few concrete suggestions:

Call out the new fields

Indicate which remote servers these fields can work with, e.g. galaxy.ansible.com and cloud.redhat.com.

Relate the new fields to the ansible-galaxy CLI documentation here https://docs.ansible.com/ansible/latest/user_guide/collections_using.html#configuring-the-ansible-galaxy-client

bmbouter · 2020-07-01T16:25:22Z

docs/workflows/collections.rst

@@ -136,6 +136,30 @@ Remote GET Response::
        "url": "https://galaxy-dev.ansible.com/api/v2/collections/testing/ansible_testing_content/",
    }

+For content sources token authentication. You can provide the ``token`` and/or ``auth_url``.


Suggested change

For content sources token authentication. You can provide the ``token`` and/or ``auth_url``.

For remote sources that require authentication, tokens can be used. You can provide the ``token`` and/or ``auth_url``.

^ still needs line wrapping

Also finding a way to https://docs.ansible.com/ansible/latest/user_guide/collections_using.html#configuring-the-ansible-galaxy-client here would be good too.

bmbouter · 2020-07-01T16:25:35Z

pulp_ansible/app/downloaders.py

+class TokenAuthHttpDownloader(HttpDownloader):
+    """
+    Custom Downloader that automatically handles Token Based and Basic Authentication.
+


extra line not needed

bmbouter · 2020-07-01T16:26:24Z

pulp_ansible/app/downloaders.py

+
+    """
+
+    token_lock = asyncio.Lock()


class attributes tend to be in all caps so /token_lock/TOKEN_LOCK/

bmbouter · 2020-07-01T16:28:12Z

pulp_ansible/app/downloaders.py

+        """
+        Initialize the downloader.
+        """
+        self.remote = kwargs.pop("remote")


This is definitly how the current ones implement this, but at a pulpcore we were hoping to have it be done differently in the future. This pattern would be replaced with a subclassed DownloadFactory and the kwargs would become positional args passed here instead. I wrote more about this change here: https://pulp.plan.io/issues/6965#note-2

bmbouter · 2020-07-01T16:30:10Z

pulp_ansible/app/downloaders.py

+            headers = {"Authorization": "Bearer {token}".format(token=token)}
+
+        # aiohttps does not allow to send auth argument and auth header together
+        self.session._default_auth = None


This could be better handled in the subclassed DownloadFactory also.

bmbouter · 2020-07-01T16:32:11Z

pulp_ansible/app/downloaders.py

+        async with self.session.get(self.url, headers=headers, proxy=self.proxy) as response:
+            response.raise_for_status()
+            to_return = await self._handle_response(response)
+            await response.release()
+
+        if self._close_session_on_finalize:
+            self.session.close()
+        return to_return


You could call super() to receive all of this instead.

I couldn't, pulpcore is different:

async with self.session.get(self.url, proxy=self.proxy, auth=self.auth) as response:

it uses auth and I use headers
https://github.com/pulp/pulpcore/blob/master/pulpcore/download/http.py#L197

bmbouter · 2020-07-01T16:33:45Z

pulp_ansible/app/models.py

+            )
+            return self._download_factory
+
+    def get_downloader(self, remote_artifact=None, url=None, **kwargs):


Using the suggestion to subclass DownloadFactory would eliminate this method also.

bmbouter · 2020-07-01T16:34:00Z

pulp_ansible/app/models.py

+        try:
+            return self._download_factory
+        except AttributeError:
+            self._download_factory = DownloaderFactory(


This is where you would use your subclassed DownloaderFactory instead

bmbouter · 2020-07-01T16:40:03Z

pulp_ansible/app/serializers.py

@@ -95,6 +95,21 @@ class CollectionRemoteSerializer(RemoteSerializer):
        required=False,
        allow_null=True,
    )
+    auth_url = serializers.CharField(
+        help_text=_(
+            "The URL of a Keycloak server ‘token_endpoint’ "


Suggested change

"The URL of a Keycloak server ‘token_endpoint’ "

"The URL to receive a session token from, e.g. used with Automation Hub."

Can we also add a sentence like: see https://docs.ansible.com/ansible/latest/user_guide/collections_using.html#configuring-the-ansible-galaxy-client for more details.

bmbouter · 2020-07-01T16:40:17Z

pulp_ansible/app/serializers.py

+        max_length=255,
+    )
+    token = serializers.CharField(
+        help_text=_("The token key to use for authentication."),


Can we also add a sentence like: See https://docs.ansible.com/ansible/latest/user_guide/collections_using.html#configuring-the-ansible-galaxy-client for more details.

bmbouter · 2020-07-01T16:41:59Z

pulp_ansible/tests/functional/cli/test_collection_install.py

+        monitor_task(sync_response.task)
+        repo = self.repo_api.read(repo.pulp_href)
+
+        # Create a distribution.


You could probably just check the unit counts at this point instead, but I'm ok w/ the rest of this test also even though it's duplicated in various places.

bmbouter · 2020-07-01T16:42:11Z

pulp_ansible/tests/functional/cli/test_collection_install.py

+        repo = self.repo_api.read(repo.pulp_href)
+
+        # Create a distribution.
+        body = gen_distribution()


You could probably just check the unit counts at this point instead, but I'm ok w/ the rest of this test also even though it's duplicated in various places.

https://pulp.plan.io/issues/6540 closes #6540

bmbouter

This is really excellent. You've implemented the suggested pattern perfectly. Thank you!

fao89 commented Jun 15, 2020

View reviewed changes

fao89 marked this pull request as ready for review June 15, 2020 18:49

fao89 requested a review from bmbouter June 15, 2020 18:52

bmbouter reviewed Jun 15, 2020

View reviewed changes

alikins reviewed Jun 23, 2020

View reviewed changes

fao89 requested a review from a team June 23, 2020 14:38

fao89 commented Jun 29, 2020

View reviewed changes

fao89 requested a review from bmbouter June 29, 2020 19:43

fao89 commented Jun 30, 2020

View reviewed changes

bmbouter reviewed Jul 1, 2020

View reviewed changes

Enable token auth sync

ae8d303

https://pulp.plan.io/issues/6540 closes #6540

fao89 requested a review from bmbouter July 2, 2020 17:17

bmbouter approved these changes Jul 2, 2020

View reviewed changes

bmbouter merged commit 41e4ef0 into pulp:master Jul 2, 2020

		@skip_if(bool, "AH_token", False)
		def test_install_collection_with_token_from_automation_hub(self):

		@skip_if(bool, "GH_token", False)
		def test_install_collection_with_token_from_galaxy(self):

	For content sources token authentication. You can provide the ``token`` and/or ``auth_url``.
	For remote sources that require authentication, tokens can be used. You can provide the ``token`` and/or ``auth_url``.

	"The URL of a Keycloak server ‘token_endpoint’ "
	"The URL to receive a session token from, e.g. used with Automation Hub."

Enable token auth sync #326

Enable token auth sync #326

Conversation

fao89 commented Jun 10, 2020

pep8speaks commented Jun 10, 2020 • edited

Comment last updated at 2020-07-01 20:54:12 UTC

pulpbot commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fao89 commented Jun 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter commented Jun 15, 2020

fao89 commented Jun 15, 2020

alikins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fao89 commented Jun 24, 2020

Choose a reason for hiding this comment

daviddavis Jun 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fao89 Jun 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fao89 Jul 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmbouter left a comment

Choose a reason for hiding this comment

pep8speaks commented Jun 10, 2020 •

edited

fao89 commented Jun 15, 2020 •

edited

daviddavis Jun 29, 2020 •

edited

fao89 Jun 30, 2020 •

edited

fao89 Jul 1, 2020 •

edited