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

Feature/recover password #113

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ data class AuthConfigProperties(
val publicKey: RSAPublicKey,
val privateKey: RSAPrivateKey,
val jwtAccessExpirationMinutes: Long,
val jwtRefreshExpirationDays: Long
val jwtRefreshExpirationDays: Long,
val jwtRecoveryExpirationMinutes: Long
)
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
package pt.up.fe.ni.website.backend.controller

import jakarta.validation.Valid
import org.springframework.beans.factory.annotation.Value
import org.springframework.security.access.prepost.PreAuthorize
import org.springframework.web.bind.annotation.GetMapping
import org.springframework.web.bind.annotation.PathVariable
import org.springframework.web.bind.annotation.PostMapping
import org.springframework.web.bind.annotation.RequestBody
import org.springframework.web.bind.annotation.RequestMapping
import org.springframework.web.bind.annotation.RestController
import pt.up.fe.ni.website.backend.dto.auth.LoginDto
import pt.up.fe.ni.website.backend.dto.auth.PasswordRecoveryConfirmDto
import pt.up.fe.ni.website.backend.dto.auth.PasswordRecoveryRequestDto
import pt.up.fe.ni.website.backend.dto.auth.TokenDto
import pt.up.fe.ni.website.backend.model.Account
import pt.up.fe.ni.website.backend.service.AuthService

@RestController
@RequestMapping("/auth")
class AuthController(val authService: AuthService) {
@field:Value("\${page.recover-password}")
private lateinit var recoverPasswordPage: String
BrunoRosendo marked this conversation as resolved.
Show resolved Hide resolved

BrunoRosendo marked this conversation as resolved.
Show resolved Hide resolved
@PostMapping("/new")
fun getNewToken(@RequestBody loginDto: LoginDto): Map<String, String> {
val account = authService.authenticate(loginDto.email, loginDto.password)
Expand All @@ -28,6 +36,21 @@ class AuthController(val authService: AuthService) {
return mapOf("access_token" to accessToken)
}

@PostMapping("/password/recovery")
fun generateRecoveryToken(@RequestBody recoveryRequestDto: PasswordRecoveryRequestDto): Map<String, String> {
val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)
?: return emptyMap()
Comment on lines +41 to +42
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)
?: return emptyMap()
val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)

// TODO: Change to email service
return mapOf("recovery_url" to "$recoverPasswordPage/$recoveryToken/confirm")
Comment on lines +41 to +44
Copy link
Member

Choose a reason for hiding this comment

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

Make this return OK all the time and comment on what is supposed to go on an email. If someone wants to test this right away, one can simply print the result. It is safer to leave the correct behavior already implemented.

Suggested change
val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)
?: return emptyMap()
// TODO: Change to email service
return mapOf("recovery_url" to "$recoverPasswordPage/$recoveryToken/confirm")
val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)
?: return emptyMap()
// TODO: Add email service with payload "$recoverPasswordPage/$recoveryToken/
return ""

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about the return "", but it should always be status 200.

Also document on why this is done for security reasons, in order to not leak what are the emails associated with our service

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't we have it return the recovery link for now so we can test the feature and remove it when the email service is complete?

If I'm not wrong it always returns status 200.

}

@PostMapping("/password/recovery/{token}/confirm")
fun confirmRecoveryToken(
@Valid @RequestBody
dto: PasswordRecoveryConfirmDto,
@PathVariable token: String
) = authService.confirmRecoveryToken(token, dto)

@GetMapping
@PreAuthorize("hasRole('MEMBER')")
fun checkAuthentication(): Map<String, Account> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import org.springframework.http.HttpStatus
import org.springframework.http.converter.HttpMessageNotReadableException
import org.springframework.security.access.AccessDeniedException
import org.springframework.security.core.AuthenticationException
import org.springframework.security.oauth2.jwt.BadJwtException
import org.springframework.security.oauth2.jwt.JwtValidationException
import org.springframework.web.bind.MethodArgumentNotValidException
import org.springframework.web.bind.annotation.ExceptionHandler
import org.springframework.web.bind.annotation.RequestMapping
Expand All @@ -19,6 +21,7 @@ import org.springframework.web.bind.annotation.RestControllerAdvice
import org.springframework.web.multipart.MaxUploadSizeExceededException
import org.springframework.web.multipart.support.MissingServletRequestPartException
import pt.up.fe.ni.website.backend.config.Logging
import pt.up.fe.ni.website.backend.service.ErrorMessages

data class SimpleError(
val message: String,
Expand Down Expand Up @@ -141,6 +144,22 @@ class ErrorController(private val objectMapper: ObjectMapper) : ErrorController,
return wrapSimpleError(e.message ?: "invalid authentication")
}

@ExceptionHandler(JwtValidationException::class)
@ResponseStatus(HttpStatus.UNAUTHORIZED)
fun invalidAuthentication(e: JwtValidationException): CustomError {
return if (e.message?.contains("expired") == true) {
wrapSimpleError(ErrorMessages.expiredToken)
} else {
wrapSimpleError(ErrorMessages.invalidToken)
}
}

@ExceptionHandler(BadJwtException::class)
@ResponseStatus(HttpStatus.UNAUTHORIZED)
fun invalidAuthentication(e: BadJwtException): CustomError {
return wrapSimpleError(ErrorMessages.invalidToken)
}

fun wrapSimpleError(msg: String, param: String? = null, value: Any? = null) = CustomError(
mutableListOf(SimpleError(msg, param, value))
)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
package pt.up.fe.ni.website.backend.dto.auth

import jakarta.validation.constraints.Size
import pt.up.fe.ni.website.backend.model.constants.AccountConstants as Constants

data class ChangePasswordDto(
val oldPassword: String,

@field:Size(min = Constants.Password.minSize, max = Constants.Password.maxSize)
LuisDuarte1 marked this conversation as resolved.
Show resolved Hide resolved
val newPassword: String
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package pt.up.fe.ni.website.backend.dto.auth

import jakarta.validation.constraints.Size
import pt.up.fe.ni.website.backend.model.constants.AccountConstants as Constants

data class PasswordRecoveryConfirmDto(
@field:Size(min = Constants.Password.minSize, max = Constants.Password.maxSize)
LuisDuarte1 marked this conversation as resolved.
Show resolved Hide resolved
val password: String
)

data class PasswordRecoveryRequestDto(
val email: String
)
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class AccountService(
return repository.save(newAccount)
}

fun updateAccount(account: Account): Account = repository.save(account)

fun getAccountByEmail(email: String): Account = repository.findByEmail(email)
?: throw NoSuchElementException(ErrorMessages.emailNotFound(email))

Expand Down
55 changes: 43 additions & 12 deletions src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import org.springframework.security.oauth2.jwt.JwtEncoderParameters
import org.springframework.security.oauth2.server.resource.InvalidBearerTokenException
import org.springframework.stereotype.Service
import pt.up.fe.ni.website.backend.config.auth.AuthConfigProperties
import pt.up.fe.ni.website.backend.dto.auth.PasswordRecoveryConfirmDto
import pt.up.fe.ni.website.backend.model.Account

@Service
Expand Down Expand Up @@ -44,39 +45,69 @@ class AuthService(
}

fun refreshAccessToken(refreshToken: String): String {
val jwt =
try {
jwtDecoder.decode(refreshToken)
} catch (e: Exception) {
throw InvalidBearerTokenException(ErrorMessages.invalidRefreshToken)
}
if (jwt.expiresAt?.isBefore(Instant.now()) != false) {
throw InvalidBearerTokenException(ErrorMessages.expiredRefreshToken)
}
val jwt = jwtDecoder.decode(refreshToken)
Comment on lines -47 to +48
Copy link
Member

Choose a reason for hiding this comment

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

Why the changes here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the try catch block and handled the exception in the ErrorController and the token expiration validation is already handled in the decode() method.

val account = accountService.getAccountByEmail(jwt.subject)
return generateAccessToken(account)
}

fun generateRecoveryToken(email: String): String? {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun generateRecoveryToken(email: String): String? {
fun generateRecoveryToken(email: String): String {

Correct me if I'm wrong but I believe it can't be null

val account = try {
accountService.getAccountByEmail(email)
} catch (e: Exception) {
return null
Copy link
Member

Choose a reason for hiding this comment

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

If the error is not the expected one (account not found), log the error so that developers can more easily find the problem

Copy link
Member Author

Choose a reason for hiding this comment

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

If someone uses an email that's not related to an account it would trigger the exception. I don't think we should log this.

}
return generateToken(
account,
Duration.ofMinutes(authConfigProperties.jwtRecoveryExpirationMinutes),
usePasswordHash = true
)
}

fun confirmRecoveryToken(recoveryToken: String, dto: PasswordRecoveryConfirmDto): Account {
val jwt = jwtDecoder.decode(recoveryToken)
val account = accountService.getAccountByEmail(jwt.subject)

val tokenPasswordHash = jwt.getClaim<String>("passwordHash")
?: throw InvalidBearerTokenException(ErrorMessages.invalidToken)

if (account.password != tokenPasswordHash) {
throw InvalidBearerTokenException(ErrorMessages.invalidToken)
}

account.password = passwordEncoder.encode(dto.password)
return accountService.updateAccount(account)
}
Comment on lines +66 to +79
Copy link
Member

Choose a reason for hiding this comment

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

This function also updates the account password, so I don't think this is the best name. I would either change the function name a little but, or extract the update logic and use this as an auxiliary function to validate the token


fun getAuthenticatedAccount(): Account {
val authentication = SecurityContextHolder.getContext().authentication
return accountService.getAccountByEmail(authentication.name)
}

private fun generateToken(account: Account, expiration: Duration, isRefresh: Boolean = false): String {
private fun generateToken(
account: Account,
expiration: Duration,
isRefresh: Boolean = false,
usePasswordHash: Boolean = false
): String {
val roles = if (isRefresh) emptyList() else getAuthorities() // TODO: Pass account to getAuthorities()
val scope = roles
.stream()
.map(GrantedAuthority::getAuthority)
.collect(Collectors.joining(" "))
val currentInstant = Instant.now()
val claims = JwtClaimsSet
val claimsBuilder = JwtClaimsSet
.builder()
.issuer("self")
.issuedAt(currentInstant)
.expiresAt(currentInstant.plus(expiration))
.subject(account.email)
.claim("scope", scope)
.build()

if (usePasswordHash) {
claimsBuilder.claim("passwordHash", account.password)
}

val claims = claimsBuilder.build()
return jwtEncoder.encode(JwtEncoderParameters.from(claims)).tokenValue
}
DoStini marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ object ErrorMessages {

const val invalidCredentials = "invalid credentials"

const val invalidRefreshToken = "invalid refresh token"
const val invalidToken = "invalid token"

const val expiredRefreshToken = "refresh token has expired"
const val expiredToken = "token has expired"

const val noGenerations = "no generations created yet"

Expand Down
5 changes: 5 additions & 0 deletions src/main/resources/application.properties
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ auth.private-key=classpath:certs/private.pem
auth.public-key=classpath:certs/public.pem
auth.jwt-access-expiration-minutes=60
auth.jwt-refresh-expiration-days=7
auth.jwt-recovery-expiration-minutes=15

# File Upload Config
spring.servlet.multipart.max-file-size=500KB
Expand All @@ -36,5 +37,9 @@ upload.static-path=classpath:static
# URL that will serve static content
upload.static-serve=http://localhost:3000/static

# Recover password page
# (for now it's the backend endpoint as we don't have the front end page yet)
page.recover-password=http://localhost:8080/auth/password/recovery

# Cors Origin
cors.allow-origin = http://localhost:3000
Loading