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

nejch

Draft

nejch

wants to merge 1 commit into

Conversation

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

Also gets rid of the requests-specific HTTPBasicAuth.

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

		)
		self.http_backend = http_backend(**kwargs)

		self._set_auth_info()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this * mean?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		self.http_username, self.http_password
		)

		if self.oauth_credentials:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		)
		self.http_backend = http_backend(**kwargs)

		self._set_auth_info()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

		)
		self.http_backend = http_backend(**kwargs)

		self._set_auth_info()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

		self.http_username, self.http_password
		)

		if self.oauth_credentials:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 marked this pull request as draft

October 12, 2023 12:03