feat(client): replace basic auth with OAuth ROPC flow by nejch · Pull Request #2422 · python-gitlab/python-gitlab

nejch · 2022-12-11T20:09:40Z

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 · 2022-12-11T20:20:36Z

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

nejch · 2022-12-19T15:22:06Z

gitlab/client.py

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


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 · 2022-12-19T15:24:36Z

@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.

lmilbaum · 2022-12-19T21:16:17Z

gitlab/client.py

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


What does this * mean?

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

@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 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.

lmilbaum · 2022-12-19T21:25:56Z

gitlab/client.py

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


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.

lmilbaum · 2022-12-19T21:32:58Z

gitlab/client.py

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


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 :)

nejch · 2023-01-23T12:56:19Z

gitlab/client.py

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


@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?

nejch · 2023-01-23T12:57:21Z

gitlab/client.py

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


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?

nejch · 2023-02-02T08:44:59Z

gitlab/client.py

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


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

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

codecov-commenter commented Dec 11, 2022 •

edited by codecov bot

Loading