-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
feat(auth): add OAuth2 login #276
Conversation
Currently GitHub login process in handled manually and Google is handled by But passport implementation now has security problem since it uses |
That's awesome, thanks! Do you think it is possible to create a generic OAuth2 configuration? If this would be possible, the admins could enter the parameters of their own identity provider, e.g., Authentik. I've been struggling with that too, but it seems like you know OAuth2 better than me. What do you think? |
General OAuth2 is impossible, but there is a standard OAuth2 authentication protocol called OpenID Connect (OIDC). Almost all these platform support OIDC as provider, including Authentik. So implementing OIDC protocol is enough for these authentication platoforms. |
Okay, sorry I've never implemented OAuth2 before so I might make some thinking mistakes. But this is the opportunity to understand it better :) If I understand it correctly every company implemented OAuth2 a bit differently so it is impossible to make this generic. OIDC is more standardized so this would allow a generic implementation? E.g if there is a new OIDC provider we don't have to update the code right? |
@stonith404 Yes, you are right :) A standard OAuth procession includes the following steps:
In summary, it's nearly impossible to implement a generic OAuth2 login. To solve the problems above, OIDC defines a configuration URL (example provided by Casdoor), with this URL we will know the necessary URLs during the processing of OAuth authentication (login, access token, user info etc.). Also, OIDC give us an id token (JWT) containing the user info, and it' filed name is fixed (example). Once we implement OIDC protocol, we'll have compatibility with all OIDC providers. |
@zz5840 Ohh I think I got it now. Thanks for explaining :) Would it be possible then to implement OIDC instead of OAuth2 in this case? GitHub and Google sign in can also be used with OIDC, right? Maybe you know Immich, they have the following configuration page for "OAuth": They call it "OAuth" but it's basically OIDC, or I'm wrong? |
@stonith404 Oh yes, according to the docs of Immich, they only support OIDC provider. Google does support both OAuth2 and OIDC, but GitHub does not support OIDC. By the way, OAuth2 is an authorization method, which means access token returned by OAuth is able to access many resources in user's account (for example user's file in Google Drive). But OIDC is just an authentication method, we will only get limited user info if using OIDC. Since google support OIDC, I will rewrite the google login with OIDC and add general OIDC support in next few days. |
Awesome. Sorry the GitHub issue was incorrectly formulated, I hope that you don't have to rewrite much :/ I think we could drop the support for signing in with Github as it would need a custom implementation. Or do you think we should still support it? I'm not sure if many admins actually want to use IDPs that are not self-hosted anyway. |
@stonith404 The OAuth login is not only for enterprise, but also for public users. If I deploy this project to my server and make it available to public, I hope users can login with their familiar social account rather than register a new one (that's why I named the config page "Social Login"), so it's necessary for us to adapt as many platforms as possible. As shown below, Gitea (a self-hosted GitHub alternative) provide a bunch of OAuth2 platforms to chosen from, and many of them do not support OIDC. |
@zz5840 Ah okay I see, that absolutely makes sense. Thanks! |
@stonith404 I'm not so familiar with Nest.js, is it possible to get user with without |
@zz5840 Yeah that's possible, do you just need the raw token? The JwtGuard validates the jwt token and sets the user context if valid. |
@stonith404 Sorry, I mean I need user (exactly user id), I know I can parse the raw JWT, but is there any convenient method? |
@zz5840 That's not possible as the user context gets set by the JWT claim Where exactly would you need this functionality? |
Linking account. I need to judge if user has logged in in callback route. If user has logged in, link their OAuth account to our account, and create a new account if not. Now, this function is implemented in two methods. pingvin-share/backend/src/oauth/oauth.controller.ts Lines 46 to 58 in c85eaad
pingvin-share/backend/src/oauth/oauth.controller.ts Lines 60 to 76 in c85eaad
|
Sorry could you elaborate? In the |
Oh, or do you mean if the user already has a Pingvin Share account and he wants to link his existing Pingvin Share account from the db with e.g GitHub? |
Yes, the following line is just used to login. pingvin-share/backend/src/oauth/oauth.service.ts Lines 61 to 63 in c85eaad
|
The current implementation separate login and linking into two routes. In login route, I check if provider user exists and return access token if true. In linking route, I check if provider user exists and store it to DB if false. So, the login route gets user by provider id and linking route gets user by JWT. Two callback URLs my cause some troubles when registering OAuth app in provider's console, so I want merge them into one route. |
Okay I see. You have to parse the jwt manually as far as I know. In the controller you can get the jwt with async getUserIdFromAccessToken(accessToken: string) {
try {
const { sub } = this.jwtService.verify(accessToken);
return sub;
} catch {
// not logged in
return null;
}
} |
Oh, I see. Thx :) |
@zz5840 No problem, just let me know if you need anything :) You can text me on Discord too (stonith404) if this is easier for you. But I take a break for today, I had an extremely long day. |
@stonith404 Ok, have a good rest. |
@stonith404 The error page is completed, I think it's ready for review now. So sorry I forgot this is a PR and rebased it to master, there's lots of unrelated commits now. |
@zz5840 Awesome, I'll look into it ASAP. |
@stonith404 Maybe the backend should only response with a i18n key rather than the explicit message. The error message should be written in local files. |
@zz5840 I'm completely with you. But the problem is that we don't know the external error messages. Don't we have to hard-code every error message for every OAuth provider if we want to store it in local files? Or how would you solve this? |
@stonith404 Now error message could be translated. pingvin-share/frontend/src/i18n/translations/en-US.ts Lines 485 to 495 in c19251b
|
According to the documentation of GitHub OAuth App (https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#error-codes-for-the-device-flow), most error code except |
@zz5840 Yeah that makes sense, thanks! I'll test it again tomorrow and then I'll merge it :) |
someone may use it (to revoke token or get other info etc.) also improved the i18n message
@zz5840 Sorry, I'm a bit busy. I'll make the final checks over the weekend and merge it afterwards. |
@stonith404 OK, I'll do some checking before then. 😃 |
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.
LGTM Thank you very much! Can I merge?
@stonith404 So does it need to be global? I'll delete it if not. pingvin-share/backend/src/main.ts Line 10 in 08ffa54
|
@zz5840 Oh yeah, this doesn't need to be global. Because we only return this error page if something went wrong with OAuth, right? For other backend errors we show toasts. |
@stonith404 OK, I think we can merge it. |
Oh I almost forgot to create a migration. I'll do it quickly |
I have created the migration, and I want to test it on my development server. Strangely, the build of the Docker image fails due to connectivity errors with npm. I will try again tomorrow. |
Thx, I didn't even know it's necessary to install Python when installing nanoid. 😖 |
This PR implement the OAuth2 login with GitHub, Google, Microsoft, Discord and OpenID Connect.
See #109.
Currently, this feature is still work in progress, it's not recommended to deploy it to production.
TODO
RevokeScreenshots
Click to view screenshots