Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 1 addition & 10 deletions Bitwarden.xcworkspace/xcshareddata/swiftpm/Package.resolved

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 33 additions & 0 deletions BitwardenKit/Core/Platform/Utilities/Base64UrlEncoder.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import Foundation

extension Data {
public func base64UrlEncodedString(trimPadding: Bool? = true) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿค” Is this function really necessary when you can just .base64EncodedString().urlEncoded()? If we really care about whether or not we trim the padding, the current function can just be modified, I think?

let shouldTrim = if trimPadding != nil { trimPadding! } else { true }
Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿ“ You can just say trimPadding == true, which covers the nil case already.

๐Ÿค” But why is trimPadding even optional in the first place? It seems to me like it shouldn't be, especially if we give it a default value.

let encoded = base64EncodedString().replacingOccurrences(of: "+", with: "-").replacingOccurrences(of: "/", with: "_")
if shouldTrim {
return encoded.trimmingCharacters(in: CharacterSet(["="]))
} else {
return encoded
}
}

public init?(base64UrlEncoded str: String) {
self.init(base64Encoded: normalizeBase64Url(str))
}
}

private func normalizeBase64Url(_ str: String) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐Ÿค” I don't think we like having global functions if at all possible. This really feels like something that should be an extension on String to me

let hasPadding = str.last == "="
let padding = if !hasPadding {
switch str.count % 4 {
case 2: "=="
case 3: "="
default: ""
}
} else { "" }
return str
.replacingOccurrences(of: "-", with: "+")
.replacingOccurrences(of: "_", with: "/")
+ padding
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import Foundation
import Networking

