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

Conversation

pKorsholm
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Jan 10, 2024

⚠️ No Changeset found

Latest commit: 228c317

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2024 9:11am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Visit Preview Jan 23, 2024 9:11am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Jan 23, 2024 9:11am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Jan 23, 2024 9:11am

Comment on lines +27 to +29
if(service.__hooks?.onApplicationStart) {
await service.__hooks.onApplicationStart()
}
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

packages/authentication/package.json Outdated Show resolved Hide resolved
packages/authentication/package.json Outdated Show resolved Hide resolved
packages/authentication/src/providers/username-password.ts Outdated Show resolved Hide resolved
packages/authentication/src/providers/username-password.ts Outdated Show resolved Hide resolved
packages/authentication/src/providers/username-password.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@carlos-r-l-rodrigues carlos-r-l-rodrigues left a comment

Choose a reason for hiding this comment

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

LGTM

packages/authentication/package.json Outdated Show resolved Hide resolved
if (password_hash && typeof password_hash === "string") {
const buf = Buffer.from(password_hash, "base64")

const success = await Scrypt.verify(buf, password)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: experiment running that with worker_threads (it doesn't have to be in this PR)

@pKorsholm pKorsholm force-pushed the feat/authentication-provider-username-password branch from b57fde1 to 5bf05cc Compare January 22, 2024 10:06
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

@@ -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.


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?

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM

@kodiakhq kodiakhq bot merged commit 24bb26b into develop Jan 23, 2024
12 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants