ion icon indicating copy to clipboard operation
ion copied to clipboard

fix: error on creating Cognito authorizers for V1 AWS APIs

Open greenlynx opened this issue 1 year ago • 5 comments

Fixes the error discussed in https://github.com/sst/ion/issues/1144

greenlynx avatar Oct 01 '24 10:10 greenlynx

Thanks

jayair avatar Oct 04 '24 20:10 jayair

I've got this same issue in V1,

This is causing an issue when trying to create a Cognito Authoriser. When setting up a Cognito Authoriser we do not required a function, and in fact errors before this line if you try to add a token/request Function.

fn? will be rightly undefined in this case, and should not be force to be non null.

If I revert this change, and do the below, my authoriser is setup correctly and works.

api.addAuthorizer({
    name: "my-authorizer",
    userPools: [myUserPoolArn],
  });

jamesleech avatar Oct 07 '24 23:10 jamesleech

it does look like this issue could also be in V2 (http) of the apigateway function also.

jamesleech avatar Oct 08 '24 06:10 jamesleech

I haven't tested it, but I don't think this is an issue in V2 API Gateway - in there, as the code currently stands, it looks like fn will only be undefined if lamb is undefined, and the fn! line is only hit if lamb is truthy. I think there's an argument to be made for avoiding the use of the non-null assertion operator as far as possible, to avoid bugs creeping in inadvertantly in future if the logic changes - but I'd rather keep this PR focused on fixing the specific bug we're hitting.

Maintainers - are you happy with this PR? Do I need to do anything further to get it merged?

greenlynx avatar Oct 09 '24 18:10 greenlynx

I haven't tested it, but I don't think this is an issue in V2 API Gateway - in there, as the code currently stands, it looks like fn will only be undefined if lamb is undefined, and the fn! line is only hit if lamb is truthy. I think there's an argument to be made for avoiding the use of the non-null assertion operator as far as possible, to avoid bugs creeping in inadvertantly in future if the logic changes - but I'd rather keep this PR focused on fixing the specific bug we're hitting.

Maintainers - are you happy with this PR? Do I need to do anything further to get it merged?

"argument to be made for avoiding the use of the non-null assertion operator as far as possible" 100% Agree.

Would appreciate this PR being released, as I've currently out of scoped auth for a specific API route because of this bug for a project I'm working on to carry on to meet project deadlines, we'll loop back once this is merged and released.

jamesleech avatar Oct 10 '24 00:10 jamesleech

Would appreciate this PR being released, as I've currently out of scoped auth for a specific API route because of this bug for a project I'm working on to carry on to meet project deadlines, we'll loop back once this is merged and released.

Me too, as it's blocking me as well. I don't know how to proceed though - I've never contributed to this project before and am not sure of the process.

greenlynx avatar Oct 14 '24 08:10 greenlynx

Will be in v3.2.29 - sorry about the delay!

fwang avatar Oct 16 '24 15:10 fwang

thanks @fwang ! great news

jamesleech avatar Oct 17 '24 00:10 jamesleech