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 #8393

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
cd974e6
add WebAuthn credentials to database schema
robert-oleynik Feb 11, 2025
47f52ac
setup WebAuthnCredentials DAO
robert-oleynik Feb 11, 2025
c522883
Implement WebAuthn backend
robert-oleynik Feb 12, 2025
8938d9e
add frontend webauthn support lib
Feb 12, 2025
f5130f6
use proper file extension in some files
Feb 12, 2025
09afa8a
WIP: Add passkey management page
Feb 12, 2025
86260d8
WIP: add some frontend part for passkey registration
Feb 13, 2025
1fe36d9
setup tls proxy
robert-oleynik Feb 18, 2025
391ec47
fix webauthn registration start
robert-oleynik Feb 18, 2025
aba3d99
fix webauthn registration finalize
robert-oleynik Feb 18, 2025
4f03669
fix webauthn auth start
robert-oleynik Feb 18, 2025
b9014a0
fix key id and authentication process
robert-oleynik Feb 18, 2025
3447565
fix frontend redirect
robert-oleynik Feb 18, 2025
3cf61e6
restyle login form
robert-oleynik Feb 20, 2025
dcff035
move user key id to separate datbase field
robert-oleynik Feb 20, 2025
898abda
fix authentication failures
robert-oleynik Feb 20, 2025
f68ab95
display and remove webauthn keys
robert-oleynik Feb 20, 2025
7ba0238
wrap blocking calls
robert-oleynik Feb 20, 2025
dc53bd0
increment schema version
robert-oleynik Feb 20, 2025
dcf027d
fix schema versioning
robert-oleynik Feb 20, 2025
a42a10d
fix schema field typo
robert-oleynik Feb 20, 2025
636de42
add reversion for database evolution
robert-oleynik Feb 20, 2025
5069e9f
fix compiler errors
robert-oleynik Feb 20, 2025
ff9fb7b
fix future box handling
robert-oleynik Feb 20, 2025
23a9591
fix frontend lints
robert-oleynik Feb 20, 2025
7592b13
fix api usage
robert-oleynik Feb 20, 2025
96cf3d6
add trailing ;
robert-oleynik Feb 20, 2025
ab95204
apply format to backend
robert-oleynik Feb 20, 2025
bce5a1d
fix uri
robert-oleynik Feb 20, 2025
e28d477
fix typecheck errors
robert-oleynik Feb 20, 2025
6838401
fixed frontend
robert-oleynik Feb 20, 2025
ce831fb
read origin from configuration
robert-oleynik Feb 25, 2025
dbccd1f
apply format
robert-oleynik Feb 25, 2025
e652f78
fix future exception handling
robert-oleynik Feb 25, 2025
3bdd978
rename PassKeys to Passkeys and add missing await
robert-oleynik Feb 25, 2025
f7734e4
merge manage passkeys and change password view
robert-oleynik Feb 26, 2025
c827ee5
fix frontend
robert-oleynik Feb 26, 2025
01198f5
catch WebAuthn exception on log in
robert-oleynik Feb 26, 2025
8a495fd
minor fixes
robert-oleynik Feb 26, 2025
d263bec
fix frontend
robert-oleynik Feb 26, 2025
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
2 changes: 2 additions & 0 deletions MIGRATIONS.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ User-facing changes are documented in the [changelog](CHANGELOG.released.md).
- Example command for the migration: `PG_PASSWORD=myPassword python main.py --src localhost:7500 --dst localhost:7155 --num_threads 20 --postgres webknossos@localhost:5430/webknossos`

### Postgres Evolutions:

- [126-add-webauthn-credentials.sql](./conf/evolutions/126-add-webauthn-credentials.sql)
202 changes: 184 additions & 18 deletions app/controllers/AuthenticationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ package controllers
import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext}
import com.scalableminds.util.objectid.ObjectId
import com.scalableminds.util.tools.{Fox, FoxImplicits, TextUtils}
import com.scalableminds.webknossos.datastore.storage.TemporaryStore
import com.yubico.webauthn._
import com.yubico.webauthn.data._
import com.yubico.webauthn.exception._
import mail.{DefaultMails, MailchimpClient, MailchimpTag, Send}
import models.analytics.{AnalyticsService, InviteEvent, JoinOrganizationEvent, SignupEvent}
import models.organization.{Organization, OrganizationDAO, OrganizationService}
Expand All @@ -11,6 +15,7 @@ import net.liftweb.common.{Box, Empty, Failure, Full}
import org.apache.commons.codec.binary.Base64
import org.apache.commons.codec.digest.{HmacAlgorithms, HmacUtils}
import org.apache.pekko.actor.ActorSystem
import play.api.Configuration
import play.api.data.Form
import play.api.data.Forms._
import play.api.data.validation.Constraints._
Expand All @@ -29,10 +34,29 @@ import utils.WkConf
import java.net.URLEncoder
import java.nio.charset.StandardCharsets
import java.security.MessageDigest
import java.util.UUID
import javax.inject.Inject
import scala.concurrent.duration.DurationInt
import scala.concurrent.{ExecutionContext, Future}
import scala.util.Try

