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

Login with Passkeys!! #1599

Open
wants to merge 21 commits into
base: trunk
Choose a base branch
from
Open

Conversation

charliescheer
Copy link
Contributor

@charliescheer charliescheer commented Jun 12, 2024

Fix

To improve the security of Simplenote accounts we are trying to move away from email/password authentication as it is insecure at best. One of the things we are implementing is Passkeys. This will allow users to create and store passkeys on their apple device and when they go to log into their account rather than using an email and password they only need to put in their email and then use the local biometrics to receive a passkey and then authenticate into Simplenote.

This has a bunch of security benefits and should also help users keep from losing their passwords.

Sign up for passkeys:

RPReplay_Final1718384460.mp4

Login with passkeys:

RPReplay_Final1718384268.mp4

Test

  1. Clean install Simplenote on device (can't access biometrics on sim) Tap on login and choose to login with email and password
  2. Once logged in go to the menu -> Settings and in the account section you will see a "Add Passkey Authentication" button. Press that. After a few seconds you will receive a notice from device asking if you want to create a passkey. Confirm and wait a few seconds. (NOTE: Currently there is no spinner or success/failure indication, that will be in a future PR)
  3. Log out of Simplenote
  4. Tap on the Login button and choose Login with Passkeys
  5. Enter your email and tap on the Login with Passkeys Button
  6. after a few seconds you will be prompted by device if you want to login with passkeys, confirm
  • Confirm you are able to log into your Simplenote account.

Review

(Required) Add instructions for reviewers. For example:

Only one developer is required to review these changes, but anyone can perform the review.

Release

(Required) Add a concise statement to RELEASE-NOTES.txt if the changes should be included in release notes. Include details about updating the notes in this section. For example:

RELEASE-NOTES.txt was updated in 0c4129 with:

Added Passkey authentication support

@dangermattic
Copy link
Collaborator

dangermattic commented Jun 12, 2024

2 Warnings
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 12, 2024

You can test the changes in simplenote-ios from this Pull Request by:

  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr1599-7e524e0-019017ab-e275-4c9a-8184-574412bb1e22 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@charliescheer charliescheer changed the title WIP: passkey implementation Login with Passkeysasskey implementation Jun 14, 2024
@charliescheer charliescheer changed the title Login with Passkeysasskey implementation Login with Passkeys!! Jun 14, 2024
@charliescheer charliescheer self-assigned this Jun 14, 2024
@charliescheer charliescheer added the [feature] login Anything relating to login. label Jun 14, 2024
@charliescheer charliescheer marked this pull request as ready for review June 14, 2024 16:54
static let passkeyCredentialCreationURL = currentPasskeyBaseURL.appendingPathComponent("/api2/login")
static let passkeyRegistrationURL = currentPasskeyBaseURL.appendingPathComponent("/auth/add-credential")
static let passkeyAuthChallengeURL = currentPasskeyBaseURL.appendingPathComponent("/auth/prepare-auth-challenge")
static let verifyPasskeyAuthChallengeURL = currentPasskeyBaseURL.appendingPathComponent("/auth/verify-login-credential")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a / onto the end of all of these endpoints? It will save app engine from doing a redirect.


body += Data("--\(boundary)--\r\n".utf8)

return body
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than converting every line into Data, adding them all up and returning, we can build the string and convert just once, in the last statement

Copy link
Contributor

@jleandroperez jleandroperez left a comment

Choose a reason for hiding this comment

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

Sending you a few notes, looking great Charlie!!

//TODO: Handle errors
//TODO: Handle email not valid
let passkeyAuthenticator = SPAppDelegate.shared().passkeyAuthenticator
try? await passkeyAuthenticator.attemptPasskeyAuth(for: email, in: self)
Copy link
Contributor

Choose a reason for hiding this comment

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

SPAuthViewController has an instance of SPAuthHandler, which, in turn, has a reference to SPAuthenticator.

No need to use a singleton for this one! 😄

@@ -627,6 +643,7 @@ struct AuthenticationMode {
let secondaryActionText: String?
let secondaryActionAttributedText: NSAttributedString?
let isPasswordHidden: Bool
let isLogin: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This property appears not to be in use anywhere, perhaps it's a leftover?

secondaryActionText: AuthenticationStrings.loginSecondaryAction,
secondaryActionAttributedText: nil,
isPasswordHidden: true,
isLogin: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

userInfo: [NSLocalizedDescriptionKey: "Can't decode base64 string"])
}
return data
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Caaaan we add a comment mentioning why this was required (rather than the standard initializer?)

Thank youuu!!

}

private func performPasskeyAuthentication(with response: PasskeyAuthResponse) {
let json = try! JSONEncoder().encode(response)
Copy link
Contributor

Choose a reason for hiding this comment

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

It should never crash, but perhaps we should avoid the force unwrap on this one

case let credential as ASAuthorizationPlatformPublicKeyCredentialAssertion:
let response = PasskeyAuthResponse(from: credential)

performPasskeyAuthentication(with: response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nipticky: Indentation is a bit off in these few lines

private let response: PasskeyRegistrationResponse.Response

init?(from credentialRegistration: ASAuthorizationPlatformPublicKeyCredentialRegistration) {
guard let email = SPAppDelegate.shared().simperium.user?.email,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should get the email via parameter (rather than coupling the initializer with the AppDelegate)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[feature] login Anything relating to login.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants