Paket icon indicating copy to clipboard operation
Paket copied to clipboard

Github authentication grammar is ambiguous - resulting in failure

Open laenas opened this issue 3 years ago • 1 comments

Description

It is not possible to reference an entire private github repository using the github source.

Repro steps

Please provide the steps required to reproduce the problem

  1. Have a private github repo
  2. github you/cantTouchThis hammerTime into paket.dependencies - where hammerTime is the credential key for a valid github username/PAT combination.
  3. dotnet paket install fails with a 404 from github.

Expected behavior

This should work the same as github you/areMySunshine - where areMySunshine is a publically visible repo.

Actual behavior

Github's API fails with a 404, since it attempts to check for a file that doesn't exist.

Known workarounds

Use the git source, referencing the full github URL and with the same username/PAT credentials added.

laenas avatar Oct 16 '21 16:10 laenas

The problem here can be traced to parseGitSource @ DependenciesFileParser.fs: 159 - where the grammar for the github can be seen to be ambiguous if you do not reference a specific file.

I'm filing this because it would, at least on my cursory investigation, appear to be a non-trivial fix - attempting to carry forward the ambiguity through the process to a point where it can be disambiguated seems tricky - as you can't be assured that a credential key existing is a surefire sign that a user did not, in fact, mean a file with the same token; and waiting until github's API rejects, and then retrying with an auth token, is clearly not elegant.

I'd propose a shift in grammar for authentication, and would be happy to help implement it: Following in the footsteps of HTTP, I'd propose: github (credentialKey@)ghUser/repo(:commits) And by doing so we should relatively neatly dodge the ambiguity and keep the change isolated to parseGitSource.

The downside here being is that it's obviously a large, breaking change. It would be possible to grandfather in the existing syntax, if only to receive a warning to update to the new syntax, as a placeholder, but would potentially cause a mismatch between what new users read in the documentation, and what they see in repos that have not updated.

laenas avatar Oct 16 '21 17:10 laenas