From 06b50e87b17a990cb01adc774bdde33bba7105f0 Mon Sep 17 00:00:00 2001 From: Wojtek Urbanski Date: Wed, 21 Oct 2020 15:18:30 +0200 Subject: [PATCH 1/4] More user friendly error messages --- .../bootzooka/passwordreset/PasswordResetService.scala | 2 +- .../scala/com/softwaremill/bootzooka/user/UserService.scala | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala b/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala index df6b1fbe8..5e643dd59 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala @@ -27,7 +27,7 @@ class PasswordResetService( def forgotPassword(loginOrEmail: String): ConnectionIO[Unit] = { userModel.findByLoginOrEmail(loginOrEmail.lowerCased).flatMap { - case None => Fail.NotFound("user").raiseError[ConnectionIO, Unit] + case None => Fail.NotFound("Unknown login/email").raiseError[ConnectionIO, Unit] case Some(user) => createCode(user).flatMap(sendCode(user, _)) } diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala b/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala index 92c7e357b..8a882ae1e 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala @@ -108,7 +108,7 @@ class UserService( private def userOrNotFound(op: ConnectionIO[Option[User]]): ConnectionIO[User] = { op.flatMap { case Some(user) => user.pure[ConnectionIO] - case None => Fail.NotFound("user").raiseError[ConnectionIO, User] + case None => Fail.NotFound("Incorrect login/email or password").raiseError[ConnectionIO, User] } } @@ -139,7 +139,7 @@ object UserRegisterValidator { """^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$""".r private def validEmail(email: String) = - if (emailRegex.findFirstMatchIn(email).isDefined) ValidationOk else Left("Invalid e-mail!") + if (emailRegex.findFirstMatchIn(email).isDefined) ValidationOk else Left("Invalid format of an e-mail!") private def validPassword(password: String) = if (password.nonEmpty) ValidationOk else Left("Password cannot be empty!") From fb1b3da3ab5c42ca88d25742f60cbd79a92f9f9f Mon Sep 17 00:00:00 2001 From: Wojtek Urbanski Date: Wed, 21 Oct 2020 17:08:05 +0200 Subject: [PATCH 2/4] * forgotPassword always return OK, even in case of error * Login and ChangePassword operations returns Unauthorized in case of unknown user instead of NotFound --- .../com/softwaremill/bootzooka/Fail.scala | 2 +- .../softwaremill/bootzooka/http/Http.scala | 2 +- .../passwordreset/PasswordResetService.scala | 2 +- .../bootzooka/security/Auth.scala | 2 +- .../bootzooka/user/UserService.scala | 12 +++---- .../passwordreset/PasswordResetApiTest.scala | 34 +++++++++++++++++-- .../bootzooka/user/UserApiTest.scala | 24 ++++++++++++- 7 files changed, 65 insertions(+), 13 deletions(-) diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/Fail.scala b/backend/src/main/scala/com/softwaremill/bootzooka/Fail.scala index d9e2a6f98..964ee2c95 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/Fail.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/Fail.scala @@ -12,6 +12,6 @@ object Fail { case class NotFound(what: String) extends Fail case class Conflict(msg: String) extends Fail case class IncorrectInput(msg: String) extends Fail - case object Unauthorized extends Fail + case class Unauthorized(msg: String) extends Fail case object Forbidden extends Fail } diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/http/Http.scala b/backend/src/main/scala/com/softwaremill/bootzooka/http/Http.scala index 3c8cf358e..29b171b6e 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/http/Http.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/http/Http.scala @@ -46,7 +46,7 @@ class Http() extends Tapir with TapirJsonCirce with TapirSchemas with StrictLogg case Fail.Conflict(msg) => (StatusCode.Conflict, msg) case Fail.IncorrectInput(msg) => (StatusCode.BadRequest, msg) case Fail.Forbidden => (StatusCode.Forbidden, "Forbidden") - case Fail.Unauthorized => (StatusCode.Unauthorized, "Unauthorized") + case Fail.Unauthorized(msg) => (StatusCode.Unauthorized, msg) case _ => InternalServerError } diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala b/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala index 5e643dd59..9379bb514 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala @@ -27,7 +27,7 @@ class PasswordResetService( def forgotPassword(loginOrEmail: String): ConnectionIO[Unit] = { userModel.findByLoginOrEmail(loginOrEmail.lowerCased).flatMap { - case None => Fail.NotFound("Unknown login/email").raiseError[ConnectionIO, Unit] + case None => ().pure[ConnectionIO] case Some(user) => createCode(user).flatMap(sendCode(user, _)) } diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/security/Auth.scala b/backend/src/main/scala/com/softwaremill/bootzooka/security/Auth.scala index 50251086f..74ef43c2d 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/security/Auth.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/security/Auth.scala @@ -38,7 +38,7 @@ class Auth[T]( case None => logger.debug(s"Auth failed for: ${authTokenOps.tokenName} $id") // random sleep to prevent timing attacks - Timer[Task].sleep(random.nextInt(1000).millis) >> Task.raiseError(Fail.Unauthorized) + Timer[Task].sleep(random.nextInt(1000).millis) >> Task.raiseError(Fail.Unauthorized("Unauthorized")) case Some(token) => val delete = if (authTokenOps.deleteWhenValid) authTokenOps.delete(token).transact(xa) else Task.unit delete >> Task.now(authTokenOps.userId(token)) diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala b/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala index 8a882ae1e..e31028f0d 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala @@ -67,7 +67,7 @@ class UserService( def login(loginOrEmail: String, password: String, apiKeyValid: Option[Duration]): ConnectionIO[ApiKey] = for { user <- userOrNotFound(userModel.findByLoginOrEmail(loginOrEmail.lowerCased)) - _ <- verifyPassword(user, password) + _ <- verifyPassword(user, password, validationErrorMsg = "Incorrect login/email or password") apiKey <- apiKeyService.create(user.id, apiKeyValid.getOrElse(config.defaultApiKeyValid)) } yield apiKey @@ -100,7 +100,7 @@ class UserService( def changePassword(userId: Id @@ User, currentPassword: String, newPassword: String): ConnectionIO[Unit] = for { user <- userOrNotFound(userModel.findById(userId)) - _ <- verifyPassword(user, currentPassword) + _ <- verifyPassword(user, currentPassword, validationErrorMsg = "Incorrect current password") _ = logger.debug(s"Changing password for user: $userId") _ <- userModel.updatePassword(userId, User.hashPassword(newPassword)) } yield () @@ -108,15 +108,15 @@ class UserService( private def userOrNotFound(op: ConnectionIO[Option[User]]): ConnectionIO[User] = { op.flatMap { case Some(user) => user.pure[ConnectionIO] - case None => Fail.NotFound("Incorrect login/email or password").raiseError[ConnectionIO, User] + case None => Fail.Unauthorized("Incorrect login/email or password").raiseError[ConnectionIO, User] } } - private def verifyPassword(user: User, password: String): ConnectionIO[Unit] = { + private def verifyPassword(user: User, password: String, validationErrorMsg: String): ConnectionIO[Unit] = { if (user.verifyPassword(password) == Verified) { ().pure[ConnectionIO] } else { - Fail.Unauthorized.raiseError[ConnectionIO, Unit] + Fail.Unauthorized(validationErrorMsg).raiseError[ConnectionIO, Unit] } } } @@ -139,7 +139,7 @@ object UserRegisterValidator { """^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$""".r private def validEmail(email: String) = - if (emailRegex.findFirstMatchIn(email).isDefined) ValidationOk else Left("Invalid format of an e-mail!") + if (emailRegex.findFirstMatchIn(email).isDefined) ValidationOk else Left("Invalid e-mail format!") private def validPassword(password: String) = if (password.nonEmpty) ValidationOk else Left("Password cannot be empty!") diff --git a/backend/src/test/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetApiTest.scala b/backend/src/test/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetApiTest.scala index 0b71b9636..291ae9725 100644 --- a/backend/src/test/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetApiTest.scala +++ b/backend/src/test/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetApiTest.scala @@ -18,11 +18,13 @@ import sttp.client3.SttpBackend class PasswordResetApiTest extends BaseTest with TestEmbeddedPostgres with Eventually { lazy val modules: MainModule = new MainModule { override def xa: Transactor[Task] = currentDb.xa + override lazy val baseSttpBackend: SttpBackend[Task, Any] = SttpBackendStub(TaskMonadAsyncError) override lazy val config: Config = TestConfig } val requests = new Requests(modules) + import requests._ "/passwordreset" should "reset the password" in { @@ -35,7 +37,9 @@ class PasswordResetApiTest extends BaseTest with TestEmbeddedPostgres with Event response1.shouldDeserializeTo[ForgotPassword_OUT] // then - val code = eventually { codeSentToEmail(email) } + val code = eventually { + codeSentToEmail(email) + } // when val response2 = resetPassword(code, newPassword) @@ -57,7 +61,9 @@ class PasswordResetApiTest extends BaseTest with TestEmbeddedPostgres with Event response1.shouldDeserializeTo[ForgotPassword_OUT] // then - val code = eventually { codeSentToEmail(email) } + val code = eventually { + codeSentToEmail(email) + } // when resetPassword(code, newPassword).shouldDeserializeTo[PasswordReset_OUT] @@ -68,6 +74,20 @@ class PasswordResetApiTest extends BaseTest with TestEmbeddedPostgres with Event loginUser(login, newerPassword, None).status shouldBe Status.Unauthorized } + "/passwordreset/forgot" should "end up with Ok HTTP status code and do not send and email if user was not found" in { + // given + val RegisteredUser(_, email, _, _) = newRegisteredUsed() + + // when + val response1 = forgotPassword("wrongUser") + + // then + response1.status shouldBe Status.Ok + eventually { + codeWasNotSentToEmail(email) + } + } + "/passwordreset" should "not reset the password given an invalid code" in { // given val RegisteredUser(login, _, password, _) = newRegisteredUsed() @@ -107,6 +127,16 @@ class PasswordResetApiTest extends BaseTest with TestEmbeddedPostgres with Event .getOrElse(throw new IllegalStateException(s"No code found in: $emailData")) } + def codeWasNotSentToEmail(email: String): Unit = { + modules.emailService.sendBatch().unwrap + + val maybeEmail = DummyEmailSender.findSentEmail(email, "SoftwareMill Bootzooka password reset") + maybeEmail match { + case Some(emailData) => throw new IllegalStateException(s"There should be no password reset email sent to $email, but instead found $emailData") + case None => () + } + } + def codeFromResetPasswordEmail(email: String): Option[String] = { val regexp = "code=([\\w]*)".r regexp.findFirstMatchIn(email).map(_.group(1)) diff --git a/backend/src/test/scala/com/softwaremill/bootzooka/user/UserApiTest.scala b/backend/src/test/scala/com/softwaremill/bootzooka/user/UserApiTest.scala index db95e8d2d..69393bae0 100644 --- a/backend/src/test/scala/com/softwaremill/bootzooka/user/UserApiTest.scala +++ b/backend/src/test/scala/com/softwaremill/bootzooka/user/UserApiTest.scala @@ -131,6 +131,26 @@ class UserApiTest extends BaseTest with TestEmbeddedPostgres with Eventually { getUser(apiKey).status shouldBe Status.Unauthorized } + "/user/login" should "respond with 403 HTTP status code and 'Incorrect login/email or password' message if user was not found" in { + // given + val RegisteredUser(_, _, password, _) = newRegisteredUsed() + + // when + val response = loginUser("unknownLogin", password, Some(3)) + response.status shouldBe Status.Unauthorized + response.shouldDeserializeToError shouldBe "Incorrect login/email or password" + } + + "/user/login" should "respond with 403 HTTP status code and 'Incorrect login/email or password' message if password is incorrect for user" in { + // given + val RegisteredUser(login, _, _, _) = newRegisteredUsed() + + // when + val response = loginUser(login, "wrongPassword", Some(3)) + response.status shouldBe Status.Unauthorized + response.shouldDeserializeToError shouldBe "Incorrect login/email or password" + } + "/user/info" should "respond with 403 if the token is invalid" in { getUser("invalid").status shouldBe Status.Unauthorized } @@ -158,7 +178,9 @@ class UserApiTest extends BaseTest with TestEmbeddedPostgres with Eventually { val response1 = changePassword(apiKey, "invalid", newPassword) // then - response1.shouldDeserializeToError + response1.status shouldBe Status.Unauthorized + response1.shouldDeserializeToError shouldBe "Incorrect current password" + loginUser(login, password, None).status shouldBe Status.Ok loginUser(login, newPassword, None).status shouldBe Status.Unauthorized } From de5cd833a05ee278ab9fdbf608c8f928847913dc Mon Sep 17 00:00:00 2001 From: Wojtek Urbanski Date: Wed, 21 Oct 2020 17:30:52 +0200 Subject: [PATCH 3/4] Removing unnecessary after backend changes 'if' from login page --- ui/src/pages/Login/Login.test.tsx | 25 ------------------------- ui/src/pages/Login/Login.tsx | 5 ----- 2 files changed, 30 deletions(-) diff --git a/ui/src/pages/Login/Login.test.tsx b/ui/src/pages/Login/Login.test.tsx index 65e120405..f0e52e766 100644 --- a/ui/src/pages/Login/Login.test.tsx +++ b/ui/src/pages/Login/Login.test.tsx @@ -92,28 +92,3 @@ test("handles login error", async () => { expect(dispatch).not.toBeCalled(); expect(getByText("Test Error")).toBeInTheDocument(); }); - -test("handles login error 404", async () => { - (userService.login as jest.Mock).mockRejectedValueOnce({ response: { status: 404 } }); - - const { getByLabelText, getByText, findByRole } = render( - - - - - - ); - - fireEvent.blur(getByLabelText("Login or email")); - fireEvent.change(getByLabelText("Login or email"), { target: { value: "test-login" } }); - fireEvent.blur(getByLabelText("Login or email")); - fireEvent.change(getByLabelText("Password"), { target: { value: "test-password" } }); - fireEvent.blur(getByLabelText("Password")); - fireEvent.click(getByText("Sign In")); - - await findByRole("loader"); - - expect(userService.login).toBeCalledWith({ loginOrEmail: "test-login", password: "test-password" }); - expect(dispatch).not.toBeCalled(); - expect(getByText("Incorrect login/email or password!")).toBeInTheDocument(); -}); diff --git a/ui/src/pages/Login/Login.tsx b/ui/src/pages/Login/Login.tsx index 54d7589aa..aa349fe6d 100644 --- a/ui/src/pages/Login/Login.tsx +++ b/ui/src/pages/Login/Login.tsx @@ -33,11 +33,6 @@ const Login: React.FC = () => { userService .login(values) .then(({ apiKey }) => dispatch({ type: "SET_API_KEY", apiKey })) - .catch((error) => { - if (error?.response?.status === 404) throw new Error("Incorrect login/email or password!"); - - throw error; - }) ); if (loggedIn) return ; From 8bec4dbbf2af80ffc39381d58a33a60808b459f1 Mon Sep 17 00:00:00 2001 From: Wojtek Urbanski Date: Thu, 22 Oct 2020 10:04:23 +0200 Subject: [PATCH 4/4] * Extracting error message to constant * Adding log message if user can't be found during forgotPassword operation --- .../bootzooka/passwordreset/PasswordResetService.scala | 4 +++- .../scala/com/softwaremill/bootzooka/user/UserService.scala | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala b/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala index 9379bb514..1db7c93e4 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/passwordreset/PasswordResetService.scala @@ -27,7 +27,9 @@ class PasswordResetService( def forgotPassword(loginOrEmail: String): ConnectionIO[Unit] = { userModel.findByLoginOrEmail(loginOrEmail.lowerCased).flatMap { - case None => ().pure[ConnectionIO] + case None => + logger.debug(s"Could not find user with $loginOrEmail login/email") + ().pure[ConnectionIO] case Some(user) => createCode(user).flatMap(sendCode(user, _)) } diff --git a/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala b/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala index e31028f0d..2152051ea 100644 --- a/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala +++ b/backend/src/main/scala/com/softwaremill/bootzooka/user/UserService.scala @@ -26,6 +26,7 @@ class UserService( private val LoginAlreadyUsed = "Login already in use!" private val EmailAlreadyUsed = "E-mail already in use!" + private val IncorrectLoginOrPassword = "Incorrect login/email or password" def registerNewUser(login: String, email: String, password: String): ConnectionIO[ApiKey] = { def failIfDefined(op: ConnectionIO[Option[User]], msg: String): ConnectionIO[Unit] = { @@ -67,7 +68,7 @@ class UserService( def login(loginOrEmail: String, password: String, apiKeyValid: Option[Duration]): ConnectionIO[ApiKey] = for { user <- userOrNotFound(userModel.findByLoginOrEmail(loginOrEmail.lowerCased)) - _ <- verifyPassword(user, password, validationErrorMsg = "Incorrect login/email or password") + _ <- verifyPassword(user, password, validationErrorMsg = IncorrectLoginOrPassword) apiKey <- apiKeyService.create(user.id, apiKeyValid.getOrElse(config.defaultApiKeyValid)) } yield apiKey @@ -108,7 +109,7 @@ class UserService( private def userOrNotFound(op: ConnectionIO[Option[User]]): ConnectionIO[User] = { op.flatMap { case Some(user) => user.pure[ConnectionIO] - case None => Fail.Unauthorized("Incorrect login/email or password").raiseError[ConnectionIO, User] + case None => Fail.Unauthorized(IncorrectLoginOrPassword).raiseError[ConnectionIO, User] } }