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

[PM-1222] Passkeys in the Bitwarden vault #2679

Merged
merged 31 commits into from
Oct 17, 2023

Conversation

coroiu
Copy link
Contributor

@coroiu coroiu commented Feb 8, 2023

Type of change

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

Objective

Introduces the ability to store Passkeys in the Bitwarden vault.

The functionality is behind the fido2-vault-credentials feature flag.

This PR represents the current state of the long-lived feature branch. Since we have a feature flag, we are introducing these changes to master before the feature is complete. The remaining work in https://bitwarden.atlassian.net/browse/PM-1222 will be introduced in a subsequent, separate PR.

Depends on bitwarden/clients#4715

Code changes

  • file.ext: Description of what was changed and why

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • 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

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

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

This looks good to me; I just added a suggestion. Thanks, @coroiu

@@ -164,6 +168,7 @@ private CipherLoginData ToCipherLoginData()
PasswordRevisionDate = Login.PasswordRevisionDate,
Totp = Login.Totp,
AutofillOnPageLoad = Login.AutofillOnPageLoad,
Fido2Key = Login.Fido2Key == null ? null : Login.Fido2Key.ToCipherLoginFido2KeyData(),
Copy link
Member

Choose a reason for hiding this comment

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

Would this be cleaner if we use the shorthand?

Fido2Key = Login.Fido2Key?.ToCipherLoginFido2KeyData();

Copy link
Member

Choose a reason for hiding this comment

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

I addressed this with #3317

gbubemismith and others added 2 commits May 8, 2023 14:06
* Added feature flag to enable pass keys

* Renamed enable pass keys to fido2 vault credentials
@gbubemismith gbubemismith marked this pull request as ready for review August 24, 2023 03:09
@gbubemismith gbubemismith requested a review from a team as a code owner August 24, 2023 03:09
@bitwarden-bot
Copy link

bitwarden-bot commented Aug 24, 2023

Logo
Checkmarx One – Scan Summary & Detailsc0f14260-0c5c-4a4f-a5ee-0f8664494414

No New Or Fixed Issues Found

@trmartin4 trmartin4 changed the title [EC-598] [BEEEP] Properly store passkeys in bitwarden [PM-1222] Passkeys in the Bitwarden vault Aug 24, 2023
trmartin4 and others added 6 commits August 31, 2023 13:53
* PM-1859 Refactor to credentialId

* PM-1859 Removed unnecessary import

---------

Co-authored-by: Andreas Coroiu <[email protected]>
* [PM-3807] feat: add discoverable property to fido2key

* [PM-3807] feat: remove standalone Fido2Key

* [PM-3807] chore: clean up unusued constant

* [PM-3807] fix: remove standadlone Fido2Key property that I missed
* [PM-3807] feat: store passkeys in array

* [PM-3807] amazing adventures with the c# linter
gbubemismith and others added 6 commits September 20, 2023 08:12
* Added creationDate property to the Fido2Key object

* Fixed lint issues

* fixed comments

* made createionDate required
…ty (#3262)

* [PM-3807] feat: add discoverable property to fido2key

* [PM-3807] feat: remove standalone Fido2Key

* [PM-3807] chore: clean up unusued constant

* [PM-3808] feat: add fido2 compatibility check before saving ciphers

* Resolved merge conflicts.

* Setting minimum version for QA.

---------

Co-authored-by: Todd Martin <[email protected]>
…g with org (#3328)

* Added compatibility checks.

* Refactored into separate methods for easier removal.

* Added check on ShareMany

* Updated method order to be consistent.

* Linting
gbubemismith
gbubemismith previously approved these changes Oct 16, 2023
Copy link
Member

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

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

Just had one question/suggestion about what looks like an unnecessary change. Otherwise, this looks good to me!

@trmartin4 trmartin4 merged commit 8c77c65 into master Oct 17, 2023
@trmartin4 trmartin4 deleted the EC-598-beeep-properly-store-passkeys-in-bitwarden branch October 17, 2023 16: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.

8 participants