fix(api): change request interceptors applying logic

[5d]

Issue #

#3184

Description

The existing implementation of the URLRequest interceptor proceeds as follows:

1. Applying customized headers.
2. Applying library predefined interceptors.
3. Applying user-defined interceptors.

However, this approach has certain issues. For instance, one of the Amplify predefined interceptors requires the calculation of the Sigv4 authentication header for the request. This calculation involves certain headers. Making it impossible for customers to modify these header fields within their own custom interceptors.

After some discussions, we have divided the library predefined interceptors into prelude and postlude interceptors, and the interceptor logic should be as follows:

1. Applying Amplify prelude interceptors.
2. Applying customized headers.
3. Applying customer-defined interceptors.
4. Applying Amplify postlude interceptors.

This updated logic ensures that customer-defined interceptors will be applied with the expected customized headers and the postlude interceptors will operate on the correct request after customer-defined interceptors have been applied.

General Checklist

• Added new tests to cover change, if needed
• Build succeeds with all target using Swift Package Manager
• All unit tests pass
• All integration tests pass
• Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)
• Documentation update for the change if required
• PR title conforms to conventional commit style
• New or updated tests include Given When Then inline code documentation and are named accordingly testThing_condition_expectation()
• If breaking change, documentation/changelog update with migration instructions

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[lawmicha]

in this flow (construct -> intercept -> apply customized header), do we still need to apply the request.headers here?

[5d]

As this method signature implicates, I think we still need to apply the headers.

static func constructURLRequest(with url: URL,
                                    operationType: RESTOperationType,
                                    headers: [String: String]?,
                                    requestPayload: Data?) -> URLRequest

[atierian]

The customized request headers provided during the creation of the RESTRequest should take precedence over interceptors.

This is already happening here before the change.

var requestHeaders = ["content-type": "application/json"]
if let headers = headers { 
    for (key, value) in headers { // if there's a "content-type" key here
        requestHeaders[key] = value  // it will overwrite the default "application/json" value here
    }
}

My understanding is that we need to change...

• IAMURLRequestInterceptor
• AuthTokenURLRequestInterceptor

... to give precedence to callsite supplied headers.

Am I missing something?

[5d]

The customized request headers provided during the creation of the RESTRequest should take precedence over interceptors.

This is already happening here before the change.

var requestHeaders = ["content-type": "application/json"]
if let headers = headers { 
    for (key, value) in headers { // if there's a "content-type" key here
        requestHeaders[key] = value  // it will overwrite the default "application/json" value here
    }
}

My understanding is that we need to change...

• IAMURLRequestInterceptor
• AuthTokenURLRequestInterceptor

... to give precedence to callsite supplied headers.

Am I missing something?

The logic to build the URLRequest for RESTRequest happends here. In my opinion, as the interceptors are simply adjust the request to align with their specific requirements, we just need to ensure the customize headers are applied after applying all the interceptors.

[sandeepklr]

In my opinion, as the interceptors are simply adjust the request to align with their specific requirements, we just need to ensure the customize headers are applied after applying all the interceptors.

For my edification, can you point to some external documentation that says you need to set a content-type: application/json header to use IAM auth or Auth token?

There is probably a requirement to set A content type [citation needed], at which point would it be cleaner for the Interceptors to check if one has not already been set and then set it based on their individual requirements? Interceptors are passed in a URLRequest object, so should be easy to do. Concretely, something like this:

if  mutableRequest.value(forHTTPHeaderField: "content-type") == nil {
        mutableRequest.setValue(URLRequestConstants.ContentType.applicationJson,
                                forHTTPHeaderField: URLRequestConstants.Header.contentType)
}

By introducing another method to set custom headers fields like this PR does, there's multiple places in this code where header fields could be set or overwritten, which makes it hard to reason or read.

Thoughts?

[5d]

In my opinion, as the interceptors are simply adjust the request to align with their specific requirements, we just need to ensure the customize headers are applied after applying all the interceptors.

For my edification, can you point to some external documentation that says you need to set a content-type: application/json header to use IAM auth or Auth token?

I don't think IAM or auth token need to be in json format. I beleive the Content-Type header was added as default just because all Amplify generated APIs are using json.

There is probably a requirement to set A content type [citation needed], at which point would it be cleaner for the Interceptors to check if one has not already been set and then set it based on their individual requirements? Interceptors are passed in a URLRequest object, so should be easy to do. Concretely, something like this:

if  mutableRequest.value(forHTTPHeaderField: "content-type") == nil {
        mutableRequest.setValue(URLRequestConstants.ContentType.applicationJson,
                                forHTTPHeaderField: URLRequestConstants.Header.contentType)
}

By introducing another method to set custom headers fields like this PR does, there's multiple places in this code where header fields could be set or overwritten, which makes it hard to reason or read.

Thoughts?

The new method is internal method. The key point of the PR is we respect the headers created with RESTRequest a higher priority than interceptors.

[sandeepklr]

The new method is internal method. The key point of the PR is we respect the headers created with RESTRequest a higher priority than interceptors.

While this might be an internal method, correct me if I'm wrong- doesn't this have ramifications of the behavior of the library?

Previously you could set a "default" value for a header while creating the RESTRequest object, while letting a later user-defined interceptor to dynamically change the value of the header. So something like this:

let request = RESTRequest(path: "/path", ["
       "customHeader": "default-value"
"] )

....

struct CustomInterceptor: URLRequestInterceptor {
    func intercept(_ request: URLRequest) throws -> URLRequest {
           ....
           if (conditionIsTrue) {
                request.setValue("customHeader", "new-value")
           }
    }

Now the behavior of the library is changing so that it the value of "customHeader" is going to be set to "default-value", as opposed to "new-value" previously.

At the very least I would think this needs a documentation change to call out this new behavior, and perhaps even a MV bump (I'm new to this library so I'm not sure what constitutes a MV bump).

Just my 2c. Thanks for looking into this.

[5d]

Thanks for your example @sandeepklr. I got your point. You are right. The current change could impact current users who rely on custom header fields to inform dynamic decisions within their user interceptors.

The desired request decoration process should follow this sequence:

1. Build the URLRequest
2. Apply interceptors provided by library
3. Apply user-defined interceptors

When we reach step 3, the custom headers should already be integrated into the URLRequest.
One potential solution I'm thinking is creating an additional library interceptor ApplyCustomizeHeadersInterceptor. This new interceptor would handle the application of custom headers and execute as the final step within the step 2. Therefore, the user-defined interceptors will remain unaffected, and library interceptors won't override the customized headers.

[sandeepklr]

Apologies in advance if I'm misunderstanding this....

While that may work for AuthTokenURLInterceptor, one thing to consider is if that breaks IAMURLRequestInterceptor?

According to SigV4 specification[1], Content-Type header must included while computing signature, if present. With your approach if someone did the following while using IAM Auth (which is actually what I'm doing in my original bug report sans IAM Auth):

let request = RESTRequest(path: "/path", ["
       "Content-Type": "multipart/form-data; boundary=......"
"] )

Assuming the library signs Content-Type header, IAMURLRequestInterceptor would sign with Content-Type: application/json, while the ApplyCustomizeHeadersInterceptor would change the request to Content-Type": "multipart/form-data; boundary=...... causing it to fail?

I haven't peeled enough layers of the library back to confirm the following:

1. Does the library include Content-Type while SigV4 signing.
2. Is there an integ test to repro changing Content-Type header after Sigv4 signing, and does the request fail.

---

[1] https://docs.aws.amazon.com/AmazonS3/latest/API/sig-v4-header-based-auth.html#:~:text=If%20the%20Content%2DType%20header%20is%20present%20in%20the%20request%2C%20you%20must%20add%20it%20to%20the%20CanonicalHeaders%20list.

[sandeepklr]

More and more, I think the fact that the IAMURLRequestInterceptor and AuthTokenURLRequestInterceptor setting Content-Type is more headache than it is worth and not intuitive. They shouldn't need to care about the Content-Type, they should only care about setting the headers that they "own" (Authorization, x-Amz-Date, are examples of headers they could own). Content-Type is not a header they should own and blindly set.

If the library cares about setting a default Content-Type, it should set it before the interceptors run, which it already is based on an earlier comment[1].

If the user chooses to change a signed Header value in their custom interceptor later, that is on the user and it's at least not the library doing something wrong. Ideally there'd be a way to detect that and emit a warning log to the user, but whatever.

[1] #3190 (review)

[5d]

More and more, I think the fact that the IAMURLRequestInterceptor and AuthTokenURLRequestInterceptor setting Content-Type is more headache than it is worth and not intuitive. They shouldn't need to care about the Content-Type, they should only care about setting the headers that they "own" (Authorization, x-Amz-Date, are examples of headers they could own). Content-Type is not a header they should own and blindly set.

If the library cares about setting a default Content-Type, it should set it before the interceptors run, which it already is based on an earlier comment[1].

If the user chooses to change a signed Header value in their custom interceptor later, that is on the user and it's at least not the library doing something wrong. Ideally there'd be a way to detect that and emit a warning log to the user, but whatever.

[1] #3190 (review)

Yep, you and @atierian are correct. I didn't know the sigv4 calculate the signature including the Content-Type header. We should have customized header override before calculating the sigv4. I will revert the changes and just remove the logic of setting the Content-Type in library provided interceptors.

[codecov-commenter]

Codecov Report

Merging #3190 (57da7c8) into main (73db7ad) will increase coverage by 0.21%.
Report is 6 commits behind head on main.
The diff coverage is 72.77%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             main    #3190      +/-   ##
==========================================
+ Coverage   64.56%   64.77%   +0.21%     
==========================================
  Files        1090     1091       +1     
  Lines       37005    37051      +46     
==========================================
+ Hits        23891    24000     +109     
+ Misses      13114    13051      -63     

Flag	Coverage Δ
API_plugin_unit_test	67.10% <72.77%> (+0.57%)	⬆️
AWSPluginsCore	69.55% <ø> (ø)
Amplify	47.86% <ø> (+0.01%)	⬆️
Analytics_plugin_unit_test	86.39% <ø> (ø)
Auth_plugin_unit_test	75.11% <ø> (-0.04%)	⬇️
CoreMLPredictions_plugin_unit_test	59.44% <ø> (ø)
DataStore_plugin_unit_test	81.11% <ø> (+1.01%)	⬆️
Geo_plugin_unit_test	54.06% <ø> (ø)
Logging_plugin_unit_test	62.77% <ø> (ø)
Predictions_plugin_unit_test	35.17% <ø> (ø)
PushNotifications_plugin_unit_test	69.69% <ø> (ø)
Storage_plugin_unit_test	54.10% <ø> (ø)
unit_tests	64.77% <72.77%> (+0.21%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
.../RequestInterceptor/IAMURLRequestInterceptor.swift	11.90% <ø> (+0.27%)	⬆️
...s/AWSAPIPlugin/Operation/AWSGraphQLOperation.swift	67.24% <63.21%> (+5.61%)	⬆️
...gin/Configuration/AWSAPIEndpointInterceptors.swift	62.50% <71.42%> (+1.63%)	⬆️
...rces/AWSAPIPlugin/Operation/AWSRESTOperation.swift	78.00% <77.21%> (+13.36%)	⬆️
...figuration/AWSAPICategoryPluginConfiguration.swift	75.30% <100.00%> (-4.46%)	⬇️
...stInterceptor/AuthTokenURLRequestInterceptor.swift	90.47% <100.00%> (-0.83%)	⬇️
...n/Support/Utils/GraphQLOperationRequestUtils.swift	91.66% <100.00%> (ø)
...ugin/Support/Utils/RESTOperationRequestUtils.swift	71.18% <100.00%> (-1.83%)	⬇️
...rces/AWSAPIPlugin/Support/Utils/Result+Async.swift	100.00% <100.00%> (ø)

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more