struct SecretVerificationRequestModel: JSONRequestBody, Equatable {
static let encoder = JSONEncoder()
Copy link
Contributor

Choose a reason for hiding this comment

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

โ›๏ธ Personally, I think this should also have a MARK for this section. That said, why use a new JSON encoder instead of the one we already have?


// MARK: Properties

let authRequestAccessCode: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ I think these properties should all have documentation comments

let masterPasswordHash: String?
let otp: String?


Copy link
Contributor

Choose a reason for hiding this comment

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

โ›๏ธ Missing a MARK for initializers

init(passwordHash: String) {
authRequestAccessCode = nil
masterPasswordHash = passwordHash
otp = nil
}

init(otp: String) {
masterPasswordHash = nil
self.otp = otp
authRequestAccessCode = nil
}

init(accessCode: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ I think these inits should be alphabetized, and they all need documentation comments

authRequestAccessCode = accessCode
masterPasswordHash = nil
otp = nil
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import Foundation
import Networking

// MARK: - SaveCredentialRequestModel

/// The request body for an answer login request request.
///
struct WebAuthnLoginSaveCredentialRequestModel: JSONRequestBody, Equatable {
static let encoder = JSONEncoder()
Copy link
Contributor

Choose a reason for hiding this comment

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

โ›๏ธ See comment earlier about not using our standard encoder.


// MARK: Properties
// The response received from the authenticator.
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ There should be a space on either side of a MARK. As well, documentation comments are three slashes, not two. If you just do a normal comment, it won't show up properly in Xcode.

// This contains all information needed for future authentication flows.
let deviceResponse: WebAuthnLoginAttestationResponseRequest

// Nickname chosen by the user to identify this credential
let name: String

// Token required by the server to complete the creation.
// It contains encrypted information that the server needs to verify the credential.
let token: String

// True if the credential was created with PRF support.
let supportsPrf: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Properties should be in alphabetical order


// Used for vault encryption. See {@link RotateableKeySet.encryptedUserKey }
let encryptedUserKey: String?

// Used for vault encryption. See {@link RotateableKeySet.encryptedPublicKey }
let encryptedPublicKey: String?

// Used for vault encryption. See {@link RotateableKeySet.encryptedPrivateKey }
let encryptedPrivateKey: String?
}

struct WebAuthnLoginAttestationResponseRequest: Encodable, Equatable {
let id: String
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ These should be alphabetized and should have documentation comments, as should the struct. As well, if there are multiple top-level entities in the file, they each should get a MARK: - comment.

let rawId: String
let type: String
// let extensions: [String: Any]
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Personally, I think any code that's commented out should include a comment indicating why it's being kept around in comments. Otherwise, it should just be deleted.

let response: WebAuthnLoginAttestationResponseRequestInner
}

struct WebAuthnLoginAttestationResponseRequestInner: Encodable, Equatable {
let attestationObject: String
let clientDataJson: String

Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Extra line at end of scope

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import Foundation
import Networking

struct WebAuthnLoginCredentialAssertionOptionsResponse: JSONResponse, Equatable, Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ The struct should have documentation comments

/// Options to be provided to the webauthn authenticator.
let options: PublicKeyCredentialAssertionOptions;

/// Contains an encrypted version of the {@link options}.
/// Used by the server to validate the attestation response of newly created credentials.
let token: String;
}

struct PublicKeyCredentialAssertionOptions: Codable, Equatable, Hashable {
let allowCredentials: [BwPublicKeyCredentialDescriptor]?
Copy link
Contributor

Choose a reason for hiding this comment

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

โ›๏ธ Documentation comments

let challenge: String
let extensions: AuthenticationExtensionsClientInputs?
let rpId: String
let timeout: Int?
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import Foundation
import Networking

struct WebAuthnLoginCredentialCreationOptionsResponse: JSONResponse, Equatable, Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ As noted above, I think each of these should be MARKed out, with documentation comments on all of the properties and structs

/// Options to be provided to the webauthn authenticator.
let options: PublicKeyCredentialCreationOptions;

/// Contains an encrypted version of the {@link options}.
/// Used by the server to validate the attestation response of newly created credentials.
let token: String;
}

struct PublicKeyCredentialCreationOptions: Codable, Equatable, Hashable {
// attestation?: AttestationConveyancePreference
// let authenticatorSelection: AuthenticatorSelectionCriteria?
let challenge: String
let excludeCredentials: [BwPublicKeyCredentialDescriptor]?
let extensions: AuthenticationExtensionsClientInputs?
let pubKeyCredParams: [BwPublicKeyCredentialParameters]
let rp: BwPublicKeyCredentialRpEntity
let timeout: Int?
let user: BwPublicKeyCredentialUserEntity
}


struct AuthenticationExtensionsClientInputs: Codable, Equatable, Hashable {
let prf: AuthenticationExtensionsPRFInputs?
}

struct AuthenticationExtensionsPRFInputs: Codable, Equatable, Hashable {
let eval: AuthenticationExtensionsPRFValues?
let evalByCredential: [String: AuthenticationExtensionsPRFValues]?
}

struct AuthenticationExtensionsPRFValues: Codable, Equatable, Hashable {
let first: String
let second: String?
}

struct BwPublicKeyCredentialDescriptor: Codable, Equatable, Hashable {
let type: String
let id: String
// let transports: [String]?
}

struct BwPublicKeyCredentialParameters: Codable, Equatable, Hashable {
let type: String
let alg: Int
}

struct BwPublicKeyCredentialRpEntity: Codable, Equatable, Hashable {
let id: String
let name: String
}

struct BwPublicKeyCredentialUserEntity: Codable, Equatable, Hashable {
let id: String
let name: String
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ protocol AuthAPIService {
/// - Returns: The pending login request.
///
func checkPendingLoginRequest(withId id: String, accessCode: String) async throws -> LoginRequest

/// Retrieves the parameters for creating a new WebAuthn credential.
/// - Parameters:
/// - request: The data needed to send the request.
func getCredentialCreationOptions(_ request: SecretVerificationRequestModel) async throws -> WebAuthnLoginCredentialCreationOptionsResponse

/// Retrieves the parameters for authenticating with a WebAuthn credential.
/// - Parameters:
/// - request: The data needed to send the request.
func getCredentialAssertionOptions(_ request: SecretVerificationRequestModel) async throws -> WebAuthnLoginCredentialAssertionOptionsResponse
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ These should be alphabetized


/// Performs the identity token request and returns the response.
///
Expand Down Expand Up @@ -83,6 +93,8 @@ protocol AuthAPIService {
/// - model: The data needed to send the request.
///
func updateTrustedDeviceKeys(deviceIdentifier: String, model: TrustedDeviceKeysRequestModel) async throws

func saveCredential(_ model: WebAuthnLoginSaveCredentialRequestModel) async throws
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ This both needs documentation comments, and to be slotted in alphabetically

}

extension APIService: AuthAPIService {
Expand All @@ -93,6 +105,14 @@ extension APIService: AuthAPIService {
func checkPendingLoginRequest(withId id: String, accessCode: String) async throws -> LoginRequest {
try await apiUnauthenticatedService.send(CheckLoginRequestRequest(accessCode: accessCode, id: id))
}

func getCredentialCreationOptions(_ request: SecretVerificationRequestModel) async throws -> WebAuthnLoginCredentialCreationOptionsResponse {
try await apiService.send(WebAuthnLoginGetCredentialCreationOptionsRequest(requestModel: request))
}

func getCredentialAssertionOptions(_ request: SecretVerificationRequestModel) async throws -> WebAuthnLoginCredentialAssertionOptionsResponse {
try await apiService.send(WebAuthnLoginGetCredentialAssertionOptionsRequest(requestModel: request))
}

func getIdentityToken(_ request: IdentityTokenRequestModel) async throws -> IdentityTokenResponseModel {
try await identityService.send(IdentityTokenRequest(requestModel: request))
Expand Down Expand Up @@ -133,6 +153,10 @@ extension APIService: AuthAPIService {
_ = try await apiUnauthenticatedService.send(ResendNewDeviceOtpRequest(model: model))
}

func saveCredential(_ model: WebAuthnLoginSaveCredentialRequestModel) async throws {
_ = try await apiService.send(WebAuthnLoginSaveCredentialRequest(requestModel: model))
}

func updateTrustedDeviceKeys(deviceIdentifier: String, model: TrustedDeviceKeysRequestModel) async throws {
_ = try await apiService.send(TrustedDeviceKeysRequest(deviceIdentifier: deviceIdentifier, requestModel: model))
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import Networking

struct WebAuthnLoginGetCredentialAssertionOptionsRequest : Request {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Documentation comments

As well, we don't put spaces around colons: Object: Protocol

typealias Response = WebAuthnLoginCredentialAssertionOptionsResponse

var body: SecretVerificationRequestModel? { requestModel }

var path: String { "/webauthn/assertion-options" }

var method: HTTPMethod { .post }

let requestModel: SecretVerificationRequestModel
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import Networking

struct WebAuthnLoginGetCredentialCreationOptionsRequest : Request {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Documentation comments and extraneous space around colon

typealias Response = WebAuthnLoginCredentialCreationOptionsResponse

var body: SecretVerificationRequestModel? { requestModel }

var path: String { "/webauthn/attestation-options" }

var method: HTTPMethod { .post }

let requestModel: SecretVerificationRequestModel
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import Networking

struct WebAuthnLoginSaveCredentialRequest : Request {
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Documentation comments and extraneous space before colon

typealias Response = EmptyResponse

var body: WebAuthnLoginSaveCredentialRequestModel? { requestModel }

var path: String { "/webauthn" }

var method: HTTPMethod { .post }

let requestModel: WebAuthnLoginSaveCredentialRequestModel
}
Loading
Loading