Allow linking and unlinking lambdas to applications #8
Conversation
rideam
commented
Aug 21, 2023
•
rideam
commented
- Change adds more lambda commands and allows lambdas to be defined as multiline when uploading
…en JWT for an application
Use yaml instead of json to allow multiline lambda bodies in files
|
@ColinFrick can you please take a look? |
There was a problem hiding this comment.
@rideam Great work!
There are some potential problems in the implementation.
Currently only JWT populate lambdas are supported for the un/link commands, and they are always set on both accessToken and idToken settings. Should this be an option for the commands? Or is this unnecessary?
As soon as the commands are released, we would have to honor backwards compatibility (or release a major update). 😅
| request.application!.lambdaConfiguration!.accessTokenPopulateId = null; | ||
| else | ||
| request.application!.lambdaConfiguration!.accessTokenPopulateId = accessTokenPopulateId; | ||
| if (idTokenPopulateId && idTokenPopulateId == lambdaId) | ||
| request.application!.lambdaConfiguration!.idTokenPopulateId = null; |
There was a problem hiding this comment.
Could you set it to undefined instead of null?
TS2322: Type null is not assignable to type string | undefined
There was a problem hiding this comment.
Could you add brackets to this if statements? It's hard to read.
There was a problem hiding this comment.
@ColinFrick Cool will fix this. I forgot to mention I had opened a PR on the typescript client FusionAuth/fusionauth-typescript-client#86 to allow for null which this PR depends on. Though after Dan's comment I realised that change cannot be made directly.
There was a problem hiding this comment.
Setting the properties to undefined and updating the application with patchApplication works for me
There was a problem hiding this comment.
setting to an empty string seems to work better to reset the value otherwise using undefined will not actually update the property
|
|
||
| // noinspection JSUnusedGlobalSymbols | ||
| export const lambdaCreate = new Command('lambda:create') | ||
| .description(`Create a lambda on FusionAuth. |
There was a problem hiding this comment.
Please add a short .summary('Create a lambda on FusionAuth') for commands with a large description. (lamdba:create, lambda:link-to-application, lamdba:unlink-from-application)
| } | ||
| }; | ||
| const fusionAuthClient = new FusionAuthClient(apiKey, host); | ||
| const clientResponse = await fusionAuthClient.updateApplication(applicationId, request) |
There was a problem hiding this comment.
This is dangerous. As the API documentation states, with PUT / updateApplication you must specify all of the properties of the Application. This will cause data loss in the application configuration.
Use patchApplication instead, then you can also drop the name property in the request.
| request.application!.lambdaConfiguration!.idTokenPopulateId = idTokenPopulateId; | ||
|
|
||
| const fusionAuthClient = new FusionAuthClient(apiKey, host); | ||
| const clientResponse = await fusionAuthClient.updateApplication(applicationId, request) |
There was a problem hiding this comment.
Same problem as before, use patchApplication
…se patchApplication instead of updateApplication
…as needed only in updateApplication
…pdate the application in cases where Typescript does not allow nulls. Origin: FusionAuth/fusionauth-node-cli#8
|
I think it would be a lot of work to support linking and unlinking lambdas across all their locations. I didn't consider the ramifications (both for initial implementation and ongoing support--every time a new lambda is added, we'd have to update this tool). Since this tool is really about making it easier to develop custom components for FusionAuth, any issues with removing the link/unlink commands? @ColinFrick @rideam |
|
@mooreds no issues removing the link/unlink commands,, I will just update the lambda testing article to remove where they are mentioned. Let me know what you decide. |
