Add KRA KeyRequestService to v2 APIs #4821
Conversation
b27d24c to
df22cd9
Compare
df22cd9 to
9740d25
Compare
| "approve")); | ||
|
|
||
| } catch (EAuthzAccessDenied e) { | ||
| throw new UnauthorizedException("Not authorized to approve request", e); |
There was a problem hiding this comment.
This is identical to the original code, but do you think we should log an audit event here? No need to fix the code now, but maybe just add a TODO comment for a future review.
There was a problem hiding this comment.
In this exception (and some other) there is no audit failure while in other there is. I have added the TODO in all cases for future evaluation.
In my opinion this should not generate a failure event for the operation in the audit because the authorisation stop any processing before it start. An authorisation failure event could be possible but not one related to the following operation.
An improvement of this would be to move the authorisation check in the ACL filter and decide what the audit should include for the authorisation event.
| id, | ||
| "reject")); | ||
| }catch (EAuthzAccessDenied e) { | ||
| throw new UnauthorizedException("Not authorized to reject request", e); |
| id, | ||
| "cancel")); | ||
| } catch (EAuthzAccessDenied e) { | ||
| throw new UnauthorizedException("Not authorized to cancel request", e); |
| } | ||
| } | ||
|
|
||
| private KeyRequestResponse generateASymKey(Principal userPrincipal, String baseUrl, AsymKeyGenerationRequest data) { |
There was a problem hiding this comment.
IIUC this method is generating an asymmetric key (as opposed to a symmetric key), so the method probably should be called generateAsymKey() with lower case s to avoid confusions.
| return createKeyRequestResponse(request, baseUrl); | ||
| } | ||
|
|
||
| private KeyRequestResponse submitGenerateASymKeyRequest(AsymKeyGenerationRequest data, String userName, String baseUrl) throws EBaseException { |
There was a problem hiding this comment.
Same thing here with AsymKey instead of ASymKey.
| RequestId requestID; | ||
| if (ephemeral) { | ||
| requestID = createEphemeralRequestID(); | ||
| } else { | ||
| requestID = requestRepository.createRequestID(); | ||
| } |
There was a problem hiding this comment.
No issue here, I just want to point out that ephemeral request ID is probably no longer needed with RSNv3, so it's something that can be removed in the future.
9740d25 to
525435b
Compare
|
@edewata Thanks! I have update the method name and added the |
|
There is a difference related to the start parameter for the list request. In the v1 APIs this is the RequestId to start from but here it is just an index. A similar change was implemented in #4672 and it is in the right direction moving from sequential to random ids.