case class WebAuthnRegistration(name: String, key: String)
object WebAuthnRegistration {
implicit val jsonFormat: OFormat[WebAuthnRegistration] = Json.format[WebAuthnRegistration]
}

case class WebAuthnAuthentication(key: String)
object WebAuthnAuthentication {
implicit val jsonFormat: OFormat[WebAuthnAuthentication] = Json.format[WebAuthnAuthentication]
}

case class WebAuthnKeyDescriptor(id: ObjectId, name: String)
object WebAuthnKeyDescriptor {
implicit val jsonFormat: OFormat[WebAuthnKeyDescriptor] = Json.format[WebAuthnKeyDescriptor]
}

class AuthenticationController @Inject()(
configuration: Configuration,
actorSystem: ActorSystem,
credentialsProvider: CredentialsProvider,
passwordHasher: PasswordHasher,
Expand All @@ -52,6 +76,10 @@ class AuthenticationController @Inject()(
openIdConnectClient: OpenIdConnectClient,
initialDataService: InitialDataService,
emailVerificationService: EmailVerificationService,
webAuthnCredentialRepository: WebAuthnCredentialRepository,
webAuthnCredentialDAO: WebAuthnCredentialDAO,
temporaryAssertionStore: TemporaryStore[String, AssertionRequest],
temporaryRegistrationStore: TemporaryStore[ObjectId, PublicKeyCredentialCreationOptions],
sil: Silhouette[WkEnv])(implicit ec: ExecutionContext, bodyParsers: PlayBodyParsers)
extends Controller
with AuthForms
Expand All @@ -66,6 +94,13 @@ class AuthenticationController @Inject()(
private lazy val ssoKey =
conf.WebKnossos.User.ssoKey

private lazy val relyingParty = {
val origin = configuration.get[String]("http.uri").split("/")(2);
val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build();
RelyingParty.builder().identity(identity).credentialRepository(webAuthnCredentialRepository).build()
}
Comment on lines +97 to +101
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve RelyingParty configuration with error handling.

The current RelyingParty initialization doesn't handle potential configuration issues or provide logging. Additionally, splitting the domain from the URL with split("/") is fragile.

  private lazy val relyingParty = {
-    val origin = configuration.get[String]("http.uri").split("/")(2);
-    val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build();
-    RelyingParty.builder().identity(identity).credentialRepository(webAuthnCredentialRepository).build()
+    try {
+      val uri = new java.net.URI(configuration.get[String]("http.uri"))
+      val origin = uri.getHost
+      logger.info(s"Initializing WebAuthn with relying party ID: $origin")
+      val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build()
+      RelyingParty.builder()
+        .identity(identity)
+        .credentialRepository(webAuthnCredentialRepository)
+        .build()
+    } catch {
+      case e: Exception =>
+        logger.error(s"Failed to initialize WebAuthn RelyingParty: ${e.getMessage}")
+        throw e
+    }
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private lazy val relyingParty = {
val origin = configuration.get[String]("http.uri").split("/")(2);
val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build();
RelyingParty.builder().identity(identity).credentialRepository(webAuthnCredentialRepository).build()
}
private lazy val relyingParty = {
try {
val uri = new java.net.URI(configuration.get[String]("http.uri"))
val origin = uri.getHost
logger.info(s"Initializing WebAuthn with relying party ID: $origin")
val identity = RelyingPartyIdentity.builder().id(origin).name("WebKnossos").build()
RelyingParty.builder()
.identity(identity)
.credentialRepository(webAuthnCredentialRepository)
.build()
} catch {
case e: Exception =>
logger.error(s"Failed to initialize WebAuthn RelyingParty: ${e.getMessage}")
throw e
}
}

private val blockingContext: ExecutionContext = actorSystem.dispatchers.lookup("play.context.blocking")

private lazy val isOIDCEnabled = conf.Features.openIdConnectEnabled

def register: Action[AnyContent] = Action.async { implicit request =>
Expand Down Expand Up @@ -143,6 +178,26 @@ class AuthenticationController @Inject()(
}
}

private def authenticateInner(loginInfo: LoginInfo)(implicit header: RequestHeader): Future[Result] =
for {
result <- userService.retrieve(loginInfo).flatMap {
case Some(user) if !user.isDeactivated =>
for {
authenticator <- combinedAuthenticatorService.create(loginInfo)
value <- combinedAuthenticatorService.init(authenticator)
result <- combinedAuthenticatorService.embed(value, Ok)
_ <- Fox.runIf(conf.WebKnossos.User.EmailVerification.activated)(
emailVerificationService.assertEmailVerifiedOrResendVerificationMail(user)(GlobalAccessContext, ec))
_ <- multiUserDAO.updateLastLoggedInIdentity(user._multiUser, user._id)(GlobalAccessContext)
_ = userDAO.updateLastActivity(user._id)(GlobalAccessContext)
_ = logger.info(f"User ${user._id} authenticated.")
} yield result
case None =>
Future.successful(BadRequest(Messages("error.noUser")))
case Some(_) => Future.successful(BadRequest(Messages("user.deactivated")))
}
} yield result;

def authenticate: Action[AnyContent] = Action.async { implicit request =>
signInForm
.bindFromRequest()
Expand All @@ -156,24 +211,8 @@ class AuthenticationController @Inject()(
idF
.map(id => Credentials(id, signInData.password))
.flatMap(credentials => credentialsProvider.authenticate(credentials))
.flatMap {
loginInfo =>
userService.retrieve(loginInfo).flatMap {
case Some(user) if !user.isDeactivated =>
for {
authenticator <- combinedAuthenticatorService.create(loginInfo)
value <- combinedAuthenticatorService.init(authenticator)
result <- combinedAuthenticatorService.embed(value, Ok)
_ <- Fox.runIf(conf.WebKnossos.User.EmailVerification.activated)(emailVerificationService
.assertEmailVerifiedOrResendVerificationMail(user)(GlobalAccessContext, ec))
_ <- multiUserDAO.updateLastLoggedInIdentity(user._multiUser, user._id)(GlobalAccessContext)
_ = userDAO.updateLastActivity(user._id)(GlobalAccessContext)
_ = logger.info(f"User ${user._id} authenticated.")
} yield result
case None =>
Future.successful(BadRequest(Messages("error.noUser")))
case Some(_) => Future.successful(BadRequest(Messages("user.deactivated")))
}
.flatMap { loginInfo =>
authenticateInner(loginInfo)
}
.recover {
case _: ProviderException => BadRequest(Messages("error.invalidCredentials"))
Expand Down Expand Up @@ -406,6 +445,133 @@ class AuthenticationController @Inject()(
}
}

def webauthnAuthStart(): Action[AnyContent] = Action {
val opts = StartAssertionOptions.builder().build();
val assertion = relyingParty.startAssertion(opts);
val sessionId = UUID.randomUUID().toString;
val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes));
Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
}
Comment on lines +448 to +455
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for WebAuthn authentication start.

The current implementation doesn't handle potential exceptions from the RelyingParty or token generation.

  def webauthnAuthStart(): Action[AnyContent] = Action {
-    val opts = StartAssertionOptions.builder().build();
-    val assertion = relyingParty.startAssertion(opts);
-    val sessionId = UUID.randomUUID().toString;
-    val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
-    temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes));
-    Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
+    try {
+      val opts = StartAssertionOptions.builder().build()
+      val assertion = relyingParty.startAssertion(opts)
+      val sessionId = UUID.randomUUID().toString
+      val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
+      temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes))
+      logger.debug("Started WebAuthn authentication")
+      Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
+    } catch {
+      case e: Exception =>
+        logger.error(s"Failed to start WebAuthn authentication: ${e.getMessage}")
+        InternalServerError(Json.obj("error" -> "Failed to start authentication"))
+    }
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def webauthnAuthStart(): Action[AnyContent] = Action {
val opts = StartAssertionOptions.builder().build();
val assertion = relyingParty.startAssertion(opts);
val sessionId = UUID.randomUUID().toString;
val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes));
Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
}
def webauthnAuthStart(): Action[AnyContent] = Action {
try {
val opts = StartAssertionOptions.builder().build()
val assertion = relyingParty.startAssertion(opts)
val sessionId = UUID.randomUUID().toString
val cookie = Cookie("webauthn-session", sessionId, maxAge = Some(120), httpOnly = true, secure = true)
temporaryAssertionStore.insert(sessionId, assertion, Some(2 minutes))
logger.debug("Started WebAuthn authentication")
Ok(Json.toJson(Json.parse(assertion.toCredentialsGetJson))).withCookies(cookie)
} catch {
case e: Exception =>
logger.error(s"Failed to start WebAuthn authentication: ${e.getMessage}")
InternalServerError(Json.obj("error" -> "Failed to start authentication"))
}
}


