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

[PS-589] Fix emergency contact takeover device verification and endpoints for its settings #2016

Merged

Conversation

fedemkr
Copy link
Member

@fedemkr fedemkr commented May 25, 2022

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Fix issue when emergency contact takes over the owner's account by disabling 2FA for new devices login (device verification). Also added endpoints for getting/putting device verification settings.

Code changes

  • TwoFactorController.cs: Added endpoints for getting/updating device verification settings
  • User.cs: Added UnknownDeviceVerificationEnabled property to check whether the user has the feature enabled.
  • UserService.cs: Added CanEditDeviceVerificationSettings(...) to check whether the user is allowed to edit the device verification settings and refactored a bit Needs2FABecauseNewDeviceAsync to use the new method as well.
  • EmergencyAccessService.cs: Disabled device verification from the grantor to fix the emergency contact takeover block issue.
  • ClaimsExtensions.cs: Added this to check if the user has been logged in using SSO, i.e. if it has a claim "idp" with value "sso".
  • Others: DB migrations and tests.

Before you submit

  • I have checked for formatting errors (dotnet tool run dotnet-format --check) (required)
  • If making database changes - I have also updated Entity Framework queries and/or migrations
  • I have added unit tests where it makes sense to do so (encouraged but not required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

…n emergency contact takes over the account. Also added endpoints to get and update 2fa device verification settings. And Updated migrations & tests
@fedemkr fedemkr requested a review from a team May 25, 2022 19:47
Copy link
Member

@justindbaur justindbaur left a comment

Choose a reason for hiding this comment

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

Overall really great. I just think true would be a sane default to establish at the database level.

src/Core/Entities/User.cs Outdated Show resolved Hide resolved
src/Core/Utilities/ClaimsExtensions.cs Outdated Show resolved Hide resolved
test/Core.Test/Services/UserServiceTests.cs Outdated Show resolved Hide resolved
src/Api/Controllers/TwoFactorController.cs Outdated Show resolved Hide resolved
@fedemkr fedemkr requested a review from justindbaur May 26, 2022 11:41
Copy link
Member

@justindbaur justindbaur left a comment

Choose a reason for hiding this comment

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

Looks great!

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.

2 participants