Verify before update email#14
Conversation
📝 WalkthroughWalkthroughThis PR changes email updates to use verification links, adds pending email-change state, and shows the pending value in account views and a new confirmation alert. ChangesPending Email Change Verification Flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PSchmiedmayer
left a comment
PSchmiedmayer
left a comment
There was a problem hiding this comment.
Thanks for looking into this @PaulGoldschmidt 🚀
|
|
||
|
|
||
| func presentEmailChangeNotice(for newEmail: String) { | ||
| pendingEmailAddress = newEmail |
There was a problem hiding this comment.
We should probably show something in the main UI where we show the email that this is still in process. Probably can't and shouldn't persist it across app launches, but at least while the user goes back in the UI for the same change, we can display this. If we would persist it across launches, then we need to pull that information from Firebase (which I don't think we can), so keeping it as local state might be a good middle ground.
There was a problem hiding this comment.
agree with the persistence part here, I will try to find a good way to show this at least in the session lifecycle.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SpeziFirebaseAccount/FirebaseAccountService.swift (1)
582-608: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPending-change notice is skipped if a later step in the same update fails.
sendEmailVerification(beforeUpdatingEmail:)is a real, non-idempotent Firebase side effect (an email is actually sent). ButpresentEmailChangeNotice(...)is only called at Line 601, after the entiremapFirebaseAccountErrorblock (Lines 582-599) completes successfully. If the email verification succeeds but a subsequent step in the same block —updatePassword(to:)orupdateDisplayName— throws, the function exits before ever recording the pending state or callingsupplyUserDetails. The user has already received a verification email, but the UI never shows the pending notice, and a retry could trigger another verification email to be sent.Move the notice call immediately after the successful
sendEmailVerificationcall, inside themapFirebaseAccountErrorclosure, so the pending state is tracked regardless of what happens afterward.🔧 Proposed fix
try await mapFirebaseAccountError { if modifications.modifiedDetails.contains(AccountKeys.userId) { logger.debug("sendEmailVerification(beforeUpdatingEmail:) for user.") // `updateEmail(to:)` is deprecated and fails when email enumeration protection is enabled (the default). // This call only sends a verification link to the new address; the email is updated once the user opens it, // at which point Firebase revokes the user's tokens and the user has to sign in again. try await currentUser.sendEmailVerification(beforeUpdatingEmail: modifications.modifiedDetails.userId) + // record the pending state immediately after the (non-idempotent) email was sent, so a later + // failure in this block (e.g., updatePassword) doesn't leave the UI unaware that a link was sent + firebaseModel.presentEmailChangeNotice( + PendingEmailChange(accountId: currentUser.uid, emailAddress: modifications.modifiedDetails.userId) + ) } if let password = modifications.modifiedDetails.password { logger.debug("updatePassword(to:) for user.") try await currentUser.updatePassword(to: password) } if let name = modifications.modifiedDetails.name { try await updateDisplayName(of: currentUser, name) } } - if modifications.modifiedDetails.contains(AccountKeys.userId) { - // the email change is pending until the user opens the verification link; present a notice and track the - // pending state so that views can display it alongside the current email address (see `withPendingEmailChange`) - firebaseModel.presentEmailChangeNotice( - PendingEmailChange(accountId: currentUser.uid, emailAddress: modifications.modifiedDetails.userId) - ) - } - var externalModifications = modifications🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/SpeziFirebaseAccount/FirebaseAccountService.swift` around lines 582 - 608, The pending email-change notice is being recorded too late in FirebaseAccountService.update, after the full mapFirebaseAccountError block succeeds. Move the presentEmailChangeNotice(PendingEmailChange(...)) call into the same branch as sendEmailVerification(beforeUpdatingEmail:) immediately after that call, so the pending state is saved even if updatePassword(to:) or updateDisplayName(of:_:) fails later. Keep the logic keyed off modifications.modifiedDetails.contains(AccountKeys.userId) and use the existing currentUser.uid and modifications.modifiedDetails.userId values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/SpeziFirebaseAccount/FirebaseAccountService.swift`:
- Around line 582-608: The pending email-change notice is being recorded too
late in FirebaseAccountService.update, after the full mapFirebaseAccountError
block succeeds. Move the presentEmailChangeNotice(PendingEmailChange(...)) call
into the same branch as sendEmailVerification(beforeUpdatingEmail:) immediately
after that call, so the pending state is saved even if updatePassword(to:) or
updateDisplayName(of:_:) fails later. Keep the logic keyed off
modifications.modifiedDetails.contains(AccountKeys.userId) and use the existing
currentUser.uid and modifications.modifiedDetails.userId values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a854a420-b2ce-4c6b-a5bb-9cdb85c8d53b
📒 Files selected for processing (9)
Sources/SpeziAccount/AccountValue/Collections/AccountDetails.swiftSources/SpeziAccount/AccountValue/Keys/PendingUserIdKey.swiftSources/SpeziAccount/Resources/Localizable.xcstringsSources/SpeziAccount/ViewModel/AccountDisplayModel.swiftSources/SpeziAccount/Views/AccountOverview/AccountOverviewHeader.swiftSources/SpeziAccount/Views/AccountOverview/NameOverview.swiftSources/SpeziFirebaseAccount/FirebaseAccountService.swiftSources/SpeziFirebaseAccount/Models/FirebaseAccountModel.swiftSources/SpeziFirebaseAccount/Views/FirebaseSecurityAlert.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/SpeziAccount/AccountValue/Collections/AccountDetails.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/SpeziFirebaseAccount/Views/FirebaseSecurityAlert.swift
PSchmiedmayer
left a comment
PSchmiedmayer
left a comment
There was a problem hiding this comment.
Thanks @PaulGoldschmidt; I'll let @lukaskollmer give a review here and then happy to merge once all CI is passing 👍
|
i had a look at this, and while it works it does introduce the side effect of forcing an unconditional logout of the user. which ideally is something we'd like avoid, especially in MHC atm, since we don't yet sync the scheduler state with the cloud backend, meaning that the user changing their email (and being forced to log out and back in as part of the process) will reset their local study participation state, which is not good. |
|
Ok, an other argument that the state storage and restoration for Study/Scheduler is a high priority there |
|
@PSchmiedmayer agreed |
### ♻️ Current situation & Problem @PSchmiedmayer had a stab at various package-wide improvements and fixes in #17, such as addressing warnings, deprecations, etc. this work also partly overlaps with some things addressed in #5 (which probably will end up getting superseded by this PR). this PR picks up these adjustments (which initially were developed against #16), and carries them over into the current #20-based main branch state of the codebase. somewhat complete list of changes: - most of what was covered by #17 - address compiler warnings where possible - address deprecation warnings where possible - note that this PR intentionally does **not** address the firebase `updateEmail(to:)` deprecation, as there currently are some issues around the behaviour of the new API. see also #14 - some testing fixes that were left out of #20 - now that the package's effective availability is iOS 18+, we can remove a bunch of `if #available(iOS 18, *)` checks from all over the codebase - *todo* ### ⚙️ Release Notes *todo* ### 📚 Documentation *todo* ### ✅ Testing *todo* ### Code of Conduct & Contributing Guidelines By creating and submitting this pull request, you agree to follow our [Code of Conduct](https://github.com/SchmiedmayerLab/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/SchmiedmayerLab/.github/blob/main/CONTRIBUTING.md): - [x] I agree to follow the [Code of Conduct](https://github.com/SchmiedmayerLab/.github/blob/main/CODE_OF_CONDUCT.md) and [Contributing Guidelines](https://github.com/SchmiedmayerLab/.github/blob/main/CONTRIBUTING.md).
3 participants
Verify before update email
♻️ Current situation & Problem
With newer Firebase projects, Email Enumeration Protection is enabled by default, requiring Spezi firebase to call the function
sendEmailVerification()instead ofupdateEmail(to:).updateEmail(to:)is deprecated and fails when email enumeration protection is enabled (the default). This call only sends a verification link to the new address; the email is updated once the user opens it, at which point Firebase revokes the user's tokens and the user has to sign in again.⚙️ Release Notes
We need to let the user know that the email change is pending until the user opens the verification link. This is why this PR introduces a isEmailChangeNoticePresented view, in which the user is let known that we've sent a confirmation link to his address.
📚 Documentation
N/A
✅ Testing
Happy for your input here, @lukaskollmer!
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:
Summary by CodeRabbit
pendingUserIdaccount metadata.