Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RequestHook]: add methods for manipulation with response headers #1657

Closed
miherlosev opened this issue Jun 21, 2018 · 11 comments
Closed

[RequestHook]: add methods for manipulation with response headers #1657

miherlosev opened this issue Jun 21, 2018 · 11 comments
Assignees
Labels
STATE: Auto-locked Issues that were automatically locked by the Lock bot TYPE: enhancement

Comments

@miherlosev
Copy link
Contributor

miherlosev commented Jun 21, 2018

https://devexpress.github.io/testcafe/documentation/test-api/intercepting-http-requests/creating-a-custom-http-request-hook.html#the-onresponse-method

ResponseEvent.setHeader(name, value)
ResponseEvent.removeHeader(name)

@miherlosev miherlosev added this to the Sprint #13 milestone Jun 21, 2018
@LavrovArtem LavrovArtem modified the milestones: Sprint #13, Planned Jul 16, 2018
@NickCis
Copy link
Contributor

NickCis commented Oct 3, 2018

Hi!, I was trying to implement this feature, but, before making changes (and get them rejected) i though that it would be better to discuss the implementation approach.

First of all, what is the ideal interface?:
a. Copy the onRequest interface, and directly set / modify headers of an object: ie: responseEvent.response.headers['some-header'] = 'some-value'
b. Implement specific methods:

  • responseEvent.setHeader(name, value)
  • responseEvent.removeHeader(name)

Regarding the request pipeline, the headers (and all the response) are sent before the onResponse event is dispatched ([1], [2]). So, am i correct saying that this order should be inverted? (ie: first dispatch the onResponse, then send headers and payload).

In addition, i suppose that the onResponse handler should be run after the headerTransforms.forResponse is executed, Is that supposition ok?

[1] sendResponseHeaders, onResponse
[2] sendResponseHeader, onResponse

@miherlosev miherlosev changed the title [RequestMock]: add methods for manipulation with response headers [RequestHook]: add methods for manipulation with response headers Oct 4, 2018
@miherlosev
Copy link
Contributor Author

miherlosev commented Oct 4, 2018

Hi @NickCis

We planned this feature for internal purposes.
Before discussing the API interfaces, could you please tell us about you use case?

@NickCis
Copy link
Contributor

NickCis commented Oct 4, 2018

Hi @miherlosev , thank you for the quick response.

The use case is interacting with an external payment api that is used by adding an iframe to the web. The idea is to set headers in order to allow interaction with the iframe.

@miherlosev
Copy link
Contributor Author

Would you please clarify which headers you want to modify to allow interaction with the iframe?

@NickCis
Copy link
Contributor

NickCis commented Oct 4, 2018

Yes, sorry for being too shynthetic.

This 3rd party API sets the X-Frame-Options header to ALLOW-FROM <our domain>. CI runs tests against a localhost domain, so the iframe cannot be used. The idea is to test a particular branch (devs usually use a browser plugin in order to modify headers).

Another use case, that addresses this feature, is implementing a workaround for Facebook login.

@miherlosev
Copy link
Contributor Author

Thank you for the detailed explanation of your scenario. Before discussing a public API we need to perform an additional investigation.

@miherlosev
Copy link
Contributor Author

Having discussed this with the team, we can suggest the following API:

  • removeHeader(name)
  • setHeader(name, value)

These methods will be defined as methods of the ConfigureResponseEvent class.

The feature implementation can be divided into 2 parts:

  • split the header-transforms.js
    file into two parts:

    1. header-transforms.js - contains only transformations for request and response headers
    2. header-transform-api.js - contains the forRequest, forResponse and transformHeadersCaseToRaw methods
  • change response headers transformation order.

    1. apply all header changes from request hooks
    2. apply header transformation
    3. write updated headers to the response.

@NickCis
Copy link
Contributor

NickCis commented Oct 12, 2018

@miherlosev I'll be working on that on PR #1790

@NickCis
Copy link
Contributor

NickCis commented Oct 12, 2018

The proposed implementation will allow to modify the headers in the onConfigureResponse event.

The problem with this event is that, in testcafe, is private. The onConfigureResponse event calls the _onConfigureResponse method of the RequestHook which has a default implementation and is somehow private.

In addition of this PR, won't testcafe need one to make this api public?, ei: add a onConfigureResponse method which has to be optionally implemented on request hooks (if we force users to implement this method, testcafe update will break people's code when they update) which will be called by the _onConfigureResponse method.

@miherlosev
Copy link
Contributor Author

miherlosev commented Oct 12, 2018

Yes, it's more appropriate to mark the onConfigureResponse event as public and remove the default implementation from the RequestHook base class.
We will do it only before the [email protected] was released, because it's a breaking change.
Now, let's keep this method as is.
So, the use of the setHeader and removeHeader methods will look like this:

class CustomRequestHook extends RequestHook {
    _onConfigureResponse (event) {
      super._onConfigureResponse(event);
 
     event.setHeader('x-frame-options', 'allow-from https://example.com/');
     event.removeHeader('x-header-name');
    }
}

AndreyBelym pushed a commit to AndreyBelym/testcafe-hammerhead that referenced this issue Feb 28, 2019
…oses DevExpress#1657) (DevExpress#1790)

* Splited header-transforms

* ConfigureResponseEvent: `setHeader` & `removeHeader`

* test/server/request-hook-test: Added ConfigureResponseEvent `removeHeader` & `setHeader` test

* test/server/proxy-test: Added Response header modification test cafe

* Renamed `header-transforms` files and exports

* fixed test/server/proxy-test typo

* request-pipeline: splitted `callResponseEventCallbackForProcessedRequest` in order to send `onResponse` on `ctx.res.end()` callback
@lock
Copy link

lock bot commented Mar 28, 2019

This thread has been automatically locked since it is closed and there has not been any recent activity. Please open a new issue for related bugs or feature requests. We recommend you ask TestCafe API, usage and configuration inquiries on StackOverflow.

@lock lock bot added the STATE: Auto-locked Issues that were automatically locked by the Lock bot label Mar 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 28, 2019
@LavrovArtem LavrovArtem removed this from the Planned milestone Feb 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
STATE: Auto-locked Issues that were automatically locked by the Lock bot TYPE: enhancement
Projects
None yet
Development

No branches or pull requests

4 participants