Skip to content

[PM-2408] Pending admin approval requests#2643

Merged
andrebispo5 merged 17 commits intoauth/pm-2293-tde-auth-requestsfrom
auth/pm-2408-admin-preapproved
Jul 27, 2023
Merged

[PM-2408] Pending admin approval requests#2643
andrebispo5 merged 17 commits intoauth/pm-2293-tde-auth-requestsfrom
auth/pm-2408-admin-preapproved

Conversation

@andrebispo5
Copy link
Contributor

Merge after bitwarden/mobile#2642

Type of change

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

Objective

Reuse created admin requests when trying to approve a login with TDE Admin approval request.

Code changes

Store admin request id and private key to secure storage.
When opening the screen check for pending admin requests.
If there is a pending admin request populate the vm data with the saved request.

Screenshots

Screen.Recording.2023-07-25.at.17.15.11.mov

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@andrebispo5 andrebispo5 changed the base branch from master to auth/pm-2293-tde-auth-requests July 25, 2023 16:30
@andrebispo5 andrebispo5 requested review from a team and jlf0dev July 25, 2023 16:31
@bitwarden-bot
Copy link

Logo
Checkmarx One – Scan Summary & Detailse7700b32-9009-4a78-ba31-b3cb317e50f6

No New Or Fixed Issues Found

PasswordlessLoginResponse response = null;
if (await _stateService.IsAuthenticatedAsync())
{
response = await _authService.GetPasswordlessLoginRequestByIdAsync(_requestId);
Copy link
Member

Choose a reason for hiding this comment

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

Now that I'm looking at this, we should probably indicate that this requires authentication whereas the other one doesn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far, we don't have any standard to indicate that endpoint need authentication. Should we place a method comment/doc for the GetPasswordlessLoginRequestByIdAsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Considering you can't get any device auth requests using this method, we might want to rename it to be GetAdminAuthRequest or something similar. We might expand it in the future, but lets make it a little more specific for 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.

This method is also used for the Login With Device it is not specific to the admin requests.

@andrebispo5 andrebispo5 requested review from fedemkr and jlf0dev July 27, 2023 11:41
Copy link
Member

@jlf0dev jlf0dev left a comment

Choose a reason for hiding this comment

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

Looks good to me, I'll let Fede give final approval

@andrebispo5 andrebispo5 requested a review from jlf0dev July 27, 2023 16:45
@andrebispo5 andrebispo5 requested a review from fedemkr July 27, 2023 16:45
andrebispo5 and others added 2 commits July 27, 2023 17:55
* [PM-1208] Add navigation to device approval options after 2FA auth when TDE is enabled

* [PM-1208] Add navigations to iOS extensions
Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>
@andrebispo5 andrebispo5 requested a review from fedemkr July 27, 2023 16:59
@andrebispo5 andrebispo5 merged commit 6edac4f into auth/pm-2293-tde-auth-requests Jul 27, 2023
@andrebispo5 andrebispo5 deleted the auth/pm-2408-admin-preapproved branch July 27, 2023 17:17
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.

4 participants