def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) {
implicit request =>
{
request.cookies.get("webauthn-session") match {
case None =>
Future.successful(BadRequest("Authentication took too long, please try again."))
case Some(cookie) =>
val sessionId = cookie.value
val challengeData = temporaryAssertionStore.get(sessionId)
temporaryAssertionStore.remove(sessionId)
challengeData match {
case None => Future.successful(Unauthorized("Authentication timeout."))
case Some(data) => {
val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key);
val opts = FinishAssertionOptions.builder().request(data).response(keyCredential).build();
for {
result <- Fox
.future2Fox(Future { Try(relyingParty.finishAssertion(opts)) })(blockingContext); // NOTE: Prevent blocking on HTTP handler
assertion <- result match {
case scala.util.Success(assertion) => Fox.successful(assertion);
case scala.util.Failure(e) => Fox.failure("Authentication failed.", Full(e));
};
userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle);
multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext);
result <- multiUser._lastLoggedInIdentity match {
case None => Future.successful(InternalServerError("user never logged in"))
case Some(userId) => {
val loginInfo = LoginInfo("credentials", userId.toString);
authenticateInner(loginInfo)
}
}
} yield result;
}
}
}
}
}
Comment on lines +457 to +493
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling and refactor WebAuthn authentication finalization.

The current implementation has nested conditionals and could benefit from a more functional style. Additionally, error handling could be improved.

  def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) {
    implicit request =>
-      {
-        request.cookies.get("webauthn-session") match {
-          case None =>
-            Future.successful(BadRequest("Authentication took too long, please try again."))
-          case Some(cookie) =>
-            val sessionId = cookie.value
-            val challengeData = temporaryAssertionStore.get(sessionId)
-            temporaryAssertionStore.remove(sessionId)
-            challengeData match {
-              case None => Future.successful(Unauthorized("Authentication timeout."))
-              case Some(data) => {
-                val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key);
-                val opts = FinishAssertionOptions.builder().request(data).response(keyCredential).build();
-                for {
-                  result <- Fox
-                    .future2Fox(Future { Try(relyingParty.finishAssertion(opts)) })(blockingContext); // NOTE: Prevent blocking on HTTP handler
-                  assertion <- result match {
-                    case scala.util.Success(assertion) => Fox.successful(assertion);
-                    case scala.util.Failure(e)         => Fox.failure("Authentication failed.", Full(e));
-                  };
-                  userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle);
-                  multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext);
-                  result <- multiUser._lastLoggedInIdentity match {
-                    case None => Future.successful(InternalServerError("user never logged in"))
-                    case Some(userId) => {
-                      val loginInfo = LoginInfo("credentials", userId.toString);
-                      authenticateInner(loginInfo)
-                    }
-                  }
-                } yield result;
-              }
-            }
-        }
-      }
+      for {
+        sessionCookie <- Fox.option2Fox(request.cookies.get("webauthn-session")) ?~> "Authentication took too long, please try again." ~> BAD_REQUEST
+        sessionId = sessionCookie.value
+        challengeData <- Fox.option2Fox(temporaryAssertionStore.get(sessionId)) ?~> "Authentication timeout." ~> UNAUTHORIZED
+        _ = temporaryAssertionStore.remove(sessionId)
+        result <- {
+          try {
+            val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key)
+            val opts = FinishAssertionOptions.builder().request(challengeData).response(keyCredential).build()
+            
+            for {
+              assertion <- Fox.future2Fox(Future { 
+                Try(relyingParty.finishAssertion(opts)) match {
+                  case scala.util.Success(a) => a
+                  case scala.util.Failure(e) => {
+                    logger.warn(s"WebAuthn authentication failed: ${e.getMessage}")
+                    throw e
+                  }
+                }
+              })(blockingContext)
+              userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle)
+              multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext) ?~> "User not found" ~> NOT_FOUND
+              lastLoggedInId <- Fox.option2Fox(multiUser._lastLoggedInIdentity) ?~> "User has never logged in before" ~> INTERNAL_SERVER_ERROR
+              loginInfo = LoginInfo("credentials", lastLoggedInId.toString)
+              result <- authenticateInner(loginInfo)
+              _ = logger.info(s"User authenticated with WebAuthn: ${lastLoggedInId}")
+            } yield result
+          } catch {
+            case e: Exception =>
+              logger.error(s"Error finalizing WebAuthn authentication: ${e.getMessage}")
+              Fox.failure("Authentication failed: " + e.getMessage, Full(e)) ~> BAD_REQUEST
+          }
+        }
+      } yield result
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) {
implicit request =>
{
request.cookies.get("webauthn-session") match {
case None =>
Future.successful(BadRequest("Authentication took too long, please try again."))
case Some(cookie) =>
val sessionId = cookie.value
val challengeData = temporaryAssertionStore.get(sessionId)
temporaryAssertionStore.remove(sessionId)
challengeData match {
case None => Future.successful(Unauthorized("Authentication timeout."))
case Some(data) => {
val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key);
val opts = FinishAssertionOptions.builder().request(data).response(keyCredential).build();
for {
result <- Fox
.future2Fox(Future { Try(relyingParty.finishAssertion(opts)) })(blockingContext); // NOTE: Prevent blocking on HTTP handler
assertion <- result match {
case scala.util.Success(assertion) => Fox.successful(assertion);
case scala.util.Failure(e) => Fox.failure("Authentication failed.", Full(e));
};
userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle);
multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext);
result <- multiUser._lastLoggedInIdentity match {
case None => Future.successful(InternalServerError("user never logged in"))
case Some(userId) => {
val loginInfo = LoginInfo("credentials", userId.toString);
authenticateInner(loginInfo)
}
}
} yield result;
}
}
}
}
}
def webauthnAuthFinalize(): Action[WebAuthnAuthentication] = Action.async(validateJson[WebAuthnAuthentication]) {
implicit request =>
for {
sessionCookie <- Fox.option2Fox(request.cookies.get("webauthn-session")) ?~> "Authentication took too long, please try again." ~> BAD_REQUEST
sessionId = sessionCookie.value
challengeData <- Fox.option2Fox(temporaryAssertionStore.get(sessionId)) ?~> "Authentication timeout." ~> UNAUTHORIZED
_ = temporaryAssertionStore.remove(sessionId)
result <- {
try {
val keyCredential = PublicKeyCredential.parseAssertionResponseJson(request.body.key)
val opts = FinishAssertionOptions.builder().request(challengeData).response(keyCredential).build()
for {
assertion <- Fox.future2Fox(Future {
Try(relyingParty.finishAssertion(opts)) match {
case scala.util.Success(a) => a
case scala.util.Failure(e) =>
logger.warn(s"WebAuthn authentication failed: ${e.getMessage}")
throw e
}
})(blockingContext)
userId = WebAuthnCredentialRepository.byteArrayToObjectId(assertion.getCredential.getUserHandle)
multiUser <- multiUserDAO.findOne(userId)(GlobalAccessContext) ?~> "User not found" ~> NOT_FOUND
lastLoggedInId <- Fox.option2Fox(multiUser._lastLoggedInIdentity) ?~> "User has never logged in before" ~> INTERNAL_SERVER_ERROR
loginInfo = LoginInfo("credentials", lastLoggedInId.toString)
result <- authenticateInner(loginInfo)
_ = logger.info(s"User authenticated with WebAuthn: ${lastLoggedInId}")
} yield result
} catch {
case e: Exception =>
logger.error(s"Error finalizing WebAuthn authentication: ${e.getMessage}")
Fox.failure("Authentication failed: " + e.getMessage, Full(e)) ~> BAD_REQUEST
}
}
} yield result
}


