IMN-616 BFF Tools router #976
Conversation
…com/pagopa/interop-be-monorepo into IMN-522_client-assertion-validation
…lient-assertion-validation
…com/pagopa/interop-be-monorepo into IMN-522_client-assertion-validation
…N-798_catalog-platformstate-writer-v2
…atalog-platformstate-writer-v1
| data: { | ||
| clientKind: authorizationApi.ClientKind.enum.API, | ||
| kid: jwt.header.kid, | ||
| algorithm: "RS256", |
There was a problem hiding this comment.
Pass the algorithm from the key (you can extract it where you extract encodedPem), the validation of this value is on the client assertion side when it performs validations on the key, later in the code.
There was a problem hiding this comment.
We have the same problem as the request params here: the only accepted value for ApiKey and ConsumerKey is a const of "RS256".. What should we do here?
There was a problem hiding this comment.
Could you modify the client assertion to accept a generic string for the types ApiKey and ConsumerKey in this same PR?
There was a problem hiding this comment.
@paologaleotti @taglioni-r if we change the headers.alg inside ClientAssertion (returned by verifyClientAssertion function) to a branded type we should solve this, right?
Then use the same type in ApiKey and ConsumerKey.
But let me know if I'm missing any conflict
| clientKind: authorizationApi.ClientKind.enum.CONSUMER, | ||
| clientId: unsafeBrandId<ClientId>(jwt.payload.iss), | ||
| kid: jwt.header.kid, | ||
| algorithm: "RS256", |
There was a problem hiding this comment.
Pass the algorithm from the key (you can extract it where you extract encodedPem), the validation of this value is on the client assertion side when it performs validations on the key, later in the code.
| ): Promise< | ||
| SuccessfulValidation<ApiKey | ConsumerKey> | FailedValidation<ErrorCodes> | ||
| > { | ||
| if (!jwt.payload.purposeId) { |
There was a problem hiding this comment.
the missing purposeId is not an error because we have 2 kinds of tokens that can be generated:
- eservice Voucher - needs the purposeId
- m2m jwt - does not need the purposeId (and it is not related to states)
The debugger should be able to handle both.
If the purposeId is missing the eservice is not retrieved/returned and platform states check is skipped (handled inside the validateClientKindAndPlatformState function).
TL;DR: this check is required only if the client kind is CONSUMER
There was a problem hiding this comment.
Perfect, thank you. The only doubt i have is: if the client kind is API, a purposeId is still required in the object. In this case what should i use as purposeId?
There was a problem hiding this comment.
you're right, it seems an erro on the Key parent type.
@taglioni-r can you check? the purposeId should be only in ConsumerKey
|
|
||
| assertIsConsumer(ctx.authData.organizationId, keyWithClient); | ||
|
|
||
| const { encodedPem } = await authorizationClient.client.getClientKeyById({ |
There was a problem hiding this comment.
suggestion for another PR: getKeyWithClientByKeyId is now used only here, we could change its spec and return the encodedPem instead of the JWK, so we would not need another call
| } | ||
|
|
||
| function retrievePurposeItemState(purpose: purposeApi.Purpose): ItemState { | ||
| const activePurposeVersion = [...purpose.versions] |
There was a problem hiding this comment.
we should take the last version with a state in ACTIVE, SUSPENDED, ARCHIVED.
We could centralize the state logic by converting all states with purposeVersionStateToItemState, taking the ACTIVE if exists, otherwise return INACTIVE
There was a problem hiding this comment.
Correct me if I'm wrong, but with the current sort + find and these states
- Version1: ARCHIVED
- Version2: ACTIVE
the function wouldn't return the Archived version?
There was a problem hiding this comment.
In this case it will just return the first one it finds. This differs from the retrieveAgreement as in that the active or suspended have priority. Should this be the logic also in this function?
| return purposeVersionStateToItemState(activePurposeVersion.state); | ||
| } | ||
|
|
||
| async function retrieveTokenValidationEService( |
There was a problem hiding this comment.
this function and retrieveKey make the same call to retrieve the same values, can we re-use them?
Note: the agreement is retrieved using 2 different approaches
There was a problem hiding this comment.
I tried but i don't think it is cleaner, the agreement is retrieved differently and also the descriptor (each one throws a different error). Maybe there is a way to write a more generic function so each one can use it but it can make the code a lot less readable.. let me know 👍🏻
…rop-be-monorepo into IMN-616_tools_router
Merge after #956 !!!
This PR adds the Tools router to BFF
The implementation differs a bit from scala (see this slack thread https://buildo.slack.com/archives/C0527LU1K4G/p1726583989026809).
When reviewing please pay particular attention to the logic behind the error handling (
handleValidationResults) andretrieveKey, as it is not a direct porting of scala logic.