chore: add authentication type to GitlabAuthenticationError #1793
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1793 +/- ##
==========================================
+ Coverage 92.02% 92.04% +0.01%
==========================================
Files 76 76
Lines 4790 4801 +11
==========================================
+ Hits 4408 4419 +11
Misses 382 382
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JohnVillalovos
force-pushed
the
jlvillal/auth_token_type
branch
from
51573dc
to
f1239fd
Compare
Add the type of authentication used to the GitlabAuthenticationError exception. Hopefully this will make it easier to help user's debug authentication issues they run into.
JohnVillalovos
force-pushed
the
jlvillal/auth_token_type
branch
from
f1239fd
to
cce2202
Compare
There was a problem hiding this comment.
Thanks John! Just a few ideas from my side on this one :) At the beginning I wasn't quite sure about the use case for this, but for authentication errors might be handy with all the job token confusion.
Especially with the job token we could add a link to the endpoints available or something (e.g. have a mapping for the error message).
| def __init__( | ||
| self, | ||
| error_message: Union[str, bytes] = "", | ||
| response_code: Optional[int] = None, | ||
| response_body: Optional[bytes] = None, | ||
| auth_type: str = "", | ||
| ) -> None: | ||
| super().__init__( | ||
| error_message=error_message, | ||
| response_code=response_code, | ||
| response_body=response_body, | ||
| ) | ||
| self.auth_type = auth_type |
There was a problem hiding this comment.
Maybe we could add **kwargs to GitlabError and we don't need to reimplement it here? Just pop it from kwargs?
| @@ -722,6 +727,7 @@ def http_request( | |||
| response_code=result.status_code, | |||
| error_message=error_message, | |||
| response_body=result.content, | |||
| auth_type=self.auth_type, | |||
There was a problem hiding this comment.
I think we could actually reuse self.headers and pass it (or just the dict keys) to the exception to infer the auth type, without adding custom variables here.
http_username/http_password is basically dead code as HTTP Basic auth has been out of GitLab since version 10 so we don't need to worry about that (I actually have a local draft that performs password-based OAuth login from that). So we're left with Private-Token, Job-Token, and Authorization (OAuth bearer) from headers. We could probably just pass the keys from the headers dict to not leak stuff accidentally.
|
|
||
| def __str__(self) -> str: | ||
| if self.auth_type: | ||
| return f"{super().__str__()}: authentication_type: {self.auth_type}" |
There was a problem hiding this comment.
If we decide to infer the auth type automatically I'd maybe reformulate this a bit. Not sure exactly how, just authentication_type sounds like it's a variable name defined/passed somewhere. We can maybe check around how more verbose exceptions do it in cpython or some other libraries.
Add the type of authentication used to the GitlabAuthenticationError
exception. Hopefully this will make it easier to help user's debug
authentication issues they run into.