[PM-22811] fix: Implement invalid_grant on 400 response behavior while refreshing token (#1694)

Co-authored-by: Matt Czech <matt@livefront.com>
This commit is contained in:
Federico Maccaroni 2025-06-27 17:32:46 -03:00 committed by GitHub
parent b5f3ab24e7
commit 35a35ee881
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
21 changed files with 316 additions and 38 deletions

View File

@ -40,8 +40,7 @@ final class CrashlyticsErrorReporter: ErrorReporter {
logger.log("Error: \(error)\n\(callStack)")
}
// Don't log networking related errors to Crashlytics.
guard !error.isNetworkingError else { return }
guard !error.isNonLoggableError else { return }
Crashlytics.crashlytics().record(error: error)
}

View File

@ -45,8 +45,7 @@ final class CrashlyticsErrorReporter: ErrorReporter {
logger.log("Error: \(error)\n\(callStack)")
}
// Don't log networking related errors to Crashlytics.
guard !error.isNetworkingError else { return }
guard !error.isNonLoggableError else { return }
Crashlytics.crashlytics().record(error: error)
}

View File

@ -1,14 +1,13 @@
import Foundation
public extension Error {
/// Attempts to determine if the error is a networking related error. This can be useful for
/// deciding if the error should be logged to an external error reporting service where
/// Attempts to determine if the error should not be logged, like a networking related error.
/// This can be useful for deciding if the error should be logged to an external error reporting service where
/// networking or server errors may add noise instead of being actionable errors that need to
/// be fixed in the app.
var isNetworkingError: Bool {
var isNonLoggableError: Bool {
switch self {
case is ResponseValidationError, // Bitwarden Server specific errors.
is ServerError, // Any other non-2XX HTTP errors.
case is NonLoggableError, // Any error marked as `NetworkingError`
is URLError: // URLSession errors.
true
default:

View File

@ -12,7 +12,7 @@ class ErrorNetworkingTests: BitwardenTestCase {
func test_isNetworkingError_other() {
struct NonNetworkingError: Error {}
XCTAssertFalse(NonNetworkingError().isNetworkingError)
XCTAssertFalse(NonNetworkingError().isNonLoggableError)
}
/// `isNetworkingError` returns `true` for `ResponseValidationError`s.
@ -20,7 +20,7 @@ class ErrorNetworkingTests: BitwardenTestCase {
let response = HTTPResponse.failure(statusCode: 500)
let error = ResponseValidationError(response: response)
XCTAssertTrue(error.isNetworkingError)
XCTAssertTrue(error.isNonLoggableError)
}
/// `isNetworkingError` returns `true` for `ServerError`s.
@ -28,13 +28,23 @@ class ErrorNetworkingTests: BitwardenTestCase {
let response = HTTPResponse.failure(statusCode: 400, body: APITestData.bitwardenErrorMessage.data)
let error = try ServerError.error(errorResponse: ErrorResponseModel(response: response))
XCTAssertTrue(error.isNetworkingError)
XCTAssertTrue(error.isNonLoggableError)
}
/// `isNetworkingError` returns `true` for `URLError`s.
func test_isNetworkingError_urlError() throws {
XCTAssertTrue(URLError(.cancelled).isNetworkingError)
XCTAssertTrue(URLError(.networkConnectionLost).isNetworkingError)
XCTAssertTrue(URLError(.timedOut).isNetworkingError)
XCTAssertTrue(URLError(.cancelled).isNonLoggableError)
XCTAssertTrue(URLError(.networkConnectionLost).isNonLoggableError)
XCTAssertTrue(URLError(.timedOut).isNonLoggableError)
}
/// `isNetworkingError` returns `true` for custom `NetworkingError`.
func test_isNetworkingError_networkingError() throws {
XCTAssertTrue(TestNetworkingError.test.isNonLoggableError)
}
}
/// Error to be used as test for `NetworkingError`.
enum TestNetworkingError: NonLoggableError {
case test
}

View File

@ -0,0 +1,2 @@
/// A protocol used to mark errors that shouldn't be logged by log reporters.
public protocol NonLoggableError: Error {}

View File

@ -4,7 +4,7 @@ import Foundation
/// An enumeration of server errors.
///
public enum ServerError: Error, Equatable, CustomNSError {
public enum ServerError: NonLoggableError, Equatable, CustomNSError {
/// A generic error.
///
/// - Parameter errorResponse: The error response returned from the API request.

View File

@ -4,7 +4,7 @@ import Networking
/// An error indicating that the response was invalid and didn't contain a successful HTTP status code.
///
public struct ResponseValidationError: Error, Equatable {
public struct ResponseValidationError: NonLoggableError, Equatable {
// MARK: Properties
/// The received HTTP response.

View File

@ -35,8 +35,7 @@ public final class OSLogErrorReporter: ErrorReporter {
logger.log("Error: \(error as NSError)\n\(callStack)")
}
// Don't crash for networking related errors.
guard !error.isNetworkingError else { return }
guard !error.isNonLoggableError else { return }
// Crash in debug builds to make the error more visible during development.
assertionFailure("Unexpected error: \(error)")

View File

@ -19,6 +19,10 @@ extension APITestData {
)
static let identityTokenNoMasterPassword = loadFromJsonBundle(resource: "IdentityTokenNoMasterPassword")
static let identityTokenRefresh = loadFromJsonBundle(resource: "identityTokenRefresh")
static let identityTokenRefreshInvalidGrantError = loadFromJsonBundle(
resource: "IdentityTokenRefreshInvalidGrantError"
)
static let identityTokenRefreshStubError = loadFromJsonBundle(resource: "IdentityTokenRefreshStubError")
static let identityTokenTrustedDevice = loadFromJsonBundle(resource: "IdentityTokenTrustedDevice")
static let identityTokenTwoFactorError = loadFromJsonBundle(resource: "IdentityTokenTwoFactorFailure")
static let preValidateSingleSignOn = loadFromJsonBundle(resource: "preValidateSingleSignOn")

View File

@ -0,0 +1,3 @@
{
"error": "invalid_grant"
}

View File

@ -0,0 +1,3 @@
{
"error": "stub"
}

View File

@ -1,5 +1,16 @@
import BitwardenKit
import Networking
// MARK: - IdentityTokenRefreshRequestError
/// Errors that can occur when sending an `IdentityTokenRefreshRequest`.
enum IdentityTokenRefreshRequestError: NonLoggableError, Equatable {
/// Not allowed because of invalid grant.
case invalidGrant
}
// MARK: - IdentityTokenRefreshRequest
/// Data model for performing a identity token refresh request.
///
struct IdentityTokenRefreshRequest: Request {
@ -30,4 +41,20 @@ struct IdentityTokenRefreshRequest: Request {
init(refreshToken: String) {
requestModel = IdentityTokenRefreshRequestModel(refreshToken: refreshToken)
}
// MARK: Methods
func validate(_ response: HTTPResponse) throws {
if response.statusCode == 400 {
guard let errorModel = try? IdentityTokenErrorModel.decoder.decode(
IdentityTokenErrorModel.self,
from: response.body
) else { return }
if let error = errorModel.error,
error == IdentityTokenError.invalidGrant {
throw IdentityTokenRefreshRequestError.invalidGrant
}
}
}
}

View File

@ -1,3 +1,5 @@
import Networking
import TestHelpers
import XCTest
@testable import BitwardenShared
@ -47,6 +49,39 @@ class IdentityTokenRefreshRequestTests: BitwardenTestCase {
XCTAssertEqual(subject.path, "/connect/token")
}
/// `validate(_:)` with a valid response does not throw a validation error.
func test_validate_with200() {
let response = HTTPResponse.success(
body: APITestData.identityTokenRefresh.data
)
XCTAssertNoThrow(try subject.validate(response))
}
/// `validate(_:)` with a `400` status code and non invalid grant in the response body
/// doesn't throw an error.
func test_validate_with400DoesntThrow() {
let response = HTTPResponse.failure(
statusCode: 400,
body: APITestData.identityTokenRefreshStubError.data
)
XCTAssertNoThrow(try subject.validate(response))
}
/// `validate(_:)` with a `400` status code and invalid grant in the response body throws a
/// `.invalidGrant` error.
func test_validate_with400InvalidGrantError() {
let response = HTTPResponse.failure(
statusCode: 400,
body: APITestData.identityTokenRefreshInvalidGrantError.data
)
XCTAssertThrowsError(try subject.validate(response)) { error in
XCTAssertEqual(error as? IdentityTokenRefreshRequestError, .invalidGrant)
}
}
/// `query` returns no query parameters.
func test_query() {
XCTAssertTrue(subject.query.isEmpty)

View File

@ -39,6 +39,8 @@ class APIService {
/// Initialize an `APIService` used to make API requests.
///
/// - Parameters:
/// - accountTokenProvider: The `AccountTokenProvider` to use. This is helpful for testing.
/// The default will be built by default.
/// - client: The underlying `HTTPClient` that performs the network request. Defaults
/// to `URLSession.shared`.
/// - environmentService: The service used by the application to retrieve the environment settings.
@ -48,6 +50,7 @@ class APIService {
/// account's tokens.
///
init(
accountTokenProvider: AccountTokenProvider? = nil,
client: HTTPClient = URLSession.shared,
environmentService: EnvironmentService,
flightRecorder: FlightRecorder,
@ -70,21 +73,21 @@ class APIService {
]
)
accountTokenProvider = AccountTokenProvider(
self.accountTokenProvider = accountTokenProvider ?? DefaultAccountTokenProvider(
httpService: httpServiceBuilder.makeService(baseURLGetter: { environmentService.identityURL }),
tokenService: tokenService
)
apiService = httpServiceBuilder.makeService(
baseURLGetter: { environmentService.apiURL },
tokenProvider: accountTokenProvider
tokenProvider: self.accountTokenProvider
)
apiUnauthenticatedService = httpServiceBuilder.makeService(
baseURLGetter: { environmentService.apiURL }
)
eventsService = httpServiceBuilder.makeService(
baseURLGetter: { environmentService.eventsURL },
tokenProvider: accountTokenProvider
tokenProvider: self.accountTokenProvider
)
hibpService = httpServiceBuilder.makeService(
baseURLGetter: { URL(string: "https://api.pwnedpasswords.com")! }
@ -107,4 +110,10 @@ class APIService {
tokenProvider: accountTokenProvider
)
}
/// Sets the account token provider delegate.
/// - Parameter delegate: The delegate to use.
func setAccountTokenProviderDelegate(delegate: AccountTokenProviderDelegate) async {
await accountTokenProvider.setDelegate(delegate: delegate)
}
}

View File

@ -6,17 +6,20 @@ import XCTest
@testable import Networking
class APIServiceTests: BitwardenTestCase {
var accountTokenProvider: MockAccountTokenProvider!
var subject: APIService!
override func setUp() {
super.setUp()
subject = APIService(client: MockHTTPClient())
accountTokenProvider = MockAccountTokenProvider()
subject = APIService(accountTokenProvider: accountTokenProvider, client: MockHTTPClient())
}
override func tearDown() {
super.tearDown()
accountTokenProvider = nil
subject = nil
}
@ -68,4 +71,11 @@ class APIServiceTests: BitwardenTestCase {
)
XCTAssertNil(subject.identityService.tokenProvider)
}
/// `setupAccountTokenProviderDelegate(:)` sets up the delegate in the account token provider.
func test_setupAccountTokenProviderDelegate() async {
await subject.setAccountTokenProviderDelegate(delegate: MockAccountTokenProviderDelegate())
let delegate = await accountTokenProvider.delegate
XCTAssertNotNil(delegate)
}
}

View File

@ -1,11 +1,25 @@
import Networking
// MARK: - AccountTokenProvider
/// A more specific `TokenProvider` protocol to use and ease testing.
protocol AccountTokenProvider: TokenProvider {
/// Sets the delegate to use in this token provider.
/// - Parameter delegate: The delegate to use.
func setDelegate(delegate: AccountTokenProviderDelegate) async
}
// MARK: - DefaultAccountTokenProvider
/// A `TokenProvider` that gets the access token for the current account and can refresh it when
/// necessary.
///
actor AccountTokenProvider: TokenProvider {
actor DefaultAccountTokenProvider: AccountTokenProvider {
// MARK: Properties
/// The delegate to use for specific operations on the token provider.
private weak var accountTokenProviderDelegate: AccountTokenProviderDelegate?
/// The `HTTPService` used to make the API call to refresh the access token.
let httpService: HTTPService
@ -54,6 +68,7 @@ actor AccountTokenProvider: TokenProvider {
let refreshTask = Task {
defer { self.refreshTask = nil }
do {
let refreshToken = try await tokenService.getRefreshToken()
let response = try await httpService.send(
IdentityTokenRefreshRequest(refreshToken: refreshToken)
@ -64,9 +79,26 @@ actor AccountTokenProvider: TokenProvider {
)
return response.accessToken
} catch {
if let accountTokenProviderDelegate {
try await accountTokenProviderDelegate.onRefreshTokenError(error: error)
}
throw error
}
}
self.refreshTask = refreshTask
_ = try await refreshTask.value
}
func setDelegate(delegate: AccountTokenProviderDelegate) async {
accountTokenProviderDelegate = delegate
}
}
/// Delegate to be used by the `AccountTokenProvider`.
protocol AccountTokenProviderDelegate: AnyObject {
/// Callbac to be used when an error is thrown when refreshing the access token.
/// - Parameter error: `Error` thrown.
func onRefreshTokenError(error: Error) async throws
}

View File

@ -8,7 +8,7 @@ class AccountTokenProviderTests: BitwardenTestCase {
// MARK: Properties
var client: MockHTTPClient!
var subject: AccountTokenProvider!
var subject: DefaultAccountTokenProvider!
var tokenService: MockTokenService!
// MARK: Setup & Teardown
@ -19,7 +19,7 @@ class AccountTokenProviderTests: BitwardenTestCase {
client = MockHTTPClient()
tokenService = MockTokenService()
subject = AccountTokenProvider(
subject = DefaultAccountTokenProvider(
httpService: HTTPService(baseURL: URL(string: "https://example.com")!, client: client),
tokenService: tokenService
)
@ -88,4 +88,34 @@ class AccountTokenProviderTests: BitwardenTestCase {
let refreshTask = await subject.refreshTask
XCTAssertNil(refreshTask)
}
/// `refreshToken()` throws trying to refresh the access token
/// and gets handled by the delegate before throwing it again.
func test_refreshToken_handlesErrorInDelegateAndThrows() async throws {
let delegate = MockAccountTokenProviderDelegate()
await subject.setDelegate(delegate: delegate)
tokenService.accessToken = "🔑"
tokenService.refreshToken = "🔒"
client.result = .failure(BitwardenTestError.example)
await assertAsyncThrows(error: BitwardenTestError.example) {
try await subject.refreshToken()
}
XCTAssertTrue(delegate.onRefreshTokenErrorCalled)
}
/// `refreshToken()` throws trying to refresh the access token
/// and gets handled by the delegate before throwing it again.
func test_refreshToken_throwsWithNoDelegate() async throws {
tokenService.accessToken = "🔑"
tokenService.refreshToken = "🔒"
client.result = .failure(BitwardenTestError.example)
await assertAsyncThrows(error: BitwardenTestError.example) {
try await subject.refreshToken()
}
}
}

View File

@ -6,12 +6,14 @@ import Networking
extension APIService {
convenience init(
accountTokenProvider: AccountTokenProvider? = nil,
client: HTTPClient,
environmentService: EnvironmentService = MockEnvironmentService(),
flightRecorder: FlightRecorder = MockFlightRecorder(),
stateService: StateService = MockStateService()
) {
self.init(
accountTokenProvider: accountTokenProvider,
client: client,
environmentService: environmentService,
flightRecorder: flightRecorder,

View File

@ -0,0 +1,36 @@
import BitwardenKit
@testable import BitwardenShared
// MARK: - MockAccountTokenProvider
@MainActor
class MockAccountTokenProvider: AccountTokenProvider {
var delegate: AccountTokenProviderDelegate?
var getTokenResult: Result<String, Error> = .success("ACCESS_TOKEN")
var refreshTokenResult: Result<Void, Error> = .success(())
func getToken() async throws -> String {
try getTokenResult.get()
}
func refreshToken() async throws {
try refreshTokenResult.get()
}
func setDelegate(delegate: AccountTokenProviderDelegate) async {
self.delegate = delegate
}
}
// MARK: - MockAccountTokenProviderDelegate
class MockAccountTokenProviderDelegate: AccountTokenProviderDelegate {
var onRefreshTokenErrorCalled = false
var onRefreshTokenErrorResult: Result<Void, Error> = .success(())
func onRefreshTokenError(error: any Error) async throws {
onRefreshTokenErrorCalled = true
try onRefreshTokenErrorResult.get()
}
}

View File

@ -81,6 +81,10 @@ public class AppProcessor {
self.services.pendingAppIntentActionMediator.setDelegate(self)
self.services.syncService.delegate = self
Task {
await services.apiService.setAccountTokenProviderDelegate(delegate: self)
}
startEventTimer()
UI.initialLanguageCode = services.appSettingsStore.appLocale ?? Bundle.main.preferredLocalizations.first
@ -495,6 +499,18 @@ extension AppProcessor {
return AppRoute.tab(.vault(.vaultItemSelection(totpKeyModel)))
}
/// Logs out the user automatically, if `nil` is passed as `userId` then it will act on the current user.
/// - Parameter userId: The ID of the user to logout, current if `nil`.
private func logOutAutomatically(userId: String? = nil) async {
coordinator?.hideLoadingOverlay()
do {
try await services.authRepository.logout(userId: userId, userInitiated: false)
} catch {
services.errorReporter.log(error: error)
}
await coordinator?.handleEvent(.didLogout(userId: userId, userInitiated: false))
}
/// Starts timer to send organization events regularly
private func startEventTimer() {
sendEventTimer = Timer.scheduledTimer(withTimeInterval: 5 * 60, repeats: true) { _ in
@ -621,9 +637,7 @@ extension AppProcessor: SyncServiceDelegate {
func securityStampChanged(userId: String) async {
// Log the user out if their security stamp changes.
coordinator?.hideLoadingOverlay()
try? await services.authRepository.logout(userId: userId, userInitiated: false)
await coordinator?.handleEvent(.didLogout(userId: userId, userInitiated: false))
await logOutAutomatically(userId: userId)
}
func setMasterPassword(orgIdentifier: String) async {
@ -652,6 +666,16 @@ public extension AppProcessor {
}
}
// MARK: - AccountTokenProviderDelegate
extension AppProcessor: AccountTokenProviderDelegate {
func onRefreshTokenError(error: any Error) async throws {
if case IdentityTokenRefreshRequestError.invalidGrant = error {
await logOutAutomatically()
}
}
}
// MARK: - AutofillCredentialServiceDelegate
extension AppProcessor: AutofillCredentialServiceDelegate {

View File

@ -1372,4 +1372,59 @@ class AppProcessorTests: BitwardenTestCase { // swiftlint:disable:this type_body
XCTAssertFalse(coordinator.isLoadingOverlayShowing)
XCTAssertEqual(coordinator.events, [.didLogout(userId: "1", userInitiated: false)])
}
/// `securityStampChanged(userId:)` throws logging the user out which is logged and notifies the coordinator.
@MainActor
func test_securityStampChanged_throwsLogging() async {
coordinator.isLoadingOverlayShowing = true
authRepository.logoutResult = .failure(BitwardenTestError.example)
await subject.securityStampChanged(userId: "1")
XCTAssertTrue(authRepository.logoutCalled)
XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example])
XCTAssertFalse(coordinator.isLoadingOverlayShowing)
XCTAssertEqual(coordinator.events, [.didLogout(userId: "1", userInitiated: false)])
}
/// `onRefreshTokenError(error:)` logs the user out and notifies the coordinator when error is `.invalidGrant`.
@MainActor
func test_onRefreshTokenError_logOutInvalidGrant() async throws {
coordinator.isLoadingOverlayShowing = true
try await subject.onRefreshTokenError(error: IdentityTokenRefreshRequestError.invalidGrant)
XCTAssertTrue(authRepository.logoutCalled)
XCTAssertEqual(authRepository.logoutUserId, nil)
XCTAssertFalse(authRepository.logoutUserInitiated)
XCTAssertFalse(coordinator.isLoadingOverlayShowing)
XCTAssertEqual(coordinator.events, [.didLogout(userId: nil, userInitiated: false)])
}
/// `onRefreshTokenError(error:)` throws logging the user out which is logged and notifies the coordinator
/// when error is `.invalidGrant`.
@MainActor
func test_onRefreshTokenError_logOutInvalidGrantThrowsLogging() async throws {
coordinator.isLoadingOverlayShowing = true
authRepository.logoutResult = .failure(BitwardenTestError.example)
try await subject.onRefreshTokenError(error: IdentityTokenRefreshRequestError.invalidGrant)
XCTAssertTrue(authRepository.logoutCalled)
XCTAssertEqual(errorReporter.errors as? [BitwardenTestError], [.example])
XCTAssertFalse(coordinator.isLoadingOverlayShowing)
XCTAssertEqual(coordinator.events, [.didLogout(userId: nil, userInitiated: false)])
}
/// `onRefreshTokenError(error:)` doesn't perform log out when error is not `.invalidGrant`.
@MainActor
func test_onRefreshTokenError_notInvalidGrant() async throws {
coordinator.isLoadingOverlayShowing = true
try await subject.onRefreshTokenError(error: BitwardenTestError.example)
XCTAssertFalse(authRepository.logoutCalled)
XCTAssertTrue(coordinator.isLoadingOverlayShowing)
XCTAssertTrue(coordinator.events.isEmpty)
}
}