def webauthnRegisterStart(): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
for {
email <- userService.emailFor(request.identity);
result <- Future {
val userIdentity = UserIdentity
.builder()
.name(email)
.displayName(request.identity.name)
.id(WebAuthnCredentialRepository.objectIdToByteArray(request.identity._multiUser))
.build();
val opts = StartRegistrationOptions
.builder()
.user(userIdentity)
.timeout(60000)
.authenticatorSelection(
AuthenticatorSelectionCriteria.builder().residentKey(ResidentKeyRequirement.REQUIRED).build())
.build()
val registration = relyingParty.startRegistration(opts);
temporaryRegistrationStore.insert(request.identity._multiUser, registration);
Ok(Json.toJson(registration.toCredentialsCreateJson))
}(blockingContext)
} yield result;
}

def webauthnRegisterFinalize(): Action[WebAuthnRegistration] =
sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request =>
{
val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser)
temporaryRegistrationStore.remove(request.identity._multiUser)
creationOpts match {
case Some(data) => {
val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key);
val opts = FinishRegistrationOptions.builder().request(data).response(response).build();
try {
for {
preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext);
key <- preKey match {
case scala.util.Success(key) => Fox.successful(key)
case scala.util.Failure(e) => Fox.failure("Registration failed", Full(e))
};
result <- {
val credential = WebAuthnCredential(
ObjectId.generate,
request.identity._multiUser,
WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
request.body.name,
key.getPublicKeyCose.getBytes,
key.getSignatureCount.toInt,
isDeleted = false,
)
webAuthnCredentialDAO.insertOne(credential).map(_ => Ok(""))
}
} yield result;
} catch {
case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key"))
}
}
case None => Future.successful(BadRequest("Challenge not found or expired"))
}
}
}
Comment on lines +519 to +555
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Strengthen error handling and improve structure of WebAuthn registration finalization.

The current implementation has a try-catch block that only catches RegistrationFailedException and uses nested conditionals. The error handling could be more comprehensive and the code structure could be improved.

  def webauthnRegisterFinalize(): Action[WebAuthnRegistration] =
    sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request =>
-      {
-        val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser)
-        temporaryRegistrationStore.remove(request.identity._multiUser)
-        creationOpts match {
-          case Some(data) => {
-            val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key);
-            val opts = FinishRegistrationOptions.builder().request(data).response(response).build();
-            try {
-              for {
-                preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext);
-                key <- preKey match {
-                  case scala.util.Success(key) => Fox.successful(key)
-                  case scala.util.Failure(e)   => Fox.failure("Registration failed", Full(e))
-                };
-                result <- {
-                  val credential = WebAuthnCredential(
-                    ObjectId.generate,
-                    request.identity._multiUser,
-                    WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
-                    request.body.name,
-                    key.getPublicKeyCose.getBytes,
-                    key.getSignatureCount.toInt,
-                    isDeleted = false,
-                  )
-                  webAuthnCredentialDAO.insertOne(credential).map(_ => Ok(""))
-                }
-              } yield result;
-            } catch {
-              case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key"))
-            }
-          }
-          case None => Future.successful(BadRequest("Challenge not found or expired"))
-        }
-      }
+      for {
+        creationOpts <- Fox.option2Fox(temporaryRegistrationStore.get(request.identity._multiUser)) ?~> 
+          "Challenge not found or expired" ~> BAD_REQUEST
+        _ = temporaryRegistrationStore.remove(request.identity._multiUser)
+        result <- {
+          try {
+            val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key)
+            val opts = FinishRegistrationOptions.builder().request(creationOpts).response(response).build()
+            
+            for {
+              key <- Fox.future2Fox(Future { 
+                Try(relyingParty.finishRegistration(opts)) match {
+                  case scala.util.Success(k) => k
+                  case scala.util.Failure(e) => {
+                    logger.warn(s"WebAuthn registration failed: ${e.getMessage}")
+                    throw e
+                  }
+                }
+              })(blockingContext)
+              
+              // Check if a passkey with this name already exists
+              existingKeys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
+              _ <- Fox.bool2Fox(!existingKeys.exists(_.name == request.body.name)) ?~> 
+                "A passkey with this name already exists" ~> BAD_REQUEST
+                
+              credential = WebAuthnCredential(
+                ObjectId.generate,
+                request.identity._multiUser,
+                WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
+                request.body.name,
+                key.getPublicKeyCose.getBytes,
+                key.getSignatureCount.toInt,
+                isDeleted = false
+              )
+              _ <- webAuthnCredentialDAO.insertOne(credential)
+              _ = logger.info(s"User ${request.identity._id} registered new passkey: ${request.body.name}")
+            } yield Ok(Json.obj("message" -> "Passkey registered successfully"))
+          } catch {
+            case e: RegistrationFailedException =>
+              logger.warn(s"WebAuthn registration failed: ${e.getMessage}")
+              Fox.failure("Failed to register key") ~> BAD_REQUEST
+            case e: Exception =>
+              logger.error(s"Error in WebAuthn registration: ${e.getMessage}", e)
+              Fox.failure("Registration failed: " + e.getMessage) ~> INTERNAL_SERVER_ERROR
+          }
+        }
+      } yield result
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def webauthnRegisterFinalize(): Action[WebAuthnRegistration] =
sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request =>
{
val creationOpts = temporaryRegistrationStore.get(request.identity._multiUser)
temporaryRegistrationStore.remove(request.identity._multiUser)
creationOpts match {
case Some(data) => {
val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key);
val opts = FinishRegistrationOptions.builder().request(data).response(response).build();
try {
for {
preKey <- Fox.future2Fox(Future { Try(relyingParty.finishRegistration(opts)) })(blockingContext);
key <- preKey match {
case scala.util.Success(key) => Fox.successful(key)
case scala.util.Failure(e) => Fox.failure("Registration failed", Full(e))
};
result <- {
val credential = WebAuthnCredential(
ObjectId.generate,
request.identity._multiUser,
WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
request.body.name,
key.getPublicKeyCose.getBytes,
key.getSignatureCount.toInt,
isDeleted = false,
)
webAuthnCredentialDAO.insertOne(credential).map(_ => Ok(""))
}
} yield result;
} catch {
case e: RegistrationFailedException => Future.successful(BadRequest("Failed to register key"))
}
}
case None => Future.successful(BadRequest("Challenge not found or expired"))
}
}
}
def webauthnRegisterFinalize(): Action[WebAuthnRegistration] =
sil.SecuredAction.async(validateJson[WebAuthnRegistration]) { implicit request =>
for {
creationOpts <- Fox.option2Fox(temporaryRegistrationStore.get(request.identity._multiUser)) ?~>
"Challenge not found or expired" ~> BAD_REQUEST
_ = temporaryRegistrationStore.remove(request.identity._multiUser)
result <- {
try {
val response = PublicKeyCredential.parseRegistrationResponseJson(request.body.key)
val opts = FinishRegistrationOptions.builder().request(creationOpts).response(response).build()
for {
key <- Fox.future2Fox(Future {
Try(relyingParty.finishRegistration(opts)) match {
case scala.util.Success(k) => k
case scala.util.Failure(e) =>
logger.warn(s"WebAuthn registration failed: ${e.getMessage}")
throw e
}
})(blockingContext)
// Check if a passkey with this name already exists
existingKeys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
_ <- Fox.bool2Fox(!existingKeys.exists(_.name == request.body.name)) ?~>
"A passkey with this name already exists" ~> BAD_REQUEST
credential = WebAuthnCredential(
ObjectId.generate,
request.identity._multiUser,
WebAuthnCredentialRepository.byteArrayToBytes(key.getKeyId.getId),
request.body.name,
key.getPublicKeyCose.getBytes,
key.getSignatureCount.toInt,
isDeleted = false
)
_ <- webAuthnCredentialDAO.insertOne(credential)
_ = logger.info(s"User ${request.identity._id} registered new passkey: ${request.body.name}")
} yield Ok(Json.obj("message" -> "Passkey registered successfully"))
} catch {
case e: RegistrationFailedException =>
logger.warn(s"WebAuthn registration failed: ${e.getMessage}")
Fox.failure("Failed to register key") ~> BAD_REQUEST
case e: Exception =>
logger.error(s"Error in WebAuthn registration: ${e.getMessage}", e)
Fox.failure("Registration failed: " + e.getMessage) ~> INTERNAL_SERVER_ERROR
}
}
} yield result
}


def webauthnListKeys: Action[AnyContent] = sil.SecuredAction.async { implicit request =>
{
for {
keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
reducedKeys = keys.map(credential => WebAuthnKeyDescriptor(credential._id, credential.name))
} yield Ok(Json.toJson(reducedKeys))
}
}

def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) {
implicit request =>
{
for {
_ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser)
} yield Ok(Json.obj())
}
}
Comment on lines +566 to +573
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add validation and error handling to WebAuthn remove key method.

The current implementation doesn't validate that the key belongs to the user nor handle potential errors from the DAO.

  def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) {
    implicit request =>
-      {
-        for {
-          _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser)
-        } yield Ok(Json.obj())
-      }
+      for {
+        // First verify the key exists and belongs to the user
+        keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
+        _ <- Fox.bool2Fox(keys.exists(_.id == request.body.id)) ?~> 
+          "Passkey not found or doesn't belong to you" ~> NOT_FOUND
+        // Then remove it
+        _ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser)
+          .recoverWith {
+            case e: Exception =>
+              logger.error(s"Error removing WebAuthn key: ${e.getMessage}")
+              Fox.failure("Failed to remove passkey") ~> INTERNAL_SERVER_ERROR
+          }
+      } yield {
+        logger.info(s"User ${request.identity._id} removed passkey: ${request.body.name}")
+        Ok(Json.obj("message" -> "Passkey removed successfully"))
+      }
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) {
implicit request =>
{
for {
_ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser)
} yield Ok(Json.obj())
}
}
def webauthnRemoveKey: Action[WebAuthnKeyDescriptor] = sil.SecuredAction.async(validateJson[WebAuthnKeyDescriptor]) {
implicit request =>
for {
// First verify the key exists and belongs to the user
keys <- webAuthnCredentialDAO.listKeys(request.identity._multiUser)
_ <- Fox.bool2Fox(keys.exists(_.id == request.body.id)) ?~>
"Passkey not found or doesn't belong to you" ~> NOT_FOUND
// Then remove it
_ <- webAuthnCredentialDAO.removeById(request.body.id, request.identity._multiUser)
.recoverWith {
case e: Exception =>
logger.error(s"Error removing WebAuthn key: ${e.getMessage}")
Fox.failure("Failed to remove passkey") ~> INTERNAL_SERVER_ERROR
}
} yield {
logger.info(s"User ${request.identity._id} removed passkey: ${request.body.name}")
Ok(Json.obj("message" -> "Passkey removed successfully"))
}
}


private lazy val absoluteOpenIdConnectCallbackURL = s"${conf.Http.uri}/api/auth/oidc/callback"

