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

Feat(authentication): username password provider #6052

Merged
merged 19 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@ export async function createAuthUsers(
userData: any[] = [
{
id: "test-id",
entity_id: "test-id",
provider: "manual",
},
{
id: "test-id-1",
entity_id: "test-id-1",
provider: "manual",
},
{
entity_id: "test-id-2",
provider: "store",
},
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ describe("AuthUser Service", () => {
{
id: "test",
provider_id: "manual",
entity_id: "test"
},
])

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ describe("AuthenticationModuleService - AuthUser", () => {
{
id: "test",
provider_id: "manual",
entity_id: "test"
},
])

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { initialize } from "../../../../src"
import { DB_URL } from "@medusajs/pricing/integration-tests/utils"
import { MedusaModule } from "@medusajs/modules-sdk"
import { IAuthenticationModuleService } from "@medusajs/types"
import { createAuthProviders } from "../../../__fixtures__/auth-provider"

jest.setTimeout(30000)

Expand All @@ -22,6 +23,10 @@ describe("AuthenticationModuleService - AuthProvider", () => {
schema: process.env.MEDUSA_PRICING_DB_SCHEMA,
},
})

if(service.__hooks?.onApplicationStart) {
await service.__hooks.onApplicationStart()
}
Comment on lines +27 to +29
Copy link
Member

Choose a reason for hiding this comment

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

todo: I would suggest to start using the medusa app to start modules. you can see how we achieved that in the search pr https://github.com/medusajs/medusa/blob/7de35d84c889070f34dd18255000bb39fba06a72/packages/medusa-test-utils/src/init-modules.ts

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not super happy about these service hooks. The consumer of a module service should never have to worry about the providers being created on load - even when using initialize.

However, this concern can be discussed and tackled separately from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed for modules that require something to be executed after all modules are loaded and the application is running.
It was originally created at the index/search module where it reads all the exported joiner configs of loaded modules.

This is not executed by the consumer of the modules. MedusaApp calls it after it has completed loading everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed a few times, at some point we need to normalize all entrypoints to be MedusaApp, and discourage the use of initialize directly

})

