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/auth #54

Merged
merged 18 commits into from
Dec 7, 2022
Merged

Feature/auth #54

merged 18 commits into from
Dec 7, 2022

Conversation

bdmendes
Copy link
Member

Closes #12

Review checklist

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

@bdmendes bdmendes closed this Nov 17, 2022
@bdmendes bdmendes reopened this Nov 17, 2022
@bdmendes bdmendes force-pushed the feature/auth branch 2 times, most recently from 323b2f7 to 199c08c Compare November 17, 2022 12:00
@bdmendes
Copy link
Member Author

The stateless implementation is working. We still need to save state, either to invalidate tokens or to link tokens to users for a manual check of permissions. How would you approach this @limwa?

@bdmendes bdmendes force-pushed the feature/auth branch 2 times, most recently from 526a51f to 6097d85 Compare November 29, 2022 18:38
@bdmendes
Copy link
Member Author

After some research, I found that JWT with total stateless behavior is the best way to go (see https://stackoverflow.com/questions/6068113/do-sessions-really-violate-restfulness).
This implementation uses access and refresh tokens. An access token contains the user roles and is to be used as a bearer token in the Authorization header. It has a short (configurable) expiration time of 60 minutes, so that invalid roles aren't allowed for much time after they get removed. For improving the user's UX on the frontend (and not force the frontend to save the password), it will save a refresh token that will be valid for 7 days and will be used to generate a new access token (with updated rules and more 60 minutes) every time it needs to. This refresh stuff should really be implemented in some Vue/React/wtv lib, although I did not do much research on this. In the worst case scenario, we can simply create our own thin wrapper around requests logic.

@bdmendes bdmendes marked this pull request as ready for review November 29, 2022 18:53
@bdmendes
Copy link
Member Author

I'll soon add a custom message for invalid tokens. Meanwhile, everything that is important is done, so this is ready for review

@bdmendes
Copy link
Member Author

bdmendes commented Dec 3, 2022

This is fully ready

Copy link
Member

@BrunoRosendo BrunoRosendo left a comment

Choose a reason for hiding this comment

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

This is a really great job! Amazing PR, this will be extremely useful going forward 🚀

That said, I wrote quite a few comments, mostly asking questions since this is a complex PR. I also left some suggestions regarding smaller details.

fun `should return access and refresh tokens`() {
mockMvc.post("/auth/new") {
contentType = MediaType.APPLICATION_JSON
content = objectMapper.writeValueAsString(LoginDto(testAccount.email, testPassword))
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 use the same method here and in the test above. I personally like the first one more because it doesn't depend on the Dto implementation but it can also be more verbose

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'll convert the first to this one since I prefer the less verbose approach

Comment on lines 51 to 54
throw InvalidBearerTokenException("invalid refresh token")
}
if (jwt.expiresAt?.isBefore(Instant.now()) != false) {
throw ResponseStatusException(HttpStatus.UNAUTHORIZED, "refresh token has expired")
throw InvalidBearerTokenException("refresh token has expired")
Copy link
Member

Choose a reason for hiding this comment

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

I didn't test this on my own so I'm going to ask. I like the change but is this exception being handled or does it just return an unexpected error?

Copy link
Member Author

@bdmendes bdmendes Dec 6, 2022

Choose a reason for hiding this comment

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

Yes, it is an AuthenticationException. This is being tested, too

Copy link
Member

@BrunoRosendo BrunoRosendo left a comment

Choose a reason for hiding this comment

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

Great job! :)

@bdmendes bdmendes merged commit abba3d1 into develop Dec 7, 2022
@bdmendes bdmendes deleted the feature/auth branch December 7, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

accounts: implement authentication
3 participants