Make cookies extraction on AWS Lambda compatible with its format v1.0

[tattee]

I tried to deploy a slack bolt application on AWS Lambda with SQLAlchemy.
In the process of the verification of oauth redirect, the app could not be installed properly.
I found that the slack request handler could not extract cookie from the lambda event.
So, I modified the to_bolt_request to meet the format of the lambda event.

[CLAassistant]

All committers have signed the CLA.

[seratch]

Hi @tattee, thank you very much for takin the time to make this pull request. Before my code review, I have two things to ask:

• Can you associate the email address that you use for git commits with your GitHub account?
• And then, would you mind signing our CLA (see the above message by the CLA bot)?

Even if your changes are great, we are unable to merge them without signed CLA. I would appreciate it if you could understand this.

[seratch]

HI @tattee, thanks for taking the time to make this pull request. Can you check this issue #332 (comment) ? This project supports only AWS lambda event payload format v2.0 as the HTTP API is the currently recommended way plus it is the greatest fit for running Bolt apps.

With that being said, we are open to support format v1.0 as well. If you have more time, could you help us add the support by making the following additional work?

• Add unit tests verifying if it works with v1.0 format
  • We already have tests with v2.0 format here, you can add more tests in the same file
  • The tests should have a realistic payload
• Update the document after merging the change (I can do this later)

[tattee]

@seratch thank you for telling me the issue #322.
I looked at the unit test code for aws lambda handler and found that the test function named "test_oauth" only tests the process of outh_flow.handle_installation().
So, I added new test method named "test_oauth_redirect".
The response from SlackRequestHandler only returns success or failure of authentication, and it does not help understand whether urlStringParameters are extracted properly.
Fortunately, in the process of outh_flow.handle_callback(), the verification of the value of "state" is taken place(in oauth_flow.py).
If the value of state is correct, the statusCode of reponse changes from 400 to 401.
Although the authentication does not succeed, you can verify that the parameters in the event were read correctly by checking that the status code value changes to 401.

I understand that this is not ideal...
If my test code is OK, I will sent a pull request again.

[seratch]

@tattee Thanks for taking the time to work on it. Ideally, we should have some tests for /slack/oauth_redirect handling but the test does not exist yet. Thus, we don't need to add a new test pattern in this pull request. We can improve it next time.

My previous comment might not be clear enough but the tests we need this time is the same patterns with the ones for the already supported AWS payload format. Considering the number of existing test methods, adding the same to the same file may not be great. How about having test_aws_lambda_format_v1.py and test_aws_lambda_format_v2.py? Having two files should be far easier to understand the intention and maintain.

[tattee]

@seratch In my understanding, the format of the events in test_oauth are v2.0 (first one) and v1.0 (second one) respectively, and the processes in the tests are almost same.
I'm afraid dividing the test file would increase mental burden.

If my concern does not make sense, please ignore it.

[seratch]

Ah thanks. I totally forgot these tests already support both formats. So, this means that only cookies extraction is not yet fully supported for both formats, right? In the case, trying to check /slack/oauth_redirect pattern is reasonable. I'm sorry about my confusion. I will add a few comments to the changes.

[seratch]

If we have a mock state_store, which returns a fixed value "uuid4-value", the test can be better. Can you check this?

[tattee]

Sorry, I could not understand where to check.
I would be grateful if you could give me specific instructions.
(I'm not familiar with @mock_lambda in moto, and you said about it??)

[seratch]

@tattee Thanks for asking this! I thought that we need to implement state_store with issue/consume methods (returning a fixed value "uuid4-value" means having the issue method). But, for this test, having only consume method is required. Thus, the following diff should work for you!

diff --git a/tests/adapter_tests/aws/test_aws_lambda.py b/tests/adapter_tests/aws/test_aws_lambda.py
index b92300e..1b0166c 100644
--- a/tests/adapter_tests/aws/test_aws_lambda.py
+++ b/tests/adapter_tests/aws/test_aws_lambda.py
@@ -3,6 +3,7 @@ from time import time
 from urllib.parse import quote
 
 from moto import mock_lambda
+from slack_sdk.oauth import OAuthStateStore
 from slack_sdk.signature import SignatureVerifier
 from slack_sdk.web import WebClient
 
@@ -321,6 +322,10 @@ class TestAWSLambda:
 
     @mock_lambda
     def test_oauth_redirect(self):
+        class TestStateStore(OAuthStateStore):
+            def consume(self, state: str) -> bool:
+                return state == "uuid4-value"
+
         app = App(
             client=self.web_client,
             signing_secret=self.signing_secret,
@@ -328,6 +333,7 @@ class TestAWSLambda:
                 client_id="111.111",
                 client_secret="xxx",
                 scopes=["chat:write", "commands"],
+                state_store=TestStateStore(),
             ),
         )
 
@@ -340,7 +346,7 @@ class TestAWSLambda:
             "isBase64Encoded": False,
         }
         response = SlackRequestHandler(app).handle(event, self.context)
-        assert response["statusCode"] == 401
+        assert response["statusCode"] == 200
         assert response["headers"]["content-type"] == "text/html; charset=utf-8"
         assert response.get("body") is not None
 
@@ -353,6 +359,6 @@ class TestAWSLambda:
             "isBase64Encoded": False,
         }
         response = SlackRequestHandler(app).handle(event, self.context)
-        assert response["statusCode"] == 401
+        assert response["statusCode"] == 200
         assert response["headers"]["content-type"] == "text/html; charset=utf-8"
         assert response.get("body") is not None

[tattee]

I checked your comments, and left replies.
Best regards,

[tattee]

Sorry, I could not understand where to check.
I would be grateful if you could give me specific instructions.
(I'm not familiar with @mock_lambda in moto, and you said about it??)

[seratch]

Thanks for updating this PR. Once the test is improved, we can merge your PR 👍

[seratch]

@tattee Thanks for asking this! I thought that we need to implement state_store with issue/consume methods (returning a fixed value "uuid4-value" means having the issue method). But, for this test, having only consume method is required. Thus, the following diff should work for you!

diff --git a/tests/adapter_tests/aws/test_aws_lambda.py b/tests/adapter_tests/aws/test_aws_lambda.py
index b92300e..1b0166c 100644
--- a/tests/adapter_tests/aws/test_aws_lambda.py
+++ b/tests/adapter_tests/aws/test_aws_lambda.py
@@ -3,6 +3,7 @@ from time import time
 from urllib.parse import quote
 
 from moto import mock_lambda
+from slack_sdk.oauth import OAuthStateStore
 from slack_sdk.signature import SignatureVerifier
 from slack_sdk.web import WebClient
 
@@ -321,6 +322,10 @@ class TestAWSLambda:
 
     @mock_lambda
     def test_oauth_redirect(self):
+        class TestStateStore(OAuthStateStore):
+            def consume(self, state: str) -> bool:
+                return state == "uuid4-value"
+
         app = App(
             client=self.web_client,
             signing_secret=self.signing_secret,
@@ -328,6 +333,7 @@ class TestAWSLambda:
                 client_id="111.111",
                 client_secret="xxx",
                 scopes=["chat:write", "commands"],
+                state_store=TestStateStore(),
             ),
         )
 
@@ -340,7 +346,7 @@ class TestAWSLambda:
             "isBase64Encoded": False,
         }
         response = SlackRequestHandler(app).handle(event, self.context)
-        assert response["statusCode"] == 401
+        assert response["statusCode"] == 200
         assert response["headers"]["content-type"] == "text/html; charset=utf-8"
         assert response.get("body") is not None
 
@@ -353,6 +359,6 @@ class TestAWSLambda:
             "isBase64Encoded": False,
         }
         response = SlackRequestHandler(app).handle(event, self.context)
-        assert response["statusCode"] == 401
+        assert response["statusCode"] == 200
         assert response["headers"]["content-type"] == "text/html; charset=utf-8"
         assert response.get("body") is not None

[tattee]

It's a nice idea to add TestStateStore for consume method.
I reflected your diff on test file.
(Sorry, I don't know how to execute pytest, and I did not confirm the test succeeds. By using unittest, I could only confirm passing consume method.)
If your diff is enough, I will submit new commit. If not, could you tell me how to use pytest?
Best regards,

[seratch]

@tattee Thanks for the comment! You can run pytest for this project this way. Also, there is a tiny script too.

[codecov]

Codecov Report

Merging #379 (c43cf70) into main (0927de4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #379      +/-   ##
==========================================
+ Coverage   91.55%   91.59%   +0.04%     
==========================================
  Files         167      167              
  Lines        5375     5378       +3     
==========================================
+ Hits         4921     4926       +5     
+ Misses        454      452       -2     

Impacted Files	Coverage Δ
slack_bolt/adapter/aws_lambda/handler.py	95.71% <100.00%> (+3.17%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0927de4...c43cf70. Read the comment docs.

[tattee]

Thank you for telling me how to execute test scripts.
I confirmed the test succeeded.
I pushed commit reflecting your test_oauth_redirect method.
Sorry for the late submission.

[seratch]

LGTM - thanks!

[weallwegot]

this is great- thank you! i realize now that this closes the loop on this exchange we had a while ago around v1 vs v2 support, glad to see this PR & release

[naveensan1]

@seratch I too deployed a Bolt-python app to AWS Lambda, integrated with REST (format v1.0) based API GW. In the process of the verification of oauth redirect, the app could not be installed properly. Taking a look at the CloudWatch logs I could see the multivalueheader in the request had the key "cookie" instead of "Cookie" as is expected by "to_bolt_request".

   if cookies is None or len(cookies) == 0:
        # In the case of format v1
        multiValueHeaders = event.get("multiValueHeaders", {})
        cookies = multiValueHeaders.get("Cookie", [])

https://github.com/slackapi/bolt-python/blob/main/slack_bolt/adapter/aws_lambda/handler.py#L91

After modifying the following line to account for lower case "cookie", everything worked fine:

     cookies = multiValueHeaders.get("cookie", [])

Recommend this "get" operation be modified to be case-insensitive as it is blocking oauth install at the moment.

[seratch]

@naveensan1 Thanks for sharing this! This makes sense 👍 If you have a chance, would you mind sending a pull request to fix this?