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

More user friendly error messages #442

Merged
merged 4 commits into from
Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 => ().pure[ConnectionIO]
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a debug-log so that it's visible in the logs that the user hasn't been found? (as it's not visible in the UI)

case Some(user) =>
createCode(user).flatMap(sendCode(user, _))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -100,23 +100,23 @@ 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 ()

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.Unauthorized("Incorrect login/email or password").raiseError[ConnectionIO, User]
Copy link
Member

Choose a reason for hiding this comment

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

We should probably extract this message to a constant so that it's not repeated, and the fact that the constant is used twice will also be informative for the developer

}
}

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]
}
}
}
Expand All @@ -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 e-mail format!")

private def validPassword(password: String) =
if (password.nonEmpty) ValidationOk else Left("Password cannot be empty!")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand All @@ -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]
Expand All @@ -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()
Expand Down Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
25 changes: 0 additions & 25 deletions ui/src/pages/Login/Login.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<Router history={history}>
<UserContext.Provider value={{ state: { ...initialUserState, loggedIn: false }, dispatch }}>
<Login />
</UserContext.Provider>
</Router>
);

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();
});
5 changes: 0 additions & 5 deletions ui/src/pages/Login/Login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Redirect to="/main" />;
Expand Down