-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(gitlab): added support for on-premise gitlab, first name and last name #145
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #145 +/- ##
==========================================
+ Coverage 94.48% 94.66% +0.18%
==========================================
Files 20 20
Lines 453 469 +16
==========================================
+ Hits 428 444 +16
Misses 25 25
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Could you please take another look? ❤️
fastapi_sso/sso/gitlab.py
Outdated
self, | ||
client_id: str, | ||
client_secret: str, | ||
base_endpoint_url: Optional[Union[pydantic.AnyHttpUrl, str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move the new argument to the end of the argument list? Also, I'm not super sure about the pydantic.AnyHttpUrl
, we are not using the url in any pydantic model, so this feel a little bit odd, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I haven't even noticed it's already there, then no problem, use it here as well 👌 Thanks!
fastapi_sso/sso/gitlab.py
Outdated
"authorization_endpoint": f"{self.base_endpoint_url}/oauth/authorize", | ||
"token_endpoint": f"{self.base_endpoint_url}/oauth/token", | ||
"userinfo_endpoint": f"{self.base_endpoint_url}/api/v4/user", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer using urljoin or r-stripping any potential trailing slash from the base_endpoint_url
before concatenating it (to remove the responsibility to know they should not add trailing slash from the users).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment, I will add urljoin
.
I will send this change in one push, after discussing about AnyHttp
A few important comments on the improvements.
And of course I tested it in a production environment on my Gitlab: {
"id": "2",
"email": "[email protected]",
"first_name": "Akim",
"last_name": "Jordobeck Jurmajohn",
"display_name": "akimrx",
"picture": "https://gitlab.xxxxx.net/uploads/-/system/user/avatar/2/avatar.png",
"provider": "gitlab"
} |
Hello!
The current Gitlab implementation does not support the use of authentication via on-premise Gitlab.
In order not to make patches on the client side, I'm making a small edit that adds only one parameter for init the provider with the default value.