Skip to content
Draft
Show file tree
Hide file tree
Changes from 7 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
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
}
31 changes: 30 additions & 1 deletion BitwardenShared/Core/Auth/Services/AuthService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ enum AuthError: Error {

/// There was a problem generating the request to resend the email with new device otp.
case unableToResendNewDeviceOtp

/// There was a problem generating the device passkey or PRF encryption key.
case unableToCreateDevicePasskey
Copy link
Contributor

Choose a reason for hiding this comment

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

๐ŸŽจ Enum cases should be in alphabetical order

}

// MARK: - LoginUnlockMethod
Expand Down Expand Up @@ -89,6 +92,9 @@ protocol AuthService {
/// Check the status of the pending login request for the unauthenticated user.
///
func checkPendingLoginRequest(withId id: String) async throws -> LoginRequest

/// Create device passkey with PRF encryption key.
func createDevicePasskey(masterPasswordHash: String) async throws

/// Deny all the pending login requests.
///
Expand Down Expand Up @@ -241,6 +247,18 @@ extension AuthService {
}
}

struct DevicePasskeyRecord: Decodable, Encodable {
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, though this also feels like something that should be in a separate file

let credId: String
let privKey: String
let prfSeed: String
let rpId: String
let rpName: String?
let userId: String?
let userName: String?
let userDisplayName: String?
let creationDate: DateTime
}

// MARK: - DefaultAuthService

/// The default implementation of `AuthService`.
Expand Down Expand Up @@ -272,6 +290,9 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
/// The store which makes credential identities available to the system for AutoFill suggestions.
private let credentialIdentityStore: CredentialIdentityStore

/// The service used to create and assert the device passkey for logging into remote clients.
private let devicePasskeyService: DevicePasskeyService

/// The service used by the application to manage the environment settings.
private let environmentService: EnvironmentService

Expand Down Expand Up @@ -326,6 +347,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
/// - configService: The service to get server-specified configuration.
/// - credentialIdentityStore: The store which makes credential identities available to the
/// system for AutoFill suggestions.
/// - devicePasskeyService: The service used to create and assert the device passkey for logging into remote clients.
/// - environmentService: The service used by the application to manage the environment settings.
/// - errorReporter: The service used by the application to report non-fatal errors.
/// - keychainRepository: The repository used to manages keychain items.
Expand All @@ -341,6 +363,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
clientService: ClientService,
configService: ConfigService,
credentialIdentityStore: CredentialIdentityStore = ASCredentialIdentityStore.shared,
devicePasskeyService: DevicePasskeyService,
environmentService: EnvironmentService,
errorReporter: ErrorReporter,
keychainRepository: KeychainRepository,
Expand All @@ -355,6 +378,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
self.clientService = clientService
self.configService = configService
self.credentialIdentityStore = credentialIdentityStore
self.devicePasskeyService = devicePasskeyService
self.environmentService = environmentService
self.errorReporter = errorReporter
self.keychainRepository = keychainRepository
Expand Down Expand Up @@ -589,7 +613,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng
email: username,
masterPassword: masterPassword
)

Copy link
Contributor

Choose a reason for hiding this comment

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

โ›๏ธ Blank lines shouldn't have spaces in them

// Save the master password hash.
try await saveMasterPasswordHash(password: masterPassword)

Expand Down Expand Up @@ -680,6 +704,11 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng

return false
}

/// Create device passkey with PRF encryption key.
func createDevicePasskey(masterPasswordHash: String) async throws {
try await devicePasskeyService.createDevicePasskey(masterPasswordHash: masterPasswordHash)
}

func loginWithSingleSignOn(code: String, email: String) async throws -> LoginUnlockMethod {
// Get the identity token to log in to Bitwarden.
Expand Down
Loading
Loading