Feature/recover password #113

jamcunha · 2023-02-20T16:21:11Z

Closes #84

Review checklist

Properly documents API changes in docs/openapi.yml
Contains enough appropriate tests
Behavior is as expected
Clean, well structured code

BrunoRosendo

The logic looks good overall. I didn't do a careful review since this is just a draft

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

codecov · 2023-05-03T16:31:19Z

Codecov Report

Patch coverage: 26.08% and project coverage change: -3.64 ⚠️

Comparison is base (af68fd8) 88.99% compared to head (36812a3) 85.36%.

❗ Current head 36812a3 differs from pull request most recent head 1be6f37. Consider uploading reports for the commit 1be6f37 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##             develop     #113      +/-   ##
=============================================
- Coverage      88.99%   85.36%   -3.64%     
+ Complexity       361      234     -127     
=============================================
  Files             63       43      -20     
  Lines            827      567     -260     
  Branches          65       29      -36     
=============================================
- Hits             736      484     -252     
- Misses            57       62       +5     
+ Partials          34       21      -13

Impacted Files	Coverage Δ
...ni/website/backend/controller/AccountController.kt	`83.33% <0.00%> (-16.67%)`	⬇️
...fe/ni/website/backend/controller/AuthController.kt	`83.33% <0.00%> (-16.67%)`	⬇️
.../fe/ni/website/backend/dto/auth/PassRecoveryDto.kt	`0.00% <0.00%> (ø)`
...pt/up/fe/ni/website/backend/service/AuthService.kt	`91.48% <0.00%> (-4.07%)`	⬇️
.../up/fe/ni/website/backend/service/ErrorMessages.kt	`71.42% <ø> (-19.49%)`	⬇️
...up/fe/ni/website/backend/service/AccountService.kt	`61.53% <28.57%> (-38.47%)`	⬇️
...ebsite/backend/config/auth/AuthConfigProperties.kt	`100.00% <100.00%> (ø)`

... and 51 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

BrunoRosendo · 2023-06-23T17:42:20Z

@jamcunha Is this ready for review or do you still have some doubts? We can talk about it in the next meeting or in 1on1

src/test/kotlin/pt/up/fe/ni/website/backend/controller/AccountControllerTest.kt

bdmendes

The code looks good to me. To improve security a bit, we can look into serializing the current password into the jwt to make sure it is only used once: https://stackoverflow.com/a/54865104

github-actions · 2023-06-28T21:04:18Z

Check the documentation preview: https://649ca04ed8b19e215c607a56--niaefeup-backend-docs.netlify.app

github-actions · 2023-06-28T21:08:33Z

Check the documentation preview: https://649ca1498b322b02fcdd527a--niaefeup-backend-docs.netlify.app

BrunoRosendo

Very good job with this implementation! I left some comments with small suggestions and doubts

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AccountController.kt

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/PassRecoveryDto.kt

src/test/kotlin/pt/up/fe/ni/website/backend/controller/AccountControllerTest.kt

src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/PassRecoveryDto.kt

github-actions · 2023-07-02T02:48:40Z

Check the documentation preview: https://64a0e585de74510aa07bd843--niaefeup-backend-docs.netlify.app

BrunoRosendo

Good job, I left a small comment but I'm pre-approving. It'd be nice if you could review your commits too, maybe squash a few

src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/ChangePasswordDto.kt

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

github-actions · 2023-07-02T16:04:41Z

Check the documentation preview: https://64a1a0168cd57a5fc9cb1bc4--niaefeup-backend-docs.netlify.app

BrunoRosendo

Regarding the latest change, where do you check to see if the hash present in the jwt is the same as the one in the db?

BrunoRosendo · 2023-07-03T00:55:30Z

Regarding the latest change, where do you check to see if the hash present in the jwt is the same as the one in the db?

nevermind this comment, I was looking at the wrong diff

BrunoRosendo

can you create 1/2 tests for the latest change? Maybe 1 where a user tries to use the same recovery link twice and another one just to check for missing hash (if the last one isn't too hard)

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AccountController.kt

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

DoStini · 2023-07-03T14:49:13Z

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

+    fun generateRecoveryToken(@PathVariable id: Long): Map<String, String> {
+        val recoveryToken = authService.generateRecoveryToken(id)
+        // TODO: Change to email service
+        return mapOf("recovery_url" to "$recoverPasswordPage/$recoveryToken")


For security reasons, users should not be able to know if a email is registered in a website, so the request should always be Ok 200

src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/ChangePasswordDto.kt

src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/PasswordRecoveryDto.kt

src/main/kotlin/pt/up/fe/ni/website/backend/service/AccountService.kt

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

src/main/kotlin/pt/up/fe/ni/website/backend/service/ErrorMessages.kt

DoStini · 2023-07-03T15:03:51Z

Overall looking good, just left some comments to improve the RESTiness of the API and privacy concerning issues 🚀

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AccountController.kt

bdmendes · 2023-07-03T16:07:43Z

src/main/kotlin/pt/up/fe/ni/website/backend/service/AccountService.kt

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


What if the hash is not in the payload (since we have that option)? You can simply let it be null and skip the next check if that is the case.

jamcunha · 2023-07-04T18:41:28Z

Just changed the recovery URL request to be provided with the email instead of the id and removed errors related to invalid tokens, logging them instead.

Seems weird to me that this is in a different controller.

I suggest either moving everything to auth controller or to account controller.

Besides that, there are two requests with a similar URL recoverPassword.

I suggest the following routes in AuthController: post -> password/recovery with email in body post -> password/recovery/{token}/confirm post -> password/update

The tests may be lacking because I didn't want to test more without making these changes.

github-actions · 2023-07-05T23:13:29Z

Check the documentation preview: https://64a5f916ff15070f4706ee8a--niaefeup-backend-docs.netlify.app

BrunoRosendo

small changes, Im also waiting for @DoStini response on my comment

BrunoRosendo · 2023-07-05T23:51:48Z

src/main/kotlin/pt/up/fe/ni/website/backend/service/AccountService.kt

    private val fileUploader: FileUploader
-) {
+) : Logging {


Is the logger being used?

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

github-actions · 2023-07-09T17:09:42Z

Check the documentation preview: https://64aae9d16ffd10485fee1ef9--niaefeup-backend-docs.netlify.app

BrunoRosendo

Small comments, besides the one still unresolved

Also, can you find a way to test the uncovered (red) lines? It'd be awesome if it's not too much trouble

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

BrunoRosendo · 2023-07-18T00:52:40Z

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

@@ -57,26 +58,69 @@ class AuthService(
        return generateAccessToken(account)
    }

+    fun generateRecoveryToken(email: String): String? {


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

src/test/kotlin/pt/up/fe/ni/website/backend/controller/AuthControllerTest.kt

...t/kotlin/pt/up/fe/ni/website/backend/utils/documentation/payloadschemas/model/PayloadAuth.kt

jamcunha · 2023-08-01T15:25:05Z

Small comments, besides the one still unresolved

Also, can you find a way to test the uncovered (red) lines? It'd be awesome if it's not too much trouble

I'm having problems trying to test those two lines because when testing token expiration, I try to create a new token with all the claims of the previous token but modifying the expireAt to Instant.now() but it's caught in an exception when trying to decode in confirmRecoveryToken. I think it's the same for passwordHash claim.

github-actions · 2023-08-01T15:36:25Z

Check the documentation preview: https://64c92675c420e6299b59bee0--niaefeup-backend-docs.netlify.app

jamcunha · 2023-08-12T06:31:55Z

For now, I only changed the error handling in the recovery logic so as not to mess up with the tests. After some inputs, I will change the handling for other jwt logic.

BrunoRosendo

Needs a rebase but I'm pre-approving

LuisDuarte1

LGTM 🚀, it looks clean to me, just solve the conflicts below 😅

LuisDuarte1 · 2023-08-30T15:36:25Z

@jamcunha can you fix the conflicts please?

github-actions · 2023-08-30T21:21:15Z

Check the documentation preview: https://64efb2c789311e0a7eba43ac--niaefeup-backend-docs.netlify.app

DoStini · 2023-09-01T13:19:21Z

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

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


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 ""

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

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.

DoStini · 2023-09-01T13:24:56Z

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

-        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)


Why the changes here?

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.

DoStini · 2023-09-01T13:28:26Z

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

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


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

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.

DoStini · 2023-09-01T13:33:19Z

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt

+    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)
+    }


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

DoStini · 2023-09-01T13:42:08Z

src/test/kotlin/pt/up/fe/ni/website/backend/controller/AuthControllerTest.kt

+        fun `should return empty if email is not found`() {
+            mockMvc.perform(
+                post("/auth/password/recovery")
+                    .contentType(MediaType.APPLICATION_JSON)
+                    .content(objectMapper.writeValueAsString(mapOf("email" to "[email protected]")))
+            )
+                .andExpectAll(
+                    status().isOk(),
+                    jsonPath("$.recovery_url").doesNotExist()
+                )
+                .andDocument(
+                    documentation,
+                    "Recover password",
+                    "This endpoint operation allows the recovery of the password of an account, " +
+                        "sending an email with a link to reset the password.",
+                    documentRequestPayload = true
+                )
+        }
+
+        @Test
+        fun `should return password recovery link`() {
+            mockMvc.perform(
+                post("/auth/password/recovery")
+                    .contentType(MediaType.APPLICATION_JSON)
+                    .content(objectMapper.writeValueAsString(mapOf("email" to testAccount.email)))
+            )
+                .andExpectAll(
+                    status().isOk(),
+                    jsonPath("$.recovery_url").exists()
+                ).andDocument(
+                    documentation,
+                    "Recover password",
+                    "This endpoint operation allows the recovery of the password of an account, " +
+                        "sending an email with a link to reset the password.",
+                    documentRequestPayload = true
+                )
+        }


As I explained before, this is not the expected behavior. Both cases will return empty and ok status.

What you should do is test the request returns empty and ok both times, but also to test if the first situation does not call the generate token function. I think the easiest way would be by mocking JwtClaimsSet and verifying if some of those functions do not get called.

Same as above, this could be as is for now and changed when the email service is implemented.
Can leave a comment to remind that.

DoStini · 2023-09-01T13:45:01Z

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt

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


Suggested change

val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)

?: return emptyMap()

val recoveryToken = authService.generateRecoveryToken(recoveryRequestDto.email)

jamcunha added 2 commits February 20, 2023 15:39

added recover request route

b838bc2

added password update route

36812a3

BrunoRosendo requested changes Feb 22, 2023

View reviewed changes

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt Outdated Show resolved Hide resolved

jamcunha added 2 commits February 28, 2023 22:51

added tests

c067a58

added docs

3179ffd

jamcunha commented Jun 26, 2023

View reviewed changes

src/test/kotlin/pt/up/fe/ni/website/backend/controller/AccountControllerTest.kt Outdated Show resolved Hide resolved

BrunoRosendo requested review from DoStini and bdmendes June 28, 2023 15:23

bdmendes reviewed Jun 28, 2023

View reviewed changes

fix documentation

413af20

jamcunha force-pushed the feature/recover-password branch from 0c241bc to 413af20 Compare June 28, 2023 16:04

added a property for backend url

1be6f37

jamcunha marked this pull request as ready for review June 28, 2023 16:23

Merge branch 'develop' into feature/recover-password

3f5ae20

jamcunha force-pushed the feature/recover-password branch from 9f89087 to 3f5ae20 Compare June 28, 2023 21:02

BrunoRosendo requested changes Jun 29, 2023

View reviewed changes

review changes

34666f6

BrunoRosendo approved these changes Jul 2, 2023

View reviewed changes

src/main/kotlin/pt/up/fe/ni/website/backend/dto/auth/ChangePasswordDto.kt Outdated Show resolved Hide resolved

refactor recover password to single use jwt

d6991b1

jamcunha commented Jul 2, 2023

View reviewed changes

src/main/kotlin/pt/up/fe/ni/website/backend/service/AuthService.kt Show resolved Hide resolved

jamcunha mentioned this pull request Jul 2, 2023

Use email service in password recovery #171

Open

BrunoRosendo requested changes Jul 3, 2023

View reviewed changes

DoStini requested changes Jul 3, 2023

View reviewed changes

bdmendes reviewed Jul 3, 2023

View reviewed changes

jamcunha requested review from BrunoRosendo, bdmendes and DoStini July 5, 2023 23:17

BrunoRosendo requested changes Jul 5, 2023

View reviewed changes

DoStini reviewed Jul 6, 2023

View reviewed changes

src/main/kotlin/pt/up/fe/ni/website/backend/controller/AuthController.kt Show resolved Hide resolved

MRita443 requested review from DoStini and BrunoRosendo July 12, 2023 15:33

BrunoRosendo requested changes Jul 18, 2023

View reviewed changes

jamcunha requested a review from BrunoRosendo August 1, 2023 18:27

BrunoRosendo approved these changes Aug 19, 2023

View reviewed changes

LuisDuarte1 self-requested a review August 23, 2023 15:21

LuisDuarte1 approved these changes Aug 23, 2023

View reviewed changes

jamcunha added 3 commits August 26, 2023 01:44

refactored endpoinnts to be email ready

723d8c9

changed endpoint error handling and minor fixes

ac49380

improved jwt token error handling

9a9fa6c

jamcunha force-pushed the feature/recover-password branch from 3e54dda to 9a9fa6c Compare August 26, 2023 00:45

Merge branch 'develop' into feature/recover-password

acac4b3

jamcunha force-pushed the feature/recover-password branch from 9850100 to acac4b3 Compare August 30, 2023 21:12

DoStini requested changes Sep 1, 2023

View reviewed changes

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

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

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

Feature/recover password #113

Are you sure you want to change the base?

Feature/recover password #113

Conversation

jamcunha commented Feb 20, 2023

Review checklist

BrunoRosendo left a comment

Choose a reason for hiding this comment

codecov bot commented May 3, 2023 • edited Loading

Codecov Report

BrunoRosendo commented Jun 23, 2023

bdmendes left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 28, 2023

github-actions bot commented Jun 28, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 2, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 2, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

BrunoRosendo commented Jul 3, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoStini commented Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

jamcunha commented Jul 4, 2023

github-actions bot commented Jul 5, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamcunha commented Aug 1, 2023 • edited Loading

github-actions bot commented Aug 1, 2023

jamcunha commented Aug 12, 2023

BrunoRosendo left a comment

Choose a reason for hiding this comment

LuisDuarte1 left a comment

Choose a reason for hiding this comment

LuisDuarte1 commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 3, 2023 •

edited

Loading

DoStini commented Jul 3, 2023 •

edited

Loading

jamcunha commented Aug 1, 2023 •

edited

Loading