afterEach(async () => {
Expand All @@ -30,7 +35,7 @@ describe("AuthenticationModuleService - AuthProvider", () => {
})

describe("listAuthProviders", () => {
it("should list default AuthProviders", async () => {
it("should list default AuthProviders registered by loaders", async () => {
const authProviders = await service.listAuthProviders()
const serialized = JSON.parse(JSON.stringify(authProviders))

Expand All @@ -42,4 +47,22 @@ describe("AuthenticationModuleService - AuthProvider", () => {
])
})
})

describe("authenticate", () => {
it("authenticate validates that a provider is registered in container", async () => {
await createAuthProviders(testManager, [
{
provider: "notRegistered",
name: "test",
},
])

const { success, error } = await service.authenticate("notRegistered", {})

expect(success).toBe(false)
expect(error).toEqual(
"AuthenticationProvider with for provider: notRegistered wasn't registered in the module. Have you configured your options correctly?"
)
})
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import { SqlEntityManager } from "@mikro-orm/postgresql"
import Scrypt from "scrypt-kdf"

import { MikroOrmWrapper } from "../../../utils"
import { initialize } from "../../../../src"
import { DB_URL } from "@medusajs/pricing/integration-tests/utils"
import { MedusaModule } from "@medusajs/modules-sdk"
import { IAuthenticationModuleService } from "@medusajs/types"
import { createAuthUsers } from "../../../__fixtures__/auth-user"
import { createAuthProviders } from "../../../__fixtures__/auth-provider"

jest.setTimeout(30000)
const seedDefaultData = async (testManager) => {
await createAuthProviders(testManager)
await createAuthUsers(testManager)
}

describe("AuthenticationModuleService - AuthProvider", () => {
let service: IAuthenticationModuleService
let testManager: SqlEntityManager

beforeEach(async () => {
await MikroOrmWrapper.setupDatabase()
testManager = MikroOrmWrapper.forkManager()

service = await initialize({
database: {
clientUrl: DB_URL,
schema: process.env.MEDUSA_PRICING_DB_SCHEMA,
},
})

if(service.__hooks?.onApplicationStart) {
await service.__hooks.onApplicationStart()
}
})

afterEach(async () => {
await MikroOrmWrapper.clearDatabase()
MedusaModule.clearInstances()
})

describe("authenticate", () => {
it("authenticate validates that a provider is registered in container", async () => {
const password = "supersecret"
const email = "test@test.com"
const passwordHash = (
await Scrypt.kdf(password, { logN: 15, r: 8, p: 1 })
).toString("base64")

await seedDefaultData(testManager)
await createAuthUsers(testManager, [
// Add authenticated user
{
provider: "usernamePassword",
entity_id: email,
provider_metadata: {
password: passwordHash,
},
},
])

const res = await service.authenticate("usernamePassword", {
body: {
email: "test@test.com",
password: password,
},
})

expect(res).toEqual({
success: true,
authUser: expect.objectContaining({
entity_id: email,
provider_metadata: {
},
}),
})
})

it("fails when no password is given", async () => {
const email = "test@test.com"

await seedDefaultData(testManager)

const res = await service.authenticate("usernamePassword", {
body: { email: "test@test.com" },
})

expect(res).toEqual({
success: false,
error: "Password should be a string",
})
})

it("fails when no email is given", async () => {
await seedDefaultData(testManager)

const res = await service.authenticate("usernamePassword", {
body: { password: "supersecret" },
})

expect(res).toEqual({
success: false,
error: "Email should be a string",
})
})

it("fails with an invalid password", async () => {
const password = "supersecret"
const email = "test@test.com"
const passwordHash = (
await Scrypt.kdf(password, { logN: 15, r: 8, p: 1 })
).toString("base64")

await seedDefaultData(testManager)
await createAuthUsers(testManager, [
// Add authenticated user
{
provider: "usernamePassword",
entity_id: email,
provider_metadata: {
password_hash: passwordHash,
},
},
])

const res = await service.authenticate("usernamePassword", {
body: {
email: "test@test.com",
password: "password",
},
})

expect(res).toEqual({
success: false,
error: "Invalid email or password",
})
})
})
})
3 changes: 2 additions & 1 deletion packages/authentication/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"@mikro-orm/postgresql": "5.9.7",
"awilix": "^8.0.0",
"dotenv": "^16.1.4",
"knex": "2.4.2"
"knex": "2.4.2",
"scrypt-kdf": "^2.0.1"
}
}
5 changes: 3 additions & 2 deletions packages/authentication/src/initialize/index.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import {
ExternalModuleDeclaration,
InternalModuleDeclaration,
MedusaModule,
MODULE_PACKAGE_NAMES,
MedusaModule,
Modules,
} from "@medusajs/modules-sdk"
import { IAuthenticationModuleService, ModulesSdkTypes } from "@medusajs/types"
import { moduleDefinition } from "../module-definition"

import { InitializeModuleInjectableDependencies } from "../types"
import { moduleDefinition } from "../module-definition"

export const initialize = async (
options?:
Expand Down
44 changes: 16 additions & 28 deletions packages/authentication/src/loaders/providers.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { LoaderOptions, ModulesSdkTypes } from "@medusajs/types"
import { asClass } from "awilix"
import * as defaultProviders from "@providers"
import { AuthProviderService } from "@services"
import { ServiceTypes } from "@types"

import { LoaderOptions, ModulesSdkTypes } from "@medusajs/types"

import { AwilixContainer, ClassOrFunctionReturning, Resolver, asClass, asFunction, asValue } from "awilix"

export default async ({
container,
options,
}: LoaderOptions<
| ModulesSdkTypes.ModuleServiceInitializeOptions
| ModulesSdkTypes.ModuleServiceInitializeCustomDataLayerOptions
Expand All @@ -17,33 +16,22 @@ export default async ({

const providersToLoad = Object.values(defaultProviders)

const authProviderService: AuthProviderService =
container.cradle["authProviderService"]

const providers = await authProviderService.list({
provider: providersToLoad.map((p) => p.PROVIDER),
})

const loadedProviders = new Map(providers.map((p) => [p.provider, p]))

const providersToCreate: ServiceTypes.CreateAuthProviderDTO[] = []

for (const provider of providersToLoad) {
container.registerAdd("providers", asClass(provider).singleton())

container.register({
[`provider_${provider.PROVIDER}`]: asClass(provider).singleton(),
[`auth_provider_${provider.PROVIDER}`]: asClass(provider).singleton(),
})
}

if (loadedProviders.has(provider.PROVIDER)) {
continue
}
container.register({
[`auth_providers`]: asArray(providersToLoad),
})
}

providersToCreate.push({
provider: provider.PROVIDER,
name: provider.DISPLAY_NAME,
})
function asArray(
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Seems off to place a generic util like this here

resolvers: (ClassOrFunctionReturning<unknown> | Resolver<unknown>)[]
): { resolve: (container: AwilixContainer) => unknown[] } {
return {
resolve: (container: AwilixContainer) =>
resolvers.map((resolver) => container.build(resolver)),
}

await authProviderService.create(providersToCreate)
}
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,15 @@
"nullable": false,
"mappedType": "text"
},
"entity_id": {
"name": "entity_id",
"type": "text",
"unsigned": false,
"autoincrement": false,
"primary": false,
"nullable": false,
"mappedType": "text"
},
"provider_id": {
"name": "provider_id",
"type": "text",
Expand Down Expand Up @@ -117,6 +126,16 @@
"name": "auth_user",
"schema": "public",
"indexes": [
{
"keyName": "IDX_auth_user_provider_entity_id",
"columnNames": [
"provider_id",
"entity_id"
],
"composite": true,
"primary": false,
"unique": true
},
{
"keyName": "auth_user_pkey",
"columnNames": [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Migration } from '@mikro-orm/migrations';
Copy link
Contributor

Choose a reason for hiding this comment

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

comment: You should be able to omit the migration entirely until we have finalized the module. The tests will read the models and do not depend on migration files.


export class Migration20240115092929 extends Migration {

async up(): Promise<void> {
this.addSql('alter table "auth_user" add column "entity_id" text not null;');
this.addSql('alter table "auth_user" add constraint "IDX_auth_user_provider_entity_id" unique ("provider_id", "entity_id");');
}

async down(): Promise<void> {
this.addSql('alter table "auth_user" drop constraint "IDX_auth_user_provider_entity_id";');
this.addSql('alter table "auth_user" drop column "entity_id";');
}

}
9 changes: 8 additions & 1 deletion packages/authentication/src/models/auth-user.ts
Original file line number Diff line number Diff line change
@@ -1,25 +1,32 @@
import { generateEntityId } from "@medusajs/utils"
import {
BeforeCreate,
Cascade,
Entity,
Index,
ManyToOne,
OnInit,
OptionalProps,
PrimaryKey,
Property,
Unique,
} from "@mikro-orm/core"

import AuthProvider from "./auth-provider"
import { generateEntityId } from "@medusajs/utils"

type OptionalFields = "provider_metadata" | "app_metadata" | "user_metadata"

@Entity()
@Unique({ properties: ["provider","entity_id" ], name: "IDX_auth_user_provider_entity_id" })
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Does this need linting?

export default class AuthUser {
[OptionalProps]: OptionalFields

@PrimaryKey({ columnType: "text" })
id!: string

@Property({ columnType: "text" })
entity_id: string

@ManyToOne(() => AuthProvider, {
joinColumn: "provider",
fieldName: "provider_id",
Expand Down
Loading