-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
🪟 🎉 Enable OAuth login #15414
🪟 🎉 Enable OAuth login #15414
Changes from 4 commits
e0d6c42
368a35d
2ad7b8e
e8676df
996cec0
7ba2a85
14cd79e
be7177a
3ba46ad
c2754d1
734fc2d
50eea48
84ff5a3
c480fdd
8d080aa
9310e92
6fd5a83
e21e0e1
23cef5b
36bbf80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
export enum AuthProviders { | ||
GoogleIdentityPlatform = "google_identity_platform", | ||
} | ||
|
||
export type OAuthProviders = "github" | "google"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,13 @@ import { User as FbUser } from "firebase/auth"; | |
import React, { useCallback, useContext, useMemo, useRef } from "react"; | ||
import { useQueryClient } from "react-query"; | ||
import { useEffectOnce } from "react-use"; | ||
import { Observable, Subject } from "rxjs"; | ||
|
||
import { Action, Namespace } from "core/analytics"; | ||
import { isCommonRequestError } from "core/request/CommonRequestError"; | ||
import { useAnalyticsService } from "hooks/services/Analytics"; | ||
import useTypesafeReducer from "hooks/useTypesafeReducer"; | ||
import { AuthProviders } from "packages/cloud/lib/auth/AuthProviders"; | ||
import { AuthProviders, OAuthProviders } from "packages/cloud/lib/auth/AuthProviders"; | ||
import { GoogleAuthService } from "packages/cloud/lib/auth/GoogleAuthService"; | ||
import { User } from "packages/cloud/lib/domain/users"; | ||
import { useGetUserService } from "packages/cloud/services/users/UserService"; | ||
|
@@ -38,13 +40,16 @@ export type AuthSendEmailVerification = () => Promise<void>; | |
export type AuthVerifyEmail = (code: string) => Promise<void>; | ||
export type AuthLogout = () => Promise<void>; | ||
|
||
type OAuthLoginState = "waiting" | "loading" | "done"; | ||
|
||
interface AuthContextApi { | ||
user: User | null; | ||
inited: boolean; | ||
emailVerified: boolean; | ||
isLoading: boolean; | ||
loggedOut: boolean; | ||
login: AuthLogin; | ||
loginWithOAuth: (provider: OAuthProviders) => Observable<OAuthLoginState>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ This method returns an Obersvable, since we need to know in the UI, when the middle of the method starts (the user selected an account, and we start loading the user), to show the loading spinner. Thus we can't express that with a Promise. |
||
signUpWithEmailLink: (form: { name: string; email: string; password: string; news: boolean }) => Promise<void>; | ||
signUp: AuthSignUp; | ||
updatePassword: AuthUpdatePassword; | ||
|
@@ -68,10 +73,57 @@ export const AuthenticationProvider: React.FC = ({ children }) => { | |
const analytics = useAnalyticsService(); | ||
const authService = useInitService(() => new GoogleAuthService(() => auth), [auth]); | ||
|
||
/** | ||
* Create a user object in the Airbyte database from an existing Firebase user. | ||
* This will make sure that the user account is tracked in our database as well | ||
* as create a workspace for that user. This method also takes care of sending | ||
* the relevant user creation analytics events. | ||
*/ | ||
const createDatabaseUser = async ( | ||
timroes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fbUser: FbUser, | ||
timroes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
userData: { name?: string; companyName?: string; news?: boolean } = {} | ||
): Promise<User> => { | ||
// Create the Airbyte user on our server | ||
const user = await userService.create({ | ||
authProvider: AuthProviders.GoogleIdentityPlatform, | ||
authUserId: fbUser.uid, | ||
email: fbUser.email ?? "", | ||
name: userData.name ?? fbUser.displayName ?? "", | ||
companyName: userData.companyName ?? "", | ||
news: userData.news ?? false, | ||
}); | ||
|
||
analytics.track(Namespace.USER, Action.CREATE, { | ||
actionDescription: "New user registered", | ||
user_id: fbUser.uid, | ||
name: user.name, | ||
email: user.email, | ||
// Which login provider was used, e.g. "password", "google.com", "github.com" | ||
provider: fbUser.providerData[0]?.providerId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ There is a chance a firebase user has multiple providers linked (e.g. mail and google as mentioned in the PR description). But that shouldn't be the case when the user is just created, in which case they should only have one provider linked, thus we can simply take the first provider here for sending the event. |
||
...getUtmFromStorage(), | ||
}); | ||
|
||
return user; | ||
}; | ||
|
||
const onAfterAuth = useCallback( | ||
async (currentUser: FbUser, user?: User) => { | ||
user ??= await userService.getByAuthId(currentUser.uid, AuthProviders.GoogleIdentityPlatform); | ||
loggedIn({ user, emailVerified: currentUser.emailVerified }); | ||
try { | ||
user ??= await userService.getByAuthId(currentUser.uid, AuthProviders.GoogleIdentityPlatform); | ||
loggedIn({ user, emailVerified: currentUser.emailVerified }); | ||
} catch (e) { | ||
if (isCommonRequestError(e) && e.status === 404) { | ||
// If there is a firebase user but not database user we'll create a db user in this step | ||
// and retry the onAfterAuth step. This will always happen when a user logins via OAuth | ||
// the first time and we don't have a database user yet for them. In rare cases this can | ||
// also happen for email/password users if they closed their browser or got some network | ||
// errors in between creating the firebase user and the database user originally. | ||
const user = await createDatabaseUser(currentUser); | ||
await onAfterAuth(currentUser, user); | ||
} else { | ||
throw e; | ||
} | ||
} | ||
}, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[userService] | ||
|
@@ -108,6 +160,26 @@ export const AuthenticationProvider: React.FC = ({ children }) => { | |
await onAfterAuth(auth.currentUser); | ||
} | ||
}, | ||
loginWithOAuth(provider): Observable<OAuthLoginState> { | ||
const state = new Subject<OAuthLoginState>(); | ||
try { | ||
state.next("waiting"); | ||
authService | ||
.loginWithOAuth(provider) | ||
.then(async () => { | ||
state.next("loading"); | ||
if (auth.currentUser) { | ||
await onAfterAuth(auth.currentUser); | ||
state.next("done"); | ||
state.complete(); | ||
} | ||
}) | ||
.catch((e) => state.error(e)); | ||
} catch (e) { | ||
state.error(e); | ||
} | ||
return state.asObservable(); | ||
}, | ||
async logout(): Promise<void> { | ||
await userService.revokeUserSession(); | ||
await authService.signOut(); | ||
|
@@ -167,27 +239,12 @@ export const AuthenticationProvider: React.FC = ({ children }) => { | |
// Create a user account in firebase | ||
const { user: fbUser } = await authService.signUp(form.email, form.password); | ||
|
||
// Create the Airbyte user on our server | ||
const user = await userService.create({ | ||
authProvider: AuthProviders.GoogleIdentityPlatform, | ||
authUserId: fbUser.uid, | ||
email: form.email, | ||
name: form.name, | ||
companyName: form.companyName, | ||
news: form.news, | ||
}); | ||
// Create a user in our database for that firebase user | ||
await createDatabaseUser(fbUser, { name: form.name, companyName: form.companyName, news: form.news }); | ||
timroes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Send verification mail via firebase | ||
await authService.sendEmailVerifiedLink(); | ||
|
||
analytics.track(Namespace.USER, Action.CREATE, { | ||
actionDescription: "New user registered", | ||
user_id: fbUser.uid, | ||
name: user.name, | ||
email: user.email, | ||
...getUtmFromStorage(), | ||
}); | ||
|
||
if (auth.currentUser) { | ||
await onAfterAuth(auth.currentUser); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
@use "../../../../../scss/variables" as vars; | ||
@use "../../../../../scss/colors"; | ||
|
||
.separator { | ||
display: flex; | ||
margin: vars.$spacing-xl 0; | ||
font-size: 16px; | ||
text-align: center; | ||
font-weight: bold; | ||
color: colors.$grey-800; | ||
text-transform: uppercase; | ||
|
||
&:before, | ||
&:after { | ||
content: ""; | ||
flex: 1 1; | ||
border-bottom: 1px solid colors.$grey-300; | ||
margin: auto; | ||
} | ||
|
||
&:before { | ||
margin-right: vars.$spacing-md; | ||
} | ||
|
||
&:after { | ||
margin-left: vars.$spacing-md; | ||
} | ||
} | ||
|
||
.buttons { | ||
display: grid; | ||
grid-template-columns: 1fr; | ||
gap: vars.$spacing-sm; | ||
} | ||
|
||
.spinner { | ||
text-align: center; | ||
} | ||
|
||
.google, | ||
.github { | ||
justify-content: center; | ||
display: flex; | ||
align-items: center; | ||
font-size: 16px; | ||
font-weight: 500; | ||
width: 100%; | ||
margin: vars.$spacing-sm auto; | ||
padding: vars.$spacing-md; | ||
gap: vars.$spacing-md; | ||
border-radius: vars.$border-radius-sm; | ||
border: none; | ||
transition: all vars.$transition; | ||
cursor: pointer; | ||
|
||
&:hover, | ||
&:focus { | ||
box-shadow: 0 1px 3px rgba(53, 53, 66, 0.2), 0 1px 2px rgba(53, 53, 66, 0.12), 0 1px 1px rgba(53, 53, 66, 0.14); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where did these drop shadow colors come from? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shadow is from the regular button components. Most likely better to exctract it into a SASS variable. Any preference for which file? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @edmundito Any preferences for a file we should pack this into? |
||
} | ||
|
||
& > img { | ||
height: 24px; | ||
} | ||
} | ||
|
||
.google { | ||
background: colors.$white; | ||
border: 1px solid colors.$grey-100; | ||
} | ||
|
||
.github { | ||
background: #333; | ||
color: colors.$white; | ||
} | ||
|
||
.error { | ||
margin-top: vars.$spacing-lg; | ||
color: colors.$red; | ||
} |
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.
ℹ️ We never exported this type, and implementing the interface in TypeScript does not even help not needing to respecify the types in the
GoogleAuthService
anyway. Thus this interface is not really fulfilling any purpose (besides needing to arbitrarily type the method signature twice) and I removed it.