Skip to content
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

Refactor Auth social features and user credential creation #25

Merged
merged 8 commits into from
Sep 27, 2020
3 changes: 2 additions & 1 deletion auth/auth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ describe("#createUserWithEmailAndPassword()", () => {
expect(auth.store.add).toHaveBeenCalledWith({
email: "foo@bar.com",
password: "password",
providerId: firebase.auth.EmailAuthProvider.PROVIDER_ID,
});
});

Expand Down Expand Up @@ -54,7 +55,7 @@ describe("#onAuthStateChange()", () => {
expect(listener).toHaveBeenCalledWith(credential.user);
});

it("doesn't invokes the listener after it's disposed", async () => {
Copy link
Owner Author

Choose a reason for hiding this comment

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

typo

it("doesn't invoke the listener after it's disposed", async () => {
const auth = new MockAuth(app);
const listener = jest.fn();
const disposer = auth.onAuthStateChanged(listener);
Expand Down
77 changes: 25 additions & 52 deletions auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { SocialSignInMock } from "./social-signin-mock";
import { User } from "./user";
import { UserStore } from "./user-store";
import { AuthSettings } from "./auth-settings";
import { UserCredential } from "./user-credential";

export type AuthStateChangeListener = (user: firebase.User | null) => void;

Expand All @@ -13,7 +14,6 @@ export class MockAuth implements firebase.auth.Auth {
public settings: firebase.auth.AuthSettings = new AuthSettings();
public tenantId: string | null;
public readonly store = new UserStore();
private readonly socialSignIns = new Set<SocialSignInMock>();
private readonly authStateEvents = new Set<AuthStateChangeListener>();

constructor(public readonly app: MockApp) {}
Expand All @@ -38,8 +38,9 @@ export class MockAuth implements firebase.auth.Auth {
throw new Error("auth/email-already-in-use");
}

const user = this.store.add({ email, password });
return this.signIn(user);
const { providerId } = new firebase.auth.EmailAuthProvider();
const user = this.store.add({ email, password, providerId });
return this.signIn(user, { isNewUser: true });
}

fetchProvidersForEmail(email: string): Promise<any> {
Expand All @@ -60,7 +61,7 @@ export class MockAuth implements firebase.auth.Auth {

mockSocialSignIn(provider: firebase.auth.AuthProvider) {
const mock = new SocialSignInMock(provider.providerId);
this.socialSignIns.add(mock);
this.store.addSocialMock(mock);
return mock;
}

Expand Down Expand Up @@ -105,19 +106,25 @@ export class MockAuth implements firebase.auth.Auth {

private signIn(
user: User,
additionalUserInfo: firebase.auth.AdditionalUserInfo | null = null
{ isNewUser }: { isNewUser: boolean }
gustavohenke marked this conversation as resolved.
Show resolved Hide resolved
): Promise<firebase.auth.UserCredential> {
this.currentUser = user;
this.authStateEvents.forEach((listener) => {
listener(user);
});

return Promise.resolve<firebase.auth.UserCredential>({
user,
additionalUserInfo,
credential: null,
operationType: "signIn",
});
return Promise.resolve(new UserCredential(user, "signIn", isNewUser));
}

private async signInWithSocial(provider: firebase.auth.AuthProvider) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we use @firebase/auth-types to avoid having to use firebase.auth for every one of these types.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This package is not intended for direct usage, and should only be used via the officially supported firebase package.

😕 from https://www.npmjs.com/package/@firebase/auth-types/v/0.10.1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but that is simply because it is intended to be a private API for firebase, we are already very much working with internal APIs when mocking.

const data = await this.store.consumeSocialMock(provider);
let user = this.store.findByProviderAndEmail(data.email, provider.providerId);
if (user) {
return this.signIn(user, { isNewUser: false });
}

user = this.store.add({ ...data, providerId: provider.providerId });
return this.signIn(user, { isNewUser: true });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Little confused why it appears that we are removing the data from the store just to add it again

Copy link
Owner Author

Choose a reason for hiding this comment

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

Can you elaborate? I'm not following.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well if I am not mistaken consumeSocialMock() removes the provider from the store, but this.store.add() would add it back to the store.

Maybe consumeSocialMock() (and other functions) could use a JSDoc comment to elaborate on what it does to make this more clear?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added some comments around, let me know if it's clearer now!

}

signInAndRetrieveDataWithCredential(credential: firebase.auth.AuthCredential): Promise<any> {
Expand All @@ -126,11 +133,11 @@ export class MockAuth implements firebase.auth.Auth {

signInAnonymously(): Promise<firebase.auth.UserCredential> {
if (this.currentUser && this.currentUser.isAnonymous) {
return this.signIn(this.currentUser);
return this.signIn(this.currentUser, { isNewUser: false });
}

const user = this.store.add({ isAnonymous: true });
return this.signIn(user);
return this.signIn(user, { isNewUser: true });
}

signInWithCredential(credential: firebase.auth.AuthCredential): Promise<any> {
Expand All @@ -152,7 +159,7 @@ export class MockAuth implements firebase.auth.Auth {
return Promise.reject(new Error("auth/wrong-password"));
}

return this.signIn(user);
return this.signIn(user, { isNewUser: false });
}

signInWithEmailLink(
Expand All @@ -169,46 +176,12 @@ export class MockAuth implements firebase.auth.Auth {
throw new Error("Method not implemented.");
}

async signInWithPopup(
provider: firebase.auth.AuthProvider
): Promise<firebase.auth.UserCredential> {
const mock = Array.from(this.socialSignIns.values()).find(
(mock) => mock.type === provider.providerId
);

if (!mock) {
throw new Error("No mock response set.");
}

// Mock is used, then it must go
this.socialSignIns.delete(mock);

const data = await mock.response;
let user = this.store.findByEmail(data.email);
if (user) {
if (user.providerId !== provider.providerId) {
throw new Error("auth/account-exists-with-different-credential");
}

return this.signIn(user, {
isNewUser: false,
providerId: provider.providerId,
profile: null,
username: data.email,
});
}

user = this.store.add({ ...data, providerId: provider.providerId });
return this.signIn(user, {
isNewUser: true,
providerId: provider.providerId,
profile: null,
username: data.email,
});
signInWithPopup(provider: firebase.auth.AuthProvider): Promise<firebase.auth.UserCredential> {
return this.signInWithSocial(provider);
}

signInWithRedirect(provider: firebase.auth.AuthProvider): Promise<void> {
throw new Error("Method not implemented.");
async signInWithRedirect(provider: firebase.auth.AuthProvider): Promise<void> {
await this.signInWithSocial(provider);
gustavohenke marked this conversation as resolved.
Show resolved Hide resolved
}

signOut(): Promise<void> {
Expand Down
22 changes: 22 additions & 0 deletions auth/user-credential.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { User } from "./user";

export class UserCredential implements firebase.auth.UserCredential {
readonly additionalUserInfo: firebase.auth.AdditionalUserInfo | null = null;
readonly credential = null;

constructor(
readonly user: User,
readonly operationType: "signIn" | "link" | "reauthenticate",
isNewUser: boolean
) {
if (!user.isAnonymous) {
this.additionalUserInfo = {
isNewUser,
profile: null,
// providerId should be at the right value in the user object already.
providerId: user.providerId,
username: user.email,
};
}
}
}
60 changes: 51 additions & 9 deletions auth/user-store.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,69 @@
import { User, UserSchema } from "./user";
import { User, UserSchema, UserInfo } from "./user";
import { SocialSignInMock } from "./social-signin-mock";

export class UserStore {
private nextId = 0;
private idStore = new Map<string, UserSchema>();
private emailStore = new Map<string, UserSchema>();
private store = new Map<string, UserSchema>();
private readonly socialMocks: SocialSignInMock[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would make sense for this to be a Map or Set to simplify consumeSocialMock() with methods like .get(), .remove() or .has()?


add(data: Partial<UserSchema>): User {
const uid = ++this.nextId + "";
const user = new User({ ...data, uid }, this);
const user = new User(
{
...data,
providerData: data.providerId ? [new UserInfo(uid, data.providerId, data)] : [],
uid,
},
this
);

const schema = user.toJSON() as UserSchema;
this.idStore.set(schema.uid, schema);
schema.email && this.emailStore.set(schema.email, schema);
this.store.set(schema.uid, schema);
return user;
}

addSocialMock(mock: SocialSignInMock) {
this.socialMocks.push(mock);
}

consumeSocialMock(provider: firebase.auth.AuthProvider) {
const index = this.socialMocks.findIndex((mock) => mock.type === provider.providerId);
if (index === -1) {
throw new Error("No mock response set.");
}

// Mock is used, then it must go
const mock = this.socialMocks[index];
this.socialMocks.splice(index, 1);

return mock.response;
}

findByEmail(email: string): User | undefined {
const schema = this.emailStore.get(email);
return schema ? new User(schema, this) : undefined;
for (const user of this.store.values()) {
if (user.email === email) {
return new User(user, this);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it would be a good place for .find()

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm quite sure this made more sense when I wrote it 😅

Copy link
Owner Author

Choose a reason for hiding this comment

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

The reason why I wrote that that way is because .values() returns an iterator. To turn it into an array, one needs to spread it, which is an extra iteration if doing .find() on top of it.

Even though that's in theory slower, I went it to make that method shorter 👍


return undefined;
}

findByProviderAndEmail(email: string, providerId: string): User | undefined {
const user = this.findByEmail(email);
if (!user) {
return undefined;
}

if (user.providerData.some((info) => info.providerId === providerId)) {
return new User({ ...user.toJSON(), providerId }, this);
}

throw new Error("auth/account-exists-with-different-credential");
}

update(id: string, data: Partial<UserSchema>) {
const schema = this.idStore.get(id);
const schema = this.store.get(id);
if (!schema) {
return;
}
Expand Down
24 changes: 21 additions & 3 deletions auth/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export interface UserSchema {
metadata: firebase.auth.UserMetadata;
password?: string;
phoneNumber: string | null;
providerData: (firebase.UserInfo | null)[];
providerData: UserInfo[];
refreshToken: string;
displayName: string | null;
email: string | null;
Expand All @@ -17,7 +17,26 @@ export interface UserSchema {
tenantId: string | null;
}

export class UserInfo implements firebase.UserInfo {
readonly displayName: string | null;
readonly email: string | null;
readonly phoneNumber: string | null;
readonly photoURL: string | null;

constructor(
readonly uid: string,
readonly providerId: string,
rest: Partial<Omit<firebase.UserInfo, "uid" | "providerId">>
) {
this.displayName = rest.displayName || null;
this.email = rest.email || null;
this.phoneNumber = rest.phoneNumber || null;
this.photoURL = rest.photoURL || null;
gustavohenke marked this conversation as resolved.
Show resolved Hide resolved
}
}

export class User implements firebase.User, UserSchema {
readonly uid: string;
displayName: string | null;
email: string | null;
emailVerified: boolean;
Expand All @@ -26,11 +45,10 @@ export class User implements firebase.User, UserSchema {
password?: string;
phoneNumber: string | null;
photoURL: string | null;
providerData: (firebase.UserInfo | null)[];
Copy link
Owner Author

Choose a reason for hiding this comment

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

this was probably inherited from Firebase v4 or something.

providerData: UserInfo[];
providerId: string;
refreshToken: string;
tenantId: string | null;
uid: string;
multiFactor: firebase.User.MultiFactorUser;

constructor(data: Partial<UserSchema>, private readonly store: UserStore) {
Expand Down