Skip to content

Do not allow confirming email addresses via add email controller for unconfirmed accounts#6897

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/do-not-confirm-emails-for-unconfirmed-accounts-in-add-email-controller
Sep 1, 2022
Merged

Do not allow confirming email addresses via add email controller for unconfirmed accounts#6897
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/do-not-confirm-emails-for-unconfirmed-accounts-in-add-email-controller

Conversation

@mitchellhenke
Copy link
Contributor

No description provided.

@aduth
Copy link
Contributor

aduth commented Sep 1, 2022

Per failing build, you might need to rebase this to pull in changes from #6893.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/do-not-confirm-emails-for-unconfirmed-accounts-in-add-email-controller branch from 24d7c84 to 091f2a2 Compare September 1, 2022 15:54
Copy link
Contributor

@aduth aduth Sep 1, 2022

Choose a reason for hiding this comment

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

Other methods in this class call methods on this return value without nil-safe operators. Do we need to update those, or add an early return somewhere?

email_address.update!(confirmed_at: Time.zone.now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding of the existing implementation is that email_address being nil is expected (as a failure condition). I think it returns nil when the token is not associated with an email address, and this change is mostly extending the cases where we'll return nil.

https://github.com/18F/identity-idp/blob/091f2a2ad6d3b5941b2972c6244182ec724e5307/app/services/email_confirmation_token_validator.rb#L17

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/do-not-confirm-emails-for-unconfirmed-accounts-in-add-email-controller branch 2 times, most recently from 091f2a2 to f6e4c3e Compare September 1, 2022 17:24
…unconfirmed accounts

changelog: Bug Fixes, Email Confirmation, Do not allow confirming email addresses via add email controller for unconfirmed accounts
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/do-not-confirm-emails-for-unconfirmed-accounts-in-add-email-controller branch from f6e4c3e to b69ff0b Compare September 1, 2022 18:10
@mitchellhenke mitchellhenke merged commit e9fab47 into main Sep 1, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/do-not-confirm-emails-for-unconfirmed-accounts-in-add-email-controller branch September 1, 2022 18:59
mitchellhenke pushed a commit that referenced this pull request Sep 1, 2022
…unconfirmed accounts (#6897)

changelog: Bug Fixes, Email Confirmation, Do not allow confirming email addresses via add email controller for unconfirmed accounts
@zachmargolis zachmargolis mentioned this pull request Sep 7, 2022
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