Skip to content

feat: security utils and setting userid on jwt#28910

Closed
OAllanFernando wants to merge 10 commits intojhipster:mainfrom
OAllanFernando:feat/user-id-accessible
Closed

feat: security utils and setting userid on jwt#28910
OAllanFernando wants to merge 10 commits intojhipster:mainfrom
OAllanFernando:feat/user-id-accessible

Conversation

@OAllanFernando
Copy link
Contributor

this makes possible recover the current user id by the jwt, its useful and can save requests

@CLAassistant
Copy link

CLAassistant commented Mar 9, 2025

CLA assistant check
All committers have signed the CLA.

@OAllanFernando
Copy link
Contributor Author

OAllanFernando commented Mar 9, 2025

Avoid this behavior

@GetMapping("/bank-accounts/{id}")
@Timed
public ResponseEntity<BankAccountDTO> getBankAccount(@PathVariable Long id) {
    log.debug("REST request to get BankAccount : {}", id);
    Optional<BankAccountDTO> bankAccountDTO = bankAccountRepository.findById(id)
        .map(bankAccountMapper::toDto);

    // Return 404 if the entity is not owned by the connected user
    Optional<String> userLogin = SecurityUtils.getCurrentUserLogin();
    if (bankAccountDTO.isPresent() &&
        userLogin.isPresent() &&
        userLogin.get().equals(bankAccountDTO.get().getUserLogin())) {
        return ResponseUtil.wrapOrNotFound(bankAccountDTO);
    } else {
        return ResponseEntity.notFound().build();
    }
}

setting this as solution

@GetMapping("/bank-accounts/{login:" + Constants.LOGIN_REGEX + "}")
@Timed
public ResponseEntity<BankAccountDTO> getBankAccount(@PathVariable String login) {
    log.debug("REST request to get BankAccount of : {}", login);
    
    // We can also validate login. Double security
   Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
    if(!Objects.equals(login, authentication){
         return ResponseEntity.notFound().build();
    };

    Long userId = SecurityUtils.getCurrentUserId();
    if (userId == null) {
        return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
    }

    return bankAccountRepository.findById(userId)
        .map(bankAccountMapper::toDto)
        .map(ResponseEntity::ok)
        .orElseGet(() -> ResponseEntity.notFound().build());
}
}

This aproch can mitigate data transfer, clowd does not need to be our associates, rigth ?

The previos code can be used to force the server, if the data is in a different server

@duongxinh2003
Copy link

@OAllanFernando Can this PR be merged?

@OAllanFernando
Copy link
Contributor Author

OAllanFernando commented Mar 26, 2025

Hi, @duongxinh2003
if anyone can review the code and try to fix the tests, yes, my only concern is the possibility of do not generate builtIn user entity, in that case it will fail because it needs the user service at all, but I think that can be a good idea for jhipster. When i got some time maybe i try.
Are you trying to use it in your project? You can just copy the methods I made and use it, in my project it works very well

@duongxinh2003
Copy link

@OAllanFernando Yes, I am currently customizing the JWT token by adding claims similar to yours. I also hope there will be an update that allows extracting the userId directly from the JWT, and I have found this commit. Hopefully, this PR will be merged, or at least there will be a future update that enables retrieving the userId.

@Rakoski
Copy link

Rakoski commented Mar 29, 2025

Hello @OAllanFernando . Hopefully this gets merged, I'm also having the same issue on one of my projects

@Gabriel-S-O
Copy link

Not having this feature yet is a pain in the ass... Thankfully someone is trying to implement it :)

@mshima
Copy link
Member

mshima commented Mar 29, 2025

Tests are not passing.

}

Optional<<%= user.persistClass %>> userOptional = userService.getUserWithAuthoritiesByLogin(authentication.getName());
Long userId = userOptional.map(User::getId).orElse(null);
Copy link
Member

Choose a reason for hiding this comment

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

userId should be passed through parameters due to reactive implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

about this, only the integration of the id in the token or should the whole implementation be in the reactive?

I did only on the integration, as you said
and what would be the reactive?

Copy link
Member

Choose a reason for hiding this comment

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

We provide reactive and imperative implementations.
So the createToken signature should be:

public String createToken(Authentication authentication, <%= user.primaryKey.type %> userId, boolean rememberMe) {

And userId retrieval should be in authorize method and then pass to createToken.

Copy link
Contributor Author

@OAllanFernando OAllanFernando Mar 31, 2025

Choose a reason for hiding this comment

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

we pass userService to the constructor in reactive line 86, so I inserted the search for the id in both possibilities line 91 and 123, in
this case we no longer need it, the reactive condition on createToken

     <%_ if (!reactive) { _%>
         if (userId != null) {
             claimsBuilder.claim(USER_ID_CLAIM, userId);
         }
         <%_ } _%>

Rigth?

validity = now.plus(this.tokenValidityInSeconds, ChronoUnit.SECONDS);
}

<%_ if (reactive) { _%>
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
<%_ if (reactive) { _%>
<%_ if (!reactive) { _%>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still necessary? Or do I leave the id only for !reactive flow?

}

Optional<<%= user.persistClass %>> userOptional = userService.getUserWithAuthoritiesByLogin(authentication.getName());
Long userId = userOptional.map(User::getId).orElse(null);
Copy link
Member

Choose a reason for hiding this comment

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

We provide reactive and imperative implementations.
So the createToken signature should be:

public String createToken(Authentication authentication, <%= user.primaryKey.type %> userId, boolean rememberMe) {

And userId retrieval should be in authorize method and then pass to createToken.

@mshima
Copy link
Member

mshima commented Mar 31, 2025

@OAllanFernando there is a cleaner way to implement using UserDetails.
I will create a PR.

@DanielFran
Copy link
Member

Superseded by #29108
Thanks anyway @OAllanFernando

@DanielFran DanielFran closed this Apr 2, 2025
@mraible mraible added this to the 8.11.0 milestone May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants