[PM-10394] Add new item type ssh key #52
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -30,6 +33,8 @@ public class SyncController : Controller | |||
| private readonly IPolicyRepository _policyRepository; | |||
| private readonly ISendRepository _sendRepository; | |||
| private readonly GlobalSettings _globalSettings; | |||
| private readonly ICurrentContext _currentContext; | |||
| private readonly Version _sshKeyCipherMinimumVersion = new(Constants.SSHKeyCipherMinimumVersion); | |||
There was a problem hiding this comment.
style: Consider moving this constant to a centralized location for version-related constants
|
|
||
| private ICollection<CipherDetails> FilterSSHKeys(ICollection<CipherDetails> ciphers) | ||
| { | ||
| if (_currentContext.ClientVersion >= _sshKeyCipherMinimumVersion && _currentContext.DeviceType != DeviceType.Android && _currentContext.DeviceType != DeviceType.iOS && _currentContext.DeviceType != DeviceType.AndroidAmazon) |
There was a problem hiding this comment.
style: This condition is complex and may be difficult to maintain. Consider extracting it into a separate method for better readability
|
|
||
| public class CipherSSHKeyModel | ||
| { | ||
| public CipherSSHKeyModel() { } |
There was a problem hiding this comment.
style: Empty constructor might allow creation of invalid objects. Consider making it private if not needed publicly.
| public CipherSSHKeyModel(CipherSSHKeyData data) | ||
| { | ||
| PrivateKey = data.PrivateKey; | ||
| PublicKey = data.PublicKey; | ||
| KeyFingerprint = data.KeyFingerprint; | ||
| } |
There was a problem hiding this comment.
logic: Constructor doesn't validate input. Consider adding null checks for data and its properties.
| [EncryptedString] | ||
| [EncryptedStringLength(5000)] | ||
| public string PrivateKey { get; set; } |
There was a problem hiding this comment.
style: 5000 characters might be excessive for a private key. Verify if this limit is appropriate.
| [EncryptedString] | ||
| [EncryptedStringLength(1000)] | ||
| public string KeyFingerprint { get; set; } |
There was a problem hiding this comment.
style: Key fingerprints are usually much shorter. 1000 characters seems excessive.
| case CipherType.SSHKey: | ||
| var sshKeyData = JsonSerializer.Deserialize<CipherSSHKeyData>(cipher.Data); | ||
| Data = sshKeyData; | ||
| cipherData = sshKeyData; | ||
| SSHKey = new CipherSSHKeyModel(sshKeyData); | ||
| break; |
There was a problem hiding this comment.
style: Consider adding a null check for sshKeyData after deserialization to prevent potential null reference exceptions
| @@ -76,6 +82,7 @@ public CipherMiniResponseModel(Cipher cipher, IGlobalSettings globalSettings, bo | |||
| public CipherCardModel Card { get; set; } | |||
| public CipherIdentityModel Identity { get; set; } | |||
| public CipherSecureNoteModel SecureNote { get; set; } | |||
| public CipherSSHKeyModel SSHKey { get; set; } | |||
There was a problem hiding this comment.
style: Ensure that the SSHKey property is properly documented with XML comments for consistency with other properties
| public string PrivateKey { get; set; } | ||
| public string PublicKey { get; set; } | ||
| public string KeyFingerprint { get; set; } |
There was a problem hiding this comment.
style: Consider using [EncryptedString] attribute for sensitive data
| @@ -0,0 +1,10 @@ | |||
| namespace Bit.Core.Vault.Models.Data; | |||
|
|
|||
| public class CipherSSHKeyData : CipherData | |||
There was a problem hiding this comment.
style: Add XML documentation for the class and its purpose
🎟️ Tracking
Server: bitwarden#4575
Add Item Type: bitwarden/clients#10360
Add SSH Agent: bitwarden/clients#10293
Add Import/Export: bitwarden/clients#10529
Jira: https://bitwarden.atlassian.net/browse/PM-10395
📔 Objective
Add server support for the new ssh key cipher type. This is mostly copy paste from the other cipher types, with the one exception that we are filtering out ssh keys for older clients, using SSHKeyCipherMinimumVersion. We will update this once we know which release ssh keys will be in.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesGreptile Summary
This pull request introduces support for a new SSH key cipher type in the Bitwarden server, expanding the vault's capabilities to store and manage SSH keys.
CipherSSHKeyModelandCipherSSHKeyDataclasses to represent SSH key data with encrypted propertiesSyncControllerto filter SSH keys based on client version and device typeCipherRequestModelandCipherResponseModelto handle the new SSH key cipher typeSSHKeyenum value toCipherTypeand introduced related constants for version control