def loginViaOpenIdConnect(): Action[AnyContent] = sil.UserAwareAction.async { implicit request =>
Expand Down
7 changes: 7 additions & 0 deletions app/models/user/MultiUser.scala
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,13 @@ class MultiUserDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext
(SELECT _id FROM webknossos.users WHERE _organization = $organizationId)""".asUpdate)
} yield ()

def findOneById(id: ObjectId)(implicit ctx: DBAccessContext): Fox[MultiUser] =
for {
accessQuery <- readAccessQuery
r <- run(q"SELECT $columns FROM $existingCollectionName WHERE _id = $id AND $accessQuery".as[MultiusersRow])
parsed <- parseFirst(r, id)
} yield parsed

def findOneByEmail(email: String)(implicit ctx: DBAccessContext): Fox[MultiUser] =
for {
accessQuery <- readAccessQuery
Expand Down
94 changes: 94 additions & 0 deletions app/models/user/WebAuthnCredentials.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
package models.user

import com.scalableminds.util.accesscontext.DBAccessContext
import com.scalableminds.util.objectid.ObjectId
import com.scalableminds.util.tools.Fox
import com.scalableminds.webknossos.schema.Tables._
import slick.lifted.Rep
import utils.sql.{SQLDAO, SqlClient}

import javax.inject.Inject
import scala.concurrent.ExecutionContext

case class WebAuthnCredential(
_id: ObjectId,
_multiUser: ObjectId,
keyId: Array[Byte],
name: String,
publicKeyCose: Array[Byte],
signatureCount: Int,
isDeleted: Boolean,
)

class WebAuthnCredentialDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
extends SQLDAO[WebAuthnCredential, WebauthncredentialsRow, Webauthncredentials](sqlClient) {
protected val collection = Webauthncredentials

override protected def idColumn(x: Webauthncredentials): Rep[String] = x._Id

override protected def isDeletedColumn(x: Webauthncredentials): Rep[Boolean] = x.isdeleted

protected def parse(r: WebauthncredentialsRow): Fox[WebAuthnCredential] =
Fox.successful(
WebAuthnCredential(
ObjectId(r._Id),
ObjectId(r._Multiuser),
r.keyid,
r.name,
r.publickeycose,
r.signaturecount,
r.isdeleted
)
)

def findAllForUser(userId: ObjectId)(implicit ctx: DBAccessContext): Fox[List[WebAuthnCredential]] =
for {
accessQuery <- readAccessQuery
r <- run(
q"SELECT $columns FROM webknossos.webauthncredentials WHERE _multiUser = $userId AND $accessQuery"
.as[WebauthncredentialsRow])
parsed <- parseAll(r)
} yield parsed

def listByKeyId(id: Array[Byte])(implicit ctx: DBAccessContext): Fox[List[WebAuthnCredential]] =
for {
accessQuery <- readAccessQuery
r <- run(
q"SELECT $columns FROM webknossos.webauthncredentials WHERE keyId = $id AND $accessQuery"
.as[WebauthncredentialsRow])
parsed <- parseAll(r)
} yield parsed

def findByKeyIdAndUserId(id: Array[Byte], userId: ObjectId)(implicit ctx: DBAccessContext): Fox[WebAuthnCredential] =
for {
accessQuery <- readAccessQuery
r <- run(
q"SELECT $columns FROM webknossos.webauthncredentials WHERE keyId = $id AND _multiUser = $userId AND $accessQuery"
.as[WebauthncredentialsRow])
parsed <- parseAll(r)
first <- Fox.option2Fox(parsed.headOption)
} yield first

def insertOne(c: WebAuthnCredential): Fox[Unit] =
for {
_ <- run(
q"""INSERT INTO webknossos.webauthncredentials(_id, _multiUser, keyId, name, publicKeyCose, signatureCount)
VALUES(${c._id}, ${c._multiUser}, ${c.keyId}, ${c.name},
${c.publicKeyCose}, ${c.signatureCount})""".asUpdate)
} yield ()

def listKeys(multiUser: ObjectId)(implicit ctx: DBAccessContext): Fox[List[WebAuthnCredential]] =
for {
accessQuery <- readAccessQuery
r <- run(
q"""SELECT $columns FROM webknossos.webauthncredentials WHERE _multiUser = $multiUser AND $accessQuery"""
.as[WebauthncredentialsRow])
parsed <- parseAll(r)
} yield parsed

def removeById(id: ObjectId, multiUser: ObjectId): Fox[Unit] =
for {
_ <- run(q"""DELETE FROM webknossos.webauthncredentials WHERE _id = ${id} AND _multiUser=${multiUser}""".asUpdate)
} yield ()

}
Loading