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

Error Handling Upgrades #1200

Merged
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
4 changes: 4 additions & 0 deletions Simplenote.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
B51AFE6E25D30A1800A196DF /* SearchField.swift in Sources */ = {isa = PBXBuildFile; fileRef = B51AFE6C25D30A1800A196DF /* SearchField.swift */; };
B51AFE7725D36CDD00A196DF /* NSFont+Simplenote.swift in Sources */ = {isa = PBXBuildFile; fileRef = B51AFE7525D36CDD00A196DF /* NSFont+Simplenote.swift */; };
B51D44582C52AB2200F296A7 /* SimplenoteEndpoints in Frameworks */ = {isa = PBXBuildFile; productRef = B51D44572C52AB2200F296A7 /* SimplenoteEndpoints */; };
B51D44672C52F5AE00F296A7 /* AuthenticationError.swift in Sources */ = {isa = PBXBuildFile; fileRef = B51D44662C52F5AE00F296A7 /* AuthenticationError.swift */; };
B51D85F525A8B392005F08CE /* NoteListPrefixFormatter.swift in Sources */ = {isa = PBXBuildFile; fileRef = B51D85F325A8B392005F08CE /* NoteListPrefixFormatter.swift */; };
B51E9FE222E615FA004F16B4 /* SPExporter.swift in Sources */ = {isa = PBXBuildFile; fileRef = B51E9FE022E615FA004F16B4 /* SPExporter.swift */; };
B51E9FE622E644A0004F16B4 /* NSObject+Helpers.swift in Sources */ = {isa = PBXBuildFile; fileRef = B51E9FE422E644A0004F16B4 /* NSObject+Helpers.swift */; };
Expand Down Expand Up @@ -510,6 +511,7 @@
B518D37D2507C356006EA7F8 /* StringSimplenoteTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StringSimplenoteTests.swift; sourceTree = "<group>"; };
B51AFE6C25D30A1800A196DF /* SearchField.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchField.swift; sourceTree = "<group>"; };
B51AFE7525D36CDD00A196DF /* NSFont+Simplenote.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "NSFont+Simplenote.swift"; sourceTree = "<group>"; };
B51D44662C52F5AE00F296A7 /* AuthenticationError.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = AuthenticationError.swift; sourceTree = "<group>"; };
B51D85F325A8B392005F08CE /* NoteListPrefixFormatter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NoteListPrefixFormatter.swift; sourceTree = "<group>"; };
B51E9FE022E615FA004F16B4 /* SPExporter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SPExporter.swift; sourceTree = "<group>"; };
B51E9FE422E644A0004F16B4 /* NSObject+Helpers.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "NSObject+Helpers.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -1356,6 +1358,7 @@
children = (
B5F5415325F0137100CAF52C /* MagicLinkAuthenticator.swift */,
B587D7E92C221575006645CF /* SimperiumAuthenticatorProtocol.swift */,
B51D44662C52F5AE00F296A7 /* AuthenticationError.swift */,
);
name = Authentication;
sourceTree = "<group>";
Expand Down Expand Up @@ -2196,6 +2199,7 @@
B5DD0F922476309000C8DD41 /* NoteTableCellView.swift in Sources */,
B503FF4924848D0B00066059 /* TagAttachmentCell.swift in Sources */,
466FFED417CC10A800399652 /* SPTableView.m in Sources */,
B51D44672C52F5AE00F296A7 /* AuthenticationError.swift in Sources */,
375D293921E033D1007AB25A /* escape.c in Sources */,
BA0B43CA26F2FCFC00B44A8C /* PreferencesViewController.swift in Sources */,
B53BF19D24ABDE7C00938C34 /* DateFormatter+Simplenote.swift in Sources */,
Expand Down
50 changes: 45 additions & 5 deletions Simplenote/AuthViewController+Swift.swift
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ extension AuthViewController {
case .success:
self.presentSignupVerification(email: email)
case .failure(let result):
self.showAuthenticationError(forCode: result.statusCode, responseString: nil)
self.showAuthenticationError(forCode: result.statusCode, responseString: result.response)
}

self.stopActionAnimation()
Expand Down Expand Up @@ -292,8 +292,9 @@ extension AuthViewController {

pushCodeLoginView()
} catch {
let statusCode = (error as? RemoteError)?.statusCode ?? .zero
self.showAuthenticationError(forCode: statusCode, responseString: nil)
// TODO: Once Xcode 16 goes GM, *please* wire Typed Errors here? (it'll always be a RemoteError instance)
let remoteError = error as? RemoteError
self.showAuthenticationError(forCode: remoteError?.statusCode ?? .zero, responseString: remoteError?.response)
}
}

Expand All @@ -319,8 +320,9 @@ extension AuthViewController {
let confirmation = try await remote.requestLoginConfirmation(email: username, authCode: code.uppercased())
authenticator.authenticate(withUsername: confirmation.username, token: confirmation.syncToken)
} catch {
let statusCode = (error as? RemoteError)?.statusCode ?? .zero
self.showAuthenticationError(forCode: statusCode, responseString: nil)
// TODO: Once Xcode 16 goes GM, *please* wire Typed Errors here? (it'll always be a RemoteError instance)
let remoteError = error as? RemoteError
self.showAuthenticationError(forCode: remoteError?.statusCode ?? .zero, responseString: remoteError?.response)
}
}

