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

Implement WebAuthn authenticator support in DirectAuth #172

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

mikenachbaur-okta
Copy link
Contributor

@mikenachbaur-okta mikenachbaur-okta commented Feb 14, 2024

Adds support for WebAuthn factors within DirectAuth. This also improves upon supplying token parameters, and defining the desired grant type, based on the supplied factor combined with the current status of the user's authentication.

Note: the JSONValue class and tests were taken from okta-idx-swift. An update to that SDK will made to consume JSONValue from AuthFoundation.

@mikenachbaur-okta mikenachbaur-okta force-pushed the OKTA-663117-WebAuthn branch 2 times, most recently from b616af7 to 26d6583 Compare February 15, 2024 17:08
@mikenachbaur-okta
Copy link
Contributor Author

Note: The version of swiftlint updated on Github Actions' runners, so this also includes some changes to address lint warnings.


/// Represent mixed JSON values as instances of `Any`. This is used to expose API response values to Swift native types
/// where Swift enums are not supported.
public enum JSONValue: Equatable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: This class came from the okta-idx-swift repo, which will be updated to consume AuthFoundation for this feature in another PR.

@@ -63,6 +84,8 @@ extension DirectAuthenticationFlow.SecondaryFactor: AuthenticationFactor {
return .otpMFA
case .oob:
return .oobMFA
case .webAuthn, .webAuthnAssertion(_):

Choose a reason for hiding this comment

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

will webauthn in MFA use case be added separately? if so this grant type will probably change to urn:okta:params:oauth:grant-type:mfa-webauthn and that will create a problem with resuming the flow using secondary factor similar to the issue #175.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, according to the spec, the grant-type is the same for both primary auth vs MFA auth. However, you did help me point out one gap, where the list of challenge_types_supported will return urn:okta:params:oauth:grant-type:mfa-webauthn when initiating a WebAuthn challenge as an MFA factor. So I need to make sure that's supported in this code.

}

var grantType: GrantType {
func grantType(currentStatus: DirectAuthenticationFlow.Status?) -> GrantType {
switch self {
case .otp:
return .otpMFA
case .oob:
return .oobMFA

Choose a reason for hiding this comment

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

can you add the check for mfaToken here too to determine the right grant type to fix the issue with push factor number challenge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that as a separate PR, since I didn't want to bloat this one any more than it already has.

// Remove the credential from storage if the access token was revoked
if shouldRemove {
if case .success = result,

Choose a reason for hiding this comment

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

In a user application, we might choose to favor prioritizing removal from the device and opting to fire off a background task for revocation. We wouldn't want any circumstance truly where the user can't "sign out" from a device.

The way this is written, if you remove the token first and then try to revoke it, it will throw an error since the removal portion will fail since it'll already have been removed, correct?

Is it possible to de-conflate the revocation to be a separate function that doesn't attempt removal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@baksha97 If the revoke is unsuccessful, the developer could still call remove(). However, if the revoke operation failed due to a network timeout/offline error, the token will still be valid on the server, and removing it will prevent the app from being able to revoke it in the future.

For example, you could do the following:

do {
  try await credential.revoke()
} catch {
  // Check to see if the error is recoverable
  if shouldRemoveTheToken {
    try credential.remove()
  }
}

}

var grantType: GrantType {
func grantType(currentStatus: DirectAuthenticationFlow.Status?) -> GrantType {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need currentStatus as a function parameter? I don't see that being used in this function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The grantType property/function is a property of the AuthenticationFactor protocol, which is shared by both Primary and Secondary factor types. The SecondaryFactor uses the current status to determine whether or not the grant type should be a primary authentication, or MFA.

For example, in this line the code checks to see if the current status is associated with an MFA Token, which changes which grant type should be used.

@mikenachbaur-okta mikenachbaur-okta merged commit 01bbaa4 into master Feb 22, 2024
7 checks passed
@mikenachbaur-okta mikenachbaur-okta deleted the OKTA-663117-WebAuthn branch February 22, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants