-
Notifications
You must be signed in to change notification settings - Fork 4
feat/email-registration #212
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
Conversation
81894da to
66e733f
Compare
brh28
left a comment
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.
There's already the following definition in the graphql schema, which appears to the same or similar to what's being done in this PR:
userEmailRegistrationInitiate(input: UserEmailRegistrationInitiateInput!): UserEmailRegistrationInitiatePayload!
userEmailRegistrationValidate(input: UserEmailRegistrationValidateInput!): UserEmailRegistrationValidatePayload!
These definitions do not allow for a email registration without an existing account. The choice was to modify these, to handle both cases (with account and without account) or create a new schema definition for new accounts. |
66e733f to
90f293e
Compare
|
@brh28 please review |
| // so that if one fails, the other is rolled back | ||
|
|
||
| // 1. Update user record with email (deviceId is preserved via spread) | ||
| const userUpdated = await UsersRepository().findById(userId) |
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.
Lines 60-65 can be consolidated to a single database update. The function would be something like:
addEmail: (userId, email) => db.updateOne( { userId }, { $set: { email })
| userId: UserId | ||
| email: EmailAddress | ||
| }): Promise<Account | RepositoryError> => { | ||
| // TODO: ideally both 1. and 2. should be done in a transaction, |
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.
I don't believe there's a way to make this atomic unless we completely rewrite our data model
|
|
||
| import { createAccountWithEmailIdentifier } from "@app/accounts" | ||
|
|
||
| export const createAccountFromEmailRegistrationPayload = async ({ |
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.
where is this function being called?
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.
nowhere! apparently its leftover from a previous attempt at getting this to work. Removed the file and the export reference
brh28
left a comment
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.
Can we do a code walk through on this one? I'm having a hard time following
|
Related to: #237 |
|
@brh28 I think this is ready for a code review again, since we patched the orphaned accounts issue. |
| const accountsRepo = AccountsRepository() | ||
| let account = await accountsRepo.findByUserId(kratosUserId) | ||
|
|
||
| if (account instanceof Error) { |
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.
probably should be checking for a specific response, such as AccountNotFoundError
| } else { | ||
| // Create new identity with email | ||
| const createIdentityBody = { | ||
| credentials: { password: { config: { password } } }, |
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.
does password have a value?
| } | ||
|
|
||
| // Send OTP code via recovery flow | ||
| const { data: recoveryFlow } = await kratosPublic.createNativeRecoveryFlow() |
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.
verify we want createNativeRecoveryFlow rather than createNativeRegistrationFlow
| type: [String], | ||
| }, | ||
| deviceId: { | ||
| email: { |
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.
emails are already stored in Kratos. Is there any reason not to use the postrgres database here?
- New GraphQL mutations: newUserEmailRegistrationInitiate and newUserEmailRegistrationValidate for email-only account creation - Kratos integration: Successfully using email recovery flow for OTP delivery - Account creation: Fixed critical bug where accounts weren't being created after validation - Code cleanup: Removed all debug console.log statements ✅ Key Changes Made 1. Fixed validation logic - Changed from checking User existence to Account existence 2. Proper account creation - Creates account with wallets when none exists 3. Clean production code - Removed debug statements for production readiness
This commit consolidates all TypeScript fixes required after rebasing the email-registration feature branch on main: - Update core Account and Wallet mocks with mandatory properties (npub, lnurlp) - Adapt to upstream API changes (@ory/client IdentityState → IdentityStateEnum) - Fix test infrastructure mock type conversions and exports - Replace deleted BTC wallet functions with USD equivalents - Use factory functions for branded types (OnChainAddress, FractionalCentAmount) - Fix OffersManager constructability and CSV export method names - Add type casts for transaction and wallet type incompatibilities Result: yarn tsc --noEmit returns 0 errors
fdea49c to
204ac45
Compare
…lidation deferral - Add ValidationError import to redeem-invite.ts to fix TypeScript errors - Document that email validation is deferred until email-only registration feature is available (see PR #212)
|
Closed as deferred |
Summary
Implements email-only authentication flow for new user registration, allowing users to sign up with just an email address (no phone required).
Key Changes:
newUserEmailRegistrationInitiateandnewUserEmailRegistrationValidateemail_no_password_v0)How It Works
newUserEmailRegistrationInitiatewith emailnewUserEmailRegistrationValidatewith flowId + codeFiles Changed (43 files, +855/-216)
Core Implementation:
src/graphql/public/root/mutation/new-user-email-registration-*.ts- GraphQL mutationssrc/app/authentication/email.ts- Email authentication logicsrc/services/kratos/auth-email-no-password.ts- Kratos integrationsrc/app/accounts/create-account.ts- Account creation updatessrc/app/accounts/upgrade-device-account.ts- Device → Email upgradesrc/domain/authentication/registration-payload-validator.ts- Validation logicdev/ory/kratos.yml- Kratos configurationTest Infrastructure:
Known Issues / Follow-up Tickets
🔴 HIGH PRIORITY (security review needed)
Account Enumeration Vulnerability
src/services/kratos/auth-email-no-password.tslines 73-78createIdentityForEmailRegistration()reveals whether email is already registeredMissing TOTP Flow Completion
newUserEmailRegistrationValidatereturnstotpRequiredbut no follow-up verificationRace Condition in Account Creation
new-user-email-registration-validate.tslines 63-78🟡 MEDIUM PRIORITY (tech debt)
SchemaIdType.EmailNoPasswordV0enumupgrade-device-account.ts🟢 LOW PRIORITY
Testing
yarn tsc --noEmit- 0 errors)Checklist