Expand Down Expand Up @@ -421,6 +423,44 @@ extension AuthViewController {
// MARK: - Login Error Handling
//
extension AuthViewController {

@objc(showAuthenticationError:)
func showAuthenticationError(_ error: String) {
errorField.stringValue = error
}

@objc(showAuthenticationErrorForCode:responseString:)
func showAuthenticationError(statusCode: Int, responseString: String?) {
let error = AuthenticationError(statusCode: statusCode, response: responseString, error: nil)
switch error {
case .compromisedPassword:
presentPasswordCompromisedAlert()

case .invalidCode:
let message = NSLocalizedString("The code you've entered is invalid.", comment: "Login po sCode Invalid Error")
showAuthenticationError(message)

case .loginBadCredentials:
let message = NSLocalizedString("Bad email or password", comment: "Error for authorization failure")
showAuthenticationError(message)

case .requestNotFound:
let message = NSLocalizedString("The authentication code you've requested has expired. Please request a new one", comment: "Login Code no longer exists")
showAuthenticationError(message)

case .tooManyAttempts:
let message = NSLocalizedString("Too many log in attempts. Try again later.", comment: "Error for too many login attempts")
showAuthenticationError(message)

case .unverifiedEmail:
presentUnverifiedEmailAlert()

default:
let message = NSLocalizedString("We're having problems. Please try again soon.", comment: "Generic error")
showAuthenticationError(message)
}
}

@objc
func showCompromisedPasswordAlert(for window: NSWindow, completion: @escaping (NSApplication.ModalResponse) -> Void) {
let alert = NSAlert()
Expand Down
2 changes: 2 additions & 0 deletions Simplenote/AuthViewController.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
- (void)setInterfaceEnabled:(BOOL)enabled;

- (void)presentPasswordResetAlert;
- (void)presentPasswordCompromisedAlert;
- (void)presentUnverifiedEmailAlert;
- (void)showAuthenticationErrorForCode:(NSInteger)responseCode responseString:(NSString *)responseString;

@end
43 changes: 0 additions & 43 deletions Simplenote/AuthViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -284,49 +284,6 @@ - (BOOL)validateCode {
[self validateCodeInput];
}

- (void)showAuthenticationError:(NSString *)errorMessage {
[self.errorField setStringValue:errorMessage];
}

- (void)showAuthenticationErrorForCode:(NSInteger)responseCode responseString:(NSString *)responseString {
switch (responseCode) {
case 409:
[self showAuthenticationError:NSLocalizedString(@"That email is already being used", @"Error when address is in use")];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For security reasons, statusCode 409 has been phased out, few years ago

[self.view.window makeFirstResponder:self.usernameField];
break;
case 401:
if ([self isPasswordCompromisedResponse:responseString]) {
[self presentPasswordCompromisedAlert];
} else {
[self showAuthenticationError:NSLocalizedString(@"Bad email or password", @"Error for bad email or password")];
}
break;
case 403:
if ([self isRequiresVerificationdResponse:responseString]) {
[self presentUnverifiedEmailAlert];
} else {
[self showAuthenticationError:NSLocalizedString(@"Authorization failed", @"Error for authorization failure")];
}
break;
case 429:
[self showAuthenticationError:NSLocalizedString(@"Too many log in attempts. Try again later.", @"Error for too many login attempts")];
break;
default:
[self showAuthenticationError:NSLocalizedString(@"We're having problems. Please try again soon.", @"Generic error")];
break;
}
}

- (BOOL)isPasswordCompromisedResponse:(NSString *)responseString
{
return ([responseString isEqual:@"compromised password"]);
}

- (BOOL)isRequiresVerificationdResponse:(NSString *)responseString
{
return ([responseString isEqual:@"verification required"]);
}

-(void)presentPasswordCompromisedAlert
{
__weak typeof(self) weakSelf = self;
Expand Down
54 changes: 54 additions & 0 deletions Simplenote/AuthenticationError.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import Foundation


// MARK: - AuthenticationError
//
public enum AuthenticationError: Error {
case compromisedPassword
case invalidCode
case loginBadCredentials
case network
case requestNotFound
case tooManyAttempts
case unverifiedEmail
case unknown(statusCode: Int, response: String?, error: Error?)
}


// MARK: - Initializers
//
extension AuthenticationError {

/// Returns the AuthenticationError for a given Login statusCode + Response
///
public init(statusCode: Int, response: String?, error: Error?) {
switch statusCode {
case .zero:
self = .network
case 400 where response == ErrorResponse.requestNotFound:
self = .requestNotFound
case 400 where response == ErrorResponse.invalidCode:
self = .invalidCode
case 401 where response == ErrorResponse.compromisedPassword:
self = .compromisedPassword
case 401:
self = .loginBadCredentials
case 403 where response == ErrorResponse.requiresVerification:
self = .unverifiedEmail
case 429:
self = .tooManyAttempts
default:
self = .unknown(statusCode: statusCode, response: response, error: error)
}
}
}


// MARK: - Error Responses
//
private struct ErrorResponse {
static let compromisedPassword = "compromised password"
static let requiresVerification = "verification required"
static let requestNotFound = "request-not-found"
static let invalidCode = "invalid-code"
}
3 changes: 2 additions & 1 deletion Simplenote/AuthenticationMode.swift
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ extension AuthenticationMode {
selector: #selector(AuthViewController.pressedLoginWithMagicLink),
text: MagicLinkStrings.primaryAction),
AuthenticationActionDescriptor(name: .tertiary,
selector: #selector(AuthViewController.wordpressSSOAction), text: LoginStrings.wordpressAction)
selector: #selector(AuthViewController.wordpressSSOAction),
text: LoginStrings.wordpressAction)
],
primaryActionAnimationText: MagicLinkStrings.primaryAnimationText)
}
Expand Down
Loading