Skip to content

task(auth): Check recovery-phone enabled config for all endpoints#18264

Merged
dschom merged 1 commit intomainfrom
add-recovery-enabled-config-check-to-all-recovery-phone-endpoints
Jan 23, 2025
Merged

task(auth): Check recovery-phone enabled config for all endpoints#18264
dschom merged 1 commit intomainfrom
add-recovery-enabled-config-check-to-all-recovery-phone-endpoints

Conversation

@dschom
Copy link
Copy Markdown
Contributor

@dschom dschom commented Jan 22, 2025

Because

  • We want this config to gate all recovery-phone endpoints

This pull request

  • Adds checks to see if config indicates recovery-phone is enabled.

Issue that this pull request solves

Closes: FXA-11020

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

@dschom dschom requested a review from a team as a code owner January 22, 2025 18:00
@dschom dschom force-pushed the add-recovery-enabled-config-check-to-all-recovery-phone-endpoints branch from c55fd4e to 64dba0f Compare January 22, 2025 19:34
Copy link
Copy Markdown
Contributor

@vpomerleau vpomerleau left a comment

Choose a reason for hiding this comment

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

Thanks for adding this safety in so we can make sure the feature is fully off when needed. I was wondering if we needed to differentiate between turning the feature off for adding new phones vs using existing phones as we're doing on the frontend but it's probably unnecessary/overkill.

  • Would we want tests for when the config is set to false?

@vbudhram
Copy link
Copy Markdown
Contributor

@dschom We also have this error

AppError.featureNotEnabled = function (retryAfter) {
that could be thrown in the handler.

@dschom
Copy link
Copy Markdown
Contributor Author

dschom commented Jan 22, 2025

@vbudhram I like the error better. I'll change this for everything except the /available handler.

@dschom
Copy link
Copy Markdown
Contributor Author

dschom commented Jan 22, 2025

Thanks for adding this safety in so we can make sure the feature is fully off when needed. I was wondering if we needed to differentiate between turning the feature off for adding new phones vs using existing phones as we're doing on the frontend but it's probably unnecessary/overkill.

* Would we want tests for when the config is set to false?

Yes, I'd say its overkill. In the event we do need something like this, it's simple to add.

Personally I think adding test cases for when the config is set is kind of a 'low value' test case, so I did not add these; however, when I switched to using errors as @vbudhram suggested I went ahead and added some test cases for this.

@dschom dschom force-pushed the add-recovery-enabled-config-check-to-all-recovery-phone-endpoints branch 2 times, most recently from b650598 to 1a51153 Compare January 22, 2025 23:45
@dschom dschom force-pushed the add-recovery-enabled-config-check-to-all-recovery-phone-endpoints branch from 1a51153 to 0a7d1dd Compare January 23, 2025 00:17
Copy link
Copy Markdown
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

Thanks!

};
} catch (error) {
if (error instanceof RecoveryPhoneNotEnabled) {
// In this case we won't throw an AppError. Unlike other endpoints,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍🏽

@dschom dschom merged commit 0a311ef into main Jan 23, 2025
@dschom dschom deleted the add-recovery-enabled-config-check-to-all-recovery-phone-endpoints branch January 23, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants