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

Added JWT authentication with Spring Security 3.2.2 #17

Merged
merged 41 commits into from
Feb 9, 2024

Conversation

Toto-hitori
Copy link
Contributor

An initial Spring Boot project has been created with Spring Initializr and the following features have been added:

  • Sign-up endpoint "/register"
  • Login endpoint "/login"
  • Token refresh endpoint "/refresh-token"
  • Initial CORS configuration

The Spring Security configuration is not final and may be subject to changes in the future.
If there is any doubts about the implementation feel free to reach out or leave a comment in this pull request.

@Toto-hitori
Copy link
Contributor Author

I forgot to mention, the already existing node.js API has not been deleted in this pull request. If you wish me to do it comment it below.

@jjgancfer
Copy link
Contributor

jjgancfer commented Feb 7, 2024

Just performed a quick static review of the code, and I have some things I'd like you to consider:

  1. It may sound hypocritical of me given I did not provide tests either in feat/initial webapp routing #14, but is there any reason why no testing was performed? I mean, the webapp router is kind of hard to test separately, so I just decided to test it once we had a functional UI, but is there any reason not to provide automated testing?
  2. Regarding @Autowired attributes, I think we should (in the future) avoid using it due to it automatically injecting an object of the given class. Though good for fast development, I really can not see how we can independently test each component if we use it.
  3. Is there any specific reason for the different loggers? For instance, in CustomControllerAdvice.java you are using lombok.extern.log4j.Log4j2 but in JwtUtils.java org.slf4j.Logger is used instead. Related to that, the autogenerated .gitignore currently uploads log files, so if other kind of logging is later added it may be convenient to add it.
  4. Please change the destiny branch from develop to initial-modifications, as discussed in Creating new branch for source modifications before first deliverable #13 .

That's about it. Other than that, I don't really see any other problem or question. Good job!

Edit: add comment 4.

@Toto-hitori
Copy link
Contributor Author

Just performed a quick static review of the code, and I have some things I'd like you to consider:

1. It may sound hypocritical of me given I did not provide tests either in [feat/initial webapp routing #14](https://github.com/Arquisoft/wiq_en2b/pull/14), but is there any reason why no testing was performed? I mean, the webapp router is kind of hard to test separately, so I just decided to test it once we had a functional UI, but is there any reason not to provide automated testing?

2. Regarding `@Autowired` attributes, I think we should (in the future) avoid using it due to it automatically injecting an object of the given class. Though good for fast development, I really can not see how we can independently test each component if we use it.

3. Is there any specific reason for the different loggers? For instance, in `CustomControllerAdvice.java` you are using `lombok.extern.log4j.Log4j2` but in `JwtUtils.java` ` org.slf4j.Logger` is used instead. Related to that, the autogenerated `.gitignore` currently uploads log files, so if other kind of logging is later added it may be convenient to add it.

4. Please change the destiny branch from `develop` to `initial-modifications`, as discussed in [Creating new branch for source modifications before first deliverable #13](https://github.com/Arquisoft/wiq_en2b/issues/13) .

That's about it. Other than that, I don't really see any other problem or question. Good job!

Edit: add comment 4.

Regarding 1 and 2, I have plans for adding unit testing and removing @Autowired annotations, although my objective for this pull request was to have a functional JWT authentication module and prove that we could use Java Spring Boot for the rest of our application. Maybe issues should be created for this tasks.
3 and 4 have been solved.

When suggesting changes in the future try to use the Github comments on the specific lines so it is easier to find the problems with the code. (look at this link for reference)

@Toto-hitori Toto-hitori changed the base branch from develop to initial-modifications February 7, 2024 08:21
Copy link

sonarcloud bot commented Feb 7, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@UO283615
Copy link
Contributor

UO283615 commented Feb 9, 2024

For me this is good to merge as a start.
To add on what Jorge said, for point 1, yes, tests could be done but I don't really see any meaningful part to test, maybe that the token is properly generated (checking that it is a long enough string? and also that the fields of the user are properly stored?
For the second point, it is true that field based injection and it makes it harder to test, so it would be necessary to pass the services in the constructor, which should be an easy fix and I think would work nicely.
As I said, I believe we should merge this to have things working and we could work in these little issues afterwards, specially the tests.

@UO283615 UO283615 merged commit 72a2a8e into initial-modifications Feb 9, 2024
2 checks passed
@jjgancfer jjgancfer deleted the feat/spring-api branch February 14, 2024 17:56
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.

3 participants