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

Enable cloning of private PMR repositories #7

Merged
merged 3 commits into from
Aug 2, 2024

Conversation

tsalemink
Copy link
Contributor

Currently, using a CmdWorkspace to clone private PMR repositories is not well supported. This is because when we instantiate a CmdWorkspace object either init or clone will be automatically called on the Cmd instance used by the CmdWorkspace, but there is no way to pass user credentials to the CmdWorkspace constructor. So, for private repositories, either clone will fail due to lack of authentication, or init will create an empty .git directory - preventing subsequent clone commands from using that directory.

To address this issue a number of changes must be made:

  • username and password parameters should be added to the clone method for each of the Cmd sub-classes (DulwichDvcsCmd, GitDvcsCmd, MercurialDvcsCmd).
  • A NotGitRepository check should be added to the DulwichDvcsCmd read_remote method, since it may be called before the .git directory has been created. (The enclosing get_remote method already handles the fallback case where no remote is returned by read_remote)

Additionally, we need to prevent the CmdWorkspace from automatically calling the non-authenticated clone method on initialisation, since this will throw an authentication error for private repositories. There are a few alternatives I can think of for this:

  • Remove the automatic calls to init and clone entirely. In this case the user would need to call these methods explicitly when required after creating a CmdWorkspace. (This change will break most of the test cases)
  • Add additional prameters to the CmdWorkspace constructor to support passing user credentials.
  • Have the clone methods check for authentication errors so that we can warn the user and skip without crashing.

@metatoaster, @hsorby, let me know what you think about the last part. Maybe you can come up with a better solution.

I will update the test cases if required after the remaining changes have been made.

@metatoaster
Copy link
Member

It should never be a specific username/password based argument, but rather an Authorization token based upon the underlying HTTP request. This Authorization token can be in the form of Basic which is simply the base64 of the username:password string, or Bearer which could be created as part of OAuth 2.0 workflow when it's supported eventually. Rather than making this part of every method signature, it should be formed as part of the constructor of the affected class(es) so that this authorization header can be sent with every request that gets made from the relevant class.

Given the concern for backwards compatibility I might suggest having new subclasses entirely for authenticated access, and leave the current ones as unauthenticated so that new and better ways of doing this can be engineered rather than trying to extend on what I did here as a basic proof of concept type code for use cases that was never really anticipated to begin with (back before it was assumed that workflows are all public, for example).

@coveralls
Copy link

coveralls commented Mar 18, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 0bdeb31 on tsalemink:dev
into 2acfc22 on PMR2:master.

@tsalemink
Copy link
Contributor Author

I've just updated the authenticated CMD classes with a set_authorization method. This way we will also be able to pass in Bearer authentication instead of Basic once it is supported.

@tsalemink tsalemink marked this pull request as ready for review March 26, 2024 05:27
@tsalemink
Copy link
Contributor Author

@metatoaster, sorry for the delay. I've just added some test cases for the authenticated CMD sub-classes. These sub-classes are now tested against all of the relevant general test cases. To achieve this I have had to override a number of the test methods. I believe the test cases for the Git CMD are relatively self explanatory - mostly similar to the unauthenticated versions of these tests.

The dulwich test cases are slightly more complicated. To support basic authorization, we are having to use a urllib3 PoolManager object, as you know. Unfortunately the LocalGitClient class that is used for testing dulwich commands does not support this PoolManager argument as the other clients do. To address this I am monkey patching a wrapper around the LocalGitClient so that we can test this argument. I have also added a TrapDulwichCmd that captures and checks this internal pool manager instance.

@metatoaster
Copy link
Member

Please do let me know when you are fully happy with merging these changes in and when a release need to be cut for PyPI.

@tsalemink
Copy link
Contributor Author

Thanks. I'm pretty happy with it, feel free to merge and release when you have the time.

@tsalemink
Copy link
Contributor Author

@metatoaster, could you please merge these changes in and make a PyPI release if you have the time.

@metatoaster metatoaster merged commit d7bf5b9 into PMR2:master Aug 2, 2024
17 checks passed
@tsalemink
Copy link
Contributor Author

Thank you!

@metatoaster
Copy link
Member

@metatoaster
Copy link
Member

For future reference: I should have audited your pull request better, and that tests must be ensured to not actually make real request over the public Internet.

@metatoaster
Copy link
Member

Well, it was more hsorby's changes a while back, I am going to revert and change how these tests are done.

@metatoaster
Copy link
Member

Released on PyPI.

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.

3 participants