feat(client): replace basic auth with OAuth ROPC flow#2422

nejch

Small step towards #1195, and also to get rid of the old username/password auth.

Also gets rid of the requests-specific HTTPBasicAuth.

codecov-commenter

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.17%. Comparing base (45b8930) to head (be7745d).
Report is 148 commits behind head on main.

❗ Current head be7745d differs from pull request most recent head 3733872

Please upload reports for the commit 3733872 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2422      +/-   ##
==========================================
+ Coverage   92.16%   96.17%   +4.00%     
==========================================
  Files          88       88              
  Lines        5708     5692      -16     
==========================================
+ Hits         5261     5474     +213     
+ Misses        447      218     -229     

Flag Coverage Δ

api_func_v4	82.57% <81.81%> (?)
cli_func_v4	82.80% <51.51%> (-0.44%)	⬇️
unit	87.66% <100.00%> (-0.35%)	⬇️

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

Files Coverage Δ

gitlab/client.py	98.81% <100.00%> (+2.55%)	⬆️
gitlab/oauth.py	100.00% <100.00%> (ø)

... and 50 files with indirect coverage changes

We have to configure the backend first to be able to make requests in _set_auth_info() because that potentially makes a request to retrieve the OAuth token, so moving this down here.

Would it make sense to use an auth class for tracking the data?

Maybe, I'm not sure I fully get what you had in mind though :) but it might make this PR grow quite a bit, could you explain a bit what you had in mind and if it's more code we can maybe expand in a follow-up?

It seems like client is a monolithic class. Moving auth properties and functions to an auth class would be easier to maintain. IMO, implementing the auth class should come first before switching to a different method.

nejch

@lmilbaum this should also help get rid of requests.HTTPBasicAuth for the backend migration, as I just pass a generic tuple in here when used. Both httpx and requests accept tuples.

What does this * mean?

@lmilbaum

@lmilbaum It means that all arguments following are required to be keyword-only arguments.

@JohnVillalovos

@JohnVillalovos Thanks for the clarification. I wasn't aware of this Python feature. BTW, does it make sense to specify the oauth_credentials argument in the kwargs such that it doesn't affect the function signature?

@lmilbaum

@lmilbaum I think that would lose some of the explicitness. That's actually why I added the * here, so it doesn't affect the users as much even if the signature changes a bit. Are you worried about the typing in the backends code?

Personally, I don't like function signatures with a large amount of arguments. On the other hand, the explicitness argument is stronger.

What if a user provides both oauth_credentials and http_username and/or http_password?

If it's only one of them it will already fail earlier. But if it's both it will ignore them and use oauth. I can make this more explicit as well :)

IMO, this use case should be checked and an error should be thrown.

Would it make sense to use an auth class for tracking the data?

nejch

@lmilbaum I just realized I left my responses as Pending for weeks :( just clicking submit now :)

@lmilbaum

@lmilbaum I think that would lose some of the explicitness. That's actually why I added the * here, so it doesn't affect the users as much even if the signature changes a bit. Are you worried about the typing in the backends code?

Maybe, I'm not sure I fully get what you had in mind though :) but it might make this PR grow quite a bit, could you explain a bit what you had in mind and if it's more code we can maybe expand in a follow-up?

If it's only one of them it will already fail earlier. But if it's both it will ignore them and use oauth. I can make this more explicit as well :)

nejch force-pushed the feat/oauth2-resource-password-flow branch 2 times, most recently from 7b1a20d to 90be806 Compare December 19, 2022 15:02

nejch commented Dec 19, 2022

View reviewed changes

nejch requested a review from lmilbaum December 19, 2022 15:23

lmilbaum assigned nejch Dec 19, 2022

lmilbaum suggested changes Dec 19, 2022

View reviewed changes

nejch commented Feb 2, 2023

View reviewed changes

nejch force-pushed the feat/oauth2-resource-password-flow branch from 90be806 to be7745d Compare February 16, 2023 19:22

nejch force-pushed the feat/oauth2-resource-password-flow branch from be7745d to 7e65adc Compare October 12, 2023 11:16

feat(client): replace basic auth with OAuth ROPC flow

3733872

nejch marked this pull request as draft October 12, 2023 12:03

nejch force-pushed the feat/oauth2-resource-password-flow branch from 7e65adc to 3733872 Compare June 8, 2024 19:29

                       user_agent: str = gitlab.const.USER_AGENT,
                       retry_transient_errors: bool = False,
                       keep_base_url: bool = False,
+                      *,

                       user_agent: str = gitlab.const.USER_AGENT,
                       retry_transient_errors: bool = False,
                       keep_base_url: bool = False,
+                      *,

                       )
                       self.http_backend = http_backend(**kwargs)
+                      self._set_auth_info()

+                              self.http_username, self.http_password
+                          )
+                      if self.oauth_credentials:

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): replace basic auth with OAuth ROPC flow#2422

feat(client): replace basic auth with OAuth ROPC flow#2422nejch wants to merge 1 commit intomainpython-gitlab/python-gitlab:mainfrom feat/oauth2-resource-password-flowpython-gitlab/python-gitlab:feat/oauth2-resource-password-flowCopy head branch name to clipboard

Conversation

nejch commented Dec 11, 2022

Uh oh!

codecov-commenter commented Dec 11, 2022 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nejch commented Dec 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nejch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Footer

Footer navigation

feat(client): replace basic auth with OAuth ROPC flow#2422
nejch wants to merge 1 commit into
mainfrom
feat/oauth2-resource-password-flow

codecov-commenter commented Dec 11, 2022 •
edited by codecov Bot
Loading