feat(client): Use requests AuthBase classes

[uda]

Changes

Use Private / Job / OAuth token auth classes based on requests' AuthBase class to provide an encapsulated mechanism for passing authentication credentials.

Additionally this helps with #2425 since we always pass the auth argument, so unless hitting redirects requests should always prefer the passed token over netrc credentials.

This does however break the behavior when credentials are set using environment variables and a user wants to set netrc to override that (username / password should be discouraged, but users have better ideas...), for example, running in the CI the CI_JOB_TOKEN is set it will prevent the netrc creds from being loaded, instead of the old API logic (as can seen in the tests changes)

Documentation and testing

This changes internal implementation by leveraging standard requests mechanisms, if the tests cover this part, they should succeed or fail the same way they did before.

Update: I misunderstood the logic re. token precedence and assumed, after rewriting the tests I got the logic

[codecov]

Codecov Report

Merging #2595 (8131e6b) into main (e4a1f6e) will increase coverage by 8.37%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2595      +/-   ##
==========================================
+ Coverage   87.99%   96.36%   +8.37%     
==========================================
  Files          87       87              
  Lines        5656     5674      +18     
==========================================
+ Hits         4977     5468     +491     
+ Misses        679      206     -473     

Flag	Coverage Δ
api_func_v4	82.71% <64.51%> (?)
cli_func_v4	83.20% <80.64%> (?)
unit	88.03% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
gitlab/_backends/__init__.py	100.00% <100.00%> (ø)
gitlab/_backends/requests_backend.py	98.80% <100.00%> (+0.44%)	⬆️
gitlab/client.py	98.74% <100.00%> (+3.90%)	⬆️

... and 45 files with indirect coverage changes

[nejch]

I hate some of the changes I had to make, the tension between the mypy requirements, pylint and flake8 feel a bit clunky, can this be revisited?

If it helps get things done, sure!

We'll just have to make sure later we get the same behavior once we refactor if needed.

[nejch]

Thanks a lot for looking into this @uda!

I had another look and had a few ideas for the inheritance/typing issues (as well as a silly naming idea).

This does however break the behavior when credentials are set using environment variables and a user wants to set netrc to override that (username / password should be discouraged, but users have better ideas...), for example, running in the CI the CI_JOB_TOKEN is set it will prevent the netrc creds from being loaded, instead of the old API logic (as can seen in the tests changes)

Just to clarify, people cannot use netrc with passwords for the GitLab API for basic auth, as that's been disabled for years (see my other comment). They might be using .netrc with tokens as a way to have technology-agnostic auth stored locally (e.g. for API, git over https and packages). But I agree that's still plain-text on a machine. I've seen these used in CI for this purpose though, so maybe we can add a breaking change trailer for this once we're done, or document and test it.

[uda]

Let me know when you want to merge and I'll rebase, so I don't chase my tail rebasing 😄

[nejch]

Sorry for keeping this open for so long @uda. I can maybe add a final push here so you don't need to go through the back and forth, but I basically want to check the new netrc behavior and maybe test it. From what I can tell from requests code, when explicitly providing auth, netrc is completely ignored, so we can document this as a breaking change. We'll get this merged though :)

[nejch]

Sorry for keeping this open for so long @uda. I can maybe add a final push here so you don't need to go through the back and forth, but I basically want to check the new netrc behavior and maybe test it. From what I can tell from requests code, when explicitly providing auth, netrc is completely ignored, so we can document this as a breaking change. We'll get this merged though :)

Added some tests and docs, as the behavior has changed.