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

[policy] [api-key] Policy failures always produce json content #1719

Closed
k-oliver opened this issue Dec 5, 2018 · 12 comments

Comments

@k-oliver
Copy link

commented Dec 5, 2018

if a policy, for example the api-key policy produces a failure and would set a content type for the PolicyResult the content type is ignored and always overwritten by content type json.

Expected Behavior

i expected the content type to be recognized. imagine a api key policy that produces xml content in case of a api key failure.

Current Behavior

the content type is overwritten here: https://github.com/gravitee-io/gravitee-gateway/blob/76bee704a624e2900e3b61c7a7b38f280e3f5332/gravitee-gateway-handlers/gravitee-gateway-handlers-api/src/main/java/io/gravitee/gateway/handlers/api/ApiReactorHandler.java#L235

Possible Solution

respect the given content type

Steps to Reproduce (for bugs)

Context

my general goal was to change the content of in invalid api key response which lead me to the question of what happens in case of api key checks in non json endpoints. really great would be a configurable response for the api key policy plugin.

Your Environment

duplicates #1473

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

Hi @k-oliver

There are two things here:

  1. Errors must be return following the Accept header send by the client.
  2. You want to change the error message for the API-key policy (but you may want the same for some of our other policies).

Am I right ?

@k-oliver

This comment has been minimized.

Copy link
Author

commented Dec 6, 2018

Hi @brasseld ,

  1. You are right. in case of the API-key policy the Accept header was my starting point. i send the Accept header application/xml to some endpoint with API-key policy, the API-Key check fails and json content is returned. so the Accept header is not resprected by the API-key policy. this should by changed i guess.

  2. Then I looked into the API-key policy to find out what went wrong and got the idea of writing an own policy where i can define the response content matching the current API. so in generell it would be great to have the possibility to define the response payload for all policies that are able to return complete responses (e.g. are calling PolicyChain.failWith(...)). in case of the API-key policy that may be something configurable like

if (errorCaseNoAPIkeygiven) {
  if (accept.equals("application/json") {
    return "{\"some\":"configured payload"}";
  }
  if (accept.equals("application/xml")) {
    return "<some>configured payload</some>"
  }
}
if (errorCaseWrongAPIKey) {
  ....
}

so the configurable part may be the different error cases combined with the Accept content type.

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Hi

  1. Duplicate of #1473

  2. Duplicate of #972

I think the first point can be resolved pretty easily.
The second one is not so easy because I'm sure that response may be customized for each API and not globally to the platform.

What do you think?

@k-oliver

This comment has been minimized.

Copy link
Author

commented Dec 6, 2018

Hi @brasseld ,

  1. great!
  2. sure. the response should be costomized for each API. so it has to be a configuration for the API-Key policy. for now i will try to write a costom api key policy with configurable payload (only json in this case) and let you know about the result?
@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

  1. With pleasure !
@k-oliver

This comment has been minimized.

Copy link
Author

commented Dec 7, 2018

Hi @brasseld ,
I tried to add a new costom policy for the API-Key handling and failed at some points. So I switched back to the original API-Key policy and made some changes that can be seen here:
k-oliver/gravitee-policy-apikey@4700cf3

As you can see I added some configuration parts for the API-Key policy where you can add costom responses by content type and error case. Error cases are MISSING_API_KEY and WRONG_API_KEY. To reach a completly costomized response i also needed to add two plans for the API: a free plan and an API-Key plan plus an API-Key definition at design level:
image
image
image
I needed to do this do this double configuration to also match the MISSING_API_KEY case that is caught by the API-Key policy at design level.

This configuration works for API calls with header Accept application/json. To be able to match application/xml the overwritten content type in the ApiReactorHandler class needs to be fixed. I guess this would result in an API break depending on the callers of PolicyResult.failure(..).

As a result i would say it is possible to costomize responses but it need to be done in any result writing policy (as you mentioned).

What do you think?

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

Hello @k-oliver

Really nice stuff, congrats!

My only concern is about the Content-Type property. I don't understand its purpose since we have to rely only on the 'Accept' header to choose for the correct response Content-type.

The other point is about the way to handle multiple error types.
I think that the best is to keep the api-key policy only at the plan level (and not the API as you've done).

Why not providing an all the valid response type within the API-key policy configuration ?

Last point:

  • type of error should not support EL (doesn't make sense)
  • idem for response code
@k-oliver

This comment has been minimized.

Copy link
Author

commented Dec 9, 2018

Hello @brasseld ,
i changed the last point with k-oliver/gravitee-policy-apikey@27f07ed.

The purpose of the Content-Type property is to configure different payloads for different Accept headers. For example if you have an API that can answer JSON and XML depending on the Accept header you may want to configure a JSON response content when JSON is requested and XML content if XML is requested. The property Content-Type is matched against the Accept header to choose the correct response from the configuration.

The multiple error handling was a work around. I first tried to do all stuff at plan level. BUT: The API-Key policy do not come to execution if no API-Key header is part of the request. See here: https://github.com/gravitee-io/gravitee-gateway/blob/f09b9d526b2c3f65852e6573c0cba7037783001f/gravitee-gateway-security/gravitee-gateway-security-core/src/main/java/io/gravitee/gateway/security/core/SecurityPolicyChainResolver.java#L56
So if no AuthenticationHandler can handle a request the default response is used. I also forked the relevant code and made a change to choose one of the configured AuthentificationHandlers in this case:
k-oliver/gravitee-gateway@2fe85c1
This enables a full plan level configuration.

do you agree?

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Hello @k-oliver

Sorry for the delay.

I understand the purpose of the content-type, but I would prefer to manage the content-type according to the Accept header (from the request). And, in that case, it will not be specific to the api-key policy only but to all the platform.

Ok, I got your 2).

If we fix the issue about content-type I think it will be easier for you to manage api-key policy's configuration and you will have to take care only about error code from api-key.

WDYT ?

@brasseld brasseld changed the title policy failures always produce json content [policy] [api-key] Policy failures always produce json content Dec 10, 2018

@k-oliver

This comment has been minimized.

Copy link
Author

commented Dec 10, 2018

Hey @brasseld ,
I'm not shure if I understood you correct: the content-type is actually managed according to the Accept header from the request. see here: https://github.com/k-oliver/gravitee-policy-apikey/blob/27f07ed700a75cf4470a89b231ebc027f93f8b8f/src/main/java/io/gravitee/policy/apikey/ApiKeyPolicy.java#L146
a current configuration only at plan level for the API-Key policy looks like this (JSON and XML responses depending on the Accept header):
image
image

It would be ok for me to remove the content-type property, if you prefer it. Should I?
Then there would be no need to check for the Accept header any more because you can only configure one response for each error type.

A generell configuration outside the API-Key policy would be ok too, but it has to be at plan or API level to be part of the API specification, right?

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Ok, I didn't understand why you do such thing but now I think I got it.
You have defined a specific response for each content-type, I think that was the missing point (from my side)...

@brasseld

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Will be done thanks to #972

@brasseld brasseld closed this Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.