Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
* Add test cases for controllers * Update maven.yml with SUPPORTED_PKCS10_MIN_MODULUS * Update maven.yml * Update maven.yml * Add application.properties to run test * Fix test * Remove unused import * Revert the user CN change
Sae126V
left a comment
Sae126V
left a comment
There was a problem hiding this comment.
The code flow is looking good to me. Just a few things mostly Q&A and minor fixes.
| * @throws java.io.IOException | ||
| */ | ||
| @RequestMapping(value = "/changeroletouser", method = RequestMethod.POST) | ||
| public String changeRoleCertificate(@RequestParam long cert_key, RedirectAttributes redirectAttrs) |
There was a problem hiding this comment.
Should we add @Transactional here to allow rollbacks if any errors occur ? - What I mean is I think we should add @Transactional for a method where it is making the DB calls. I will leave it in your fine hands to decide that, Manoj .
There was a problem hiding this comment.
As discussed, since we are not dealing with multiple databases call that require coordinated rollback on failure, the use of @transactional is unnecessary
| return false; | ||
| } | ||
|
|
||
| // Compare DN attributes (OU and L) between current user and target certificate |
There was a problem hiding this comment.
Do we need the Organization(O) comparison as well. I might be wrong, here.
There was a problem hiding this comment.
Not included as other part of the code not using Organization(O).
| */ | ||
| @Secured("ROLE_CAOP") | ||
| @RequestMapping(value = "/approverolechange", method = RequestMethod.POST) | ||
| public String approverolechange(@RequestParam long certKey, @RequestParam Integer requestId, |
There was a problem hiding this comment.
Same as previous, @Transactional perhaps because we are making a commits to the DB ?
| * | ||
| * @param dataSource | ||
| */ | ||
| @Autowired |
There was a problem hiding this comment.
We need to remove the unused imports here. I think same goes for other *Dao.java files as well.
| @@ -0,0 +1,17 @@ | |||
| package uk.ac.ngs.dao; | |||
| @@ -0,0 +1,77 @@ | |||
| package uk.ac.ngs.domain; | |||
There was a problem hiding this comment.
Shall we add copyright here ? Also, In *Tests.java files there few places we might need to add.
| String targetCertOU = CertUtil.extractDnAttribute(targetCert.getDn(), CertUtil.DNAttributeType.OU); | ||
| String targetCertL = CertUtil.extractDnAttribute(targetCert.getDn(), CertUtil.DNAttributeType.L); | ||
|
|
||
| return currentUserOU.equals(targetCertOU) && currentUserL.equals(targetCertL); |
There was a problem hiding this comment.
I am leaving up to you, Manoj. Should we add null-safe comparison here ?
| CertificateRow requestedBy = roleChangeRequestRepository.findById(requestId) | ||
| .map(RoleChangeRequest::getRequestedBy) | ||
| .map(jdbcCertDao::findById) | ||
| .orElse(null); |
There was a problem hiding this comment.
Since requestedBy can be null. I think we should add a null check before getting the data when the methods like sendEmailNotificationOnApproval, sendEmailNotificationOnRejection and other places where the method receives the variable
|
LGTM!...🚀 |
2 participants
Role assignment interface for RA Operators
Notification system for role changes request and approval
Page for request approval for CA Admins
Database update upon approval
Role downgrade functionality for RA Operators
Page for RAOP
Page for CAOP
