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

BIT-1003: Generate plus addressed emails #129

Merged
merged 2 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions BitwardenShared/Core/Platform/Utilities/Constants.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ enum Constants {
/// The client type corresponding to the app.
static let clientType: ClientType = "mobile"

/// The default generated username if there isn't enough information to generate a username.
static let defaultGeneratedUsername = "-"

/// The URL for the web vault if the user account doesn't have one specified.
static let defaultWebVaultHost = "bitwarden.com"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ protocol GeneratorRepository: AnyObject {
/// - Returns: The generated password.
///
func generatePassword(settings: PasswordGeneratorRequest) async throws -> String

/// Generates a plus-addressed email based on the username generation options.
///
/// - Returns: The generated plus-addressed email.
///
func generateUsernamePlusAddressedEmail(email: String) async throws -> String
}

// MARK: - DefaultGeneratorRepository
Expand All @@ -28,14 +34,23 @@ class DefaultGeneratorRepository {
/// The client used for generating passwords and passphrases.
let clientGenerators: ClientGeneratorsProtocol

/// The service used to handle cryptographic operations.
let cryptoService: CryptoService

// MARK: Initialization

/// Initialize a `DefaultGeneratorRepository`
///
/// - Parameter clientGenerators: The client used for generating passwords and passphrases.
/// - Parameters:
/// - clientGenerators: The client used for generating passwords and passphrases.
/// - cryptoService: The service used to handle cryptographic operations.
///
init(clientGenerators: ClientGeneratorsProtocol) {
init(
clientGenerators: ClientGeneratorsProtocol,
cryptoService: CryptoService = DefaultCryptoService(randomNumberGenerator: SecureRandomNumberGenerator())
) {
self.clientGenerators = clientGenerators
self.cryptoService = cryptoService
}
}

Expand All @@ -49,4 +64,23 @@ extension DefaultGeneratorRepository: GeneratorRepository {
func generatePassword(settings: PasswordGeneratorRequest) async throws -> String {
try await clientGenerators.password(settings: settings)
}

func generateUsernamePlusAddressedEmail(email: String) async throws -> String {
fedemkr marked this conversation as resolved.
Show resolved Hide resolved
guard email.trimmingCharacters(in: .whitespacesAndNewlines).count >= 3 else {
return Constants.defaultGeneratedUsername
}

guard let atIndex = email.firstIndex(of: "@"),
// Ensure '@' symbol isn't the first or last character.
atIndex > email.startIndex,
atIndex < email.index(before: email.endIndex) else {
return email
}
Comment on lines +73 to +78
Copy link
Member

Choose a reason for hiding this comment

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

๐Ÿค” What do you think of moving this to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you proposing replacing this entire guard with the isValidEmail check? I think these do slightly different, yet similar things. But if we're trying to match the Xamarin app which has been driving our requirements, the isValidEmail only checks that the string contains an @ symbol and doesn't check that it isn't the first or last character.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, for matching purposes we can leave it like this and refactor in the future. Cause IMO wherever isValidEmail is used, when checking if it has a @ it shouldn't be neither the start nor the end of the string so if we'd replace the current isValidEmail logic it'd be an improvement. However you're right and let's leave it like this so it's equal to the Xamarin app.


let randomString = try cryptoService.randomString(length: 8)

var email = email
email.insert(contentsOf: "+\(randomString)", at: atIndex)
return email
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class GeneratorRepositoryTests: BitwardenTestCase {
// MARK: Properties

var clientGenerators: MockClientGenerators!
var cryptoService: MockCryptoService!
var subject: GeneratorRepository!

// MARK: Setup & Teardown
Expand All @@ -15,14 +16,19 @@ class GeneratorRepositoryTests: BitwardenTestCase {
super.setUp()

clientGenerators = MockClientGenerators()
cryptoService = MockCryptoService()

subject = DefaultGeneratorRepository(clientGenerators: clientGenerators)
subject = DefaultGeneratorRepository(
clientGenerators: clientGenerators,
cryptoService: cryptoService
)
}

override func tearDown() {
super.tearDown()

clientGenerators = nil
cryptoService = nil
subject = nil
}

Expand Down Expand Up @@ -103,4 +109,59 @@ class GeneratorRepositoryTests: BitwardenTestCase {
)
}
}

/// `generateUsernamePlusAddressedEmail` returns the generated plus addressed email.
func test_generateUsernamePlusAddressedEmail() async throws {
var email = try await subject.generateUsernamePlusAddressedEmail(email: "[email protected]")
XCTAssertEqual(email, "[email protected]")
XCTAssertEqual(cryptoService.randomStringLength, 8)

email = try await subject.generateUsernamePlusAddressedEmail(email: "user@[email protected]")
XCTAssertEqual(email, "user+ku5eoyi3@[email protected]")

cryptoService.randomStringResult = .success("abcd0123")
email = try await subject.generateUsernamePlusAddressedEmail(email: "[email protected]")
XCTAssertEqual(email, "[email protected]")
}

/// `generateUsernamePlusAddressedEmail` returns "-" if there aren't enough characters entered.
func test_generateUsernamePlusAddressedEmail_tooFewCharacters() async throws {
var email = try await subject.generateUsernamePlusAddressedEmail(email: "")
XCTAssertEqual(email, "-")

email = try await subject.generateUsernamePlusAddressedEmail(email: "ab")
XCTAssertEqual(email, "-")
}

/// `generateUsernamePlusAddressedEmail` returns the email with no changes if it doesn't contain
/// an '@' symbol.
func test_generateUsernamePlusAddressedEmail_missingAt() async throws {
var email = try await subject.generateUsernamePlusAddressedEmail(email: "abc")
XCTAssertEqual(email, "abc")

email = try await subject.generateUsernamePlusAddressedEmail(email: "user")
XCTAssertEqual(email, "user")

email = try await subject.generateUsernamePlusAddressedEmail(email: "bitwarden.com")
XCTAssertEqual(email, "bitwarden.com")
}

/// `generateUsernamePlusAddressedEmail` returns the email with no changes if the '@' symbol is
/// the first or last character.
func test_generateUsernamePlusAddressedEmail_atFirstOrLast() async throws {
var email = try await subject.generateUsernamePlusAddressedEmail(email: "@bitwarden.com")
XCTAssertEqual(email, "@bitwarden.com")

email = try await subject.generateUsernamePlusAddressedEmail(email: "user@")
XCTAssertEqual(email, "user@")
}

/// `generateUsernamePlusAddressedEmail` throws an error if generating a username fails.
func test_generateUsernamePlusAddressedEmail_error() async {
cryptoService.randomStringResult = .failure(CryptoServiceError.randomNumberGenerationFailed(-1))

await assertAsyncThrows(error: CryptoServiceError.randomNumberGenerationFailed(-1)) {
_ = try await subject.generateUsernamePlusAddressedEmail(email: "[email protected]")
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ class MockGeneratorRepository: GeneratorRepository {
var passwordGeneratorRequest: PasswordGeneratorRequest?
var passwordResult: Result<String, Error> = .success("PASSWORD")

var usernamePlusAddressEmail: String?
var usernamePlusAddressEmailResult: Result<String, Error> = .success("[email protected]")

func generatePassphrase(settings: PassphraseGeneratorRequest) async throws -> String {
passphraseGeneratorRequest = settings
return try passphraseResult.get()
Expand All @@ -18,4 +21,9 @@ class MockGeneratorRepository: GeneratorRepository {
passwordGeneratorRequest = settings
return try passwordResult.get()
}

func generateUsernamePlusAddressedEmail(email: String) async throws -> String {
usernamePlusAddressEmail = email
return try usernamePlusAddressEmailResult.get()
}
}
81 changes: 81 additions & 0 deletions BitwardenShared/Core/Tools/Services/CryptoService.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import Foundation

// MARK: - CryptoServiceError

/// Errors that are thrown from `CryptoService`.
///
enum CryptoServiceError: Error, Equatable {
/// Generating a random number failed.
case randomNumberGenerationFailed(OSStatus)
}

// MARK: - CryptoService

/// A protocol for a service that handles cryptographic operations.
///
protocol CryptoService: AnyObject {
/// Returns a randomly generated string of a specified length.
///
/// - Parameter length: The length of the generated string.
/// - Returns: The randomly generated string.
///
func randomString(length: Int) throws -> String
}

// MARK: - DefaultCryptoService

/// The default implementation of a `CryptoService` which handles cryptographic operations.
///
class DefaultCryptoService: CryptoService {
// MARK: Properties

/// The generator used for generating random numbers.
let randomNumberGenerator: RandomNumberGenerator

/// A string containing the possible characters used for generating a random string.
let randomStringCharacters = "abcdefghijklmnopqrstuvwxyz1234567890"

// MARK: Initialization

/// Initialize a `DefaultCryptoService`.
///
/// - Parameter randomNumberGenerator: The generator used for generating random numbers.
///
init(randomNumberGenerator: RandomNumberGenerator) {
self.randomNumberGenerator = randomNumberGenerator
}

// MARK: CryptoService

func randomString(length: Int) throws -> String {
let characters = try (0 ..< length).map { _ in
let randomOffset = try randomNumber(min: 0, max: randomStringCharacters.count - 1)
let randomIndex = randomStringCharacters.index(randomStringCharacters.startIndex, offsetBy: randomOffset)
return randomStringCharacters[randomIndex]
}
return String(characters)
}

// MARK: Private

/// Generates a random number within a range.
///
/// - Parameters:
/// - min: The minimum number in the range of possible random numbers.
/// - max: The maximum number in the range of possible random numbers.
/// - Returns: A random number within the range.
///
func randomNumber(min: Int, max: Int) throws -> Int {
// Make max inclusive.
let max = max + 1

let diff = UInt(max - min)
let upperBound = UInt.max / diff * diff
var randomNumber: UInt
repeat {
randomNumber = try randomNumberGenerator.randomNumber()
} while randomNumber >= upperBound

return min + Int(randomNumber % diff)
}
}
61 changes: 61 additions & 0 deletions BitwardenShared/Core/Tools/Services/CryptoServiceTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import XCTest

@testable import BitwardenShared

class CryptoServiceTests: BitwardenTestCase {
// MARK: Properties

var randomNumberGenerator: MockRandomNumberGenerator!
var subject: DefaultCryptoService!

// MARK: Setup & Teardown

override func setUp() {
super.setUp()

randomNumberGenerator = MockRandomNumberGenerator()

subject = DefaultCryptoService(randomNumberGenerator: randomNumberGenerator)
}

override func tearDown() {
super.tearDown()

randomNumberGenerator = nil
subject = nil
}

// MARK: Tests

/// `randomString(length:)` returns a random string of the specified length.
func test_randomString() throws {
randomNumberGenerator.randomNumberResults = [0]
try XCTAssertEqual(subject.randomString(length: 1), "a")

randomNumberGenerator.randomNumberResults = [18]
try XCTAssertEqual(subject.randomString(length: 1), "s")

randomNumberGenerator.randomNumberResults = [35]
try XCTAssertEqual(subject.randomString(length: 1), "0")

randomNumberGenerator.randomNumberResults = [36]
try XCTAssertEqual(subject.randomString(length: 1), "a")

randomNumberGenerator.randomNumberResults = [UInt.max, UInt.max - 36]
try XCTAssertEqual(subject.randomString(length: 1), "p")

randomNumberGenerator.randomNumberResults = [10, 20, 30, 40, 50, 60, 80, 100]
try XCTAssertEqual(subject.randomString(length: 8), "ku5eoyi3")
}
}

class MockRandomNumberGenerator: RandomNumberGenerator {
var randomNumberResults = [UInt]()

func randomNumber() throws -> UInt {
guard !randomNumberResults.isEmpty else {
throw CryptoServiceError.randomNumberGenerationFailed(-1)
}
return randomNumberResults.removeFirst()
}
}
32 changes: 32 additions & 0 deletions BitwardenShared/Core/Tools/Services/RandomNumberGenerator.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import Foundation
import Security

// MARK: - RandomNumberGenerator

/// A protocol for an object that can generate random numbers.
///
protocol RandomNumberGenerator {
/// Generates a random number.
///
/// - Returns: The randomly generated number.
///
func randomNumber() throws -> UInt
}

// MARK: - SecureRandomNumberGenerator

/// An `RandomNumberGenerator` instance which generates secure random numbers.
///
class SecureRandomNumberGenerator: RandomNumberGenerator {
func randomNumber() throws -> UInt {
let count = MemoryLayout<UInt>.size
var bytes = [Int8](repeating: 0, count: count)

let status = SecRandomCopyBytes(kSecRandomDefault, count, &bytes)
guard status == errSecSuccess else { throw CryptoServiceError.randomNumberGenerationFailed(status) }

return bytes.withUnsafeBytes { pointer in
pointer.load(as: UInt.self)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
@testable import BitwardenShared

class MockCryptoService: CryptoService {
var randomStringLength: Int?
var randomStringResult: Result<String, Error> = .success("ku5eoyi3")

func randomString(length: Int) throws -> String {
randomStringLength = length
return try randomStringResult.get()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ enum GeneratorAction: Equatable {
/// A stepper field value was changed.
case stepperValueChanged(field: StepperField<GeneratorState>, value: Int)

/// A text field was focused or lost focus.
case textFieldFocusChanged(keyPath: KeyPath<GeneratorState, String>?)

/// A text field value was changed.
case textValueChanged(field: FormTextField<GeneratorState>, value: String)

Expand All @@ -44,6 +47,7 @@ extension GeneratorAction {
.refreshGeneratedValue,
.sliderValueChanged,
.stepperValueChanged,
.textFieldFocusChanged,
.textValueChanged,
.toggleValueChanged,
.usernameGeneratorTypeChanged:
Expand Down
Loading
Loading