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

Fix for bug 264: API Gateway is bypassed when server return 302 #276

Merged
merged 17 commits into from May 20, 2019

Conversation

@CindyZX
Copy link

commented Mar 29, 2019

No description provided.

@CindyZX CindyZX requested review from plavjanik and JirkaAichler Mar 29, 2019

@ghost ghost assigned CindyZX Mar 29, 2019

@ghost ghost added the review label Mar 29, 2019

@JirkaAichler JirkaAichler requested a review from ilkinabdullayev Mar 29, 2019

@JirkaAichler

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Hello Cindy,
thank you for your contribution into APIML. We really appropriate that!

I will review it as soon as possible. Do you think you could meantime create also integration test for it?

@codecov

This comment has been minimized.

Copy link

commented Mar 29, 2019

Codecov Report

Merging #276 into master will decrease coverage by 0.78%.
The diff coverage is 84.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #276      +/-   ##
============================================
- Coverage      70.2%   69.42%   -0.79%     
  Complexity       12       12              
============================================
  Files           231      235       +4     
  Lines          4612     4500     -112     
  Branches        588      568      -20     
============================================
- Hits           3238     3124     -114     
  Misses         1223     1223              
- Partials        151      153       +2
Impacted Files Coverage Δ Complexity Δ
...m/ca/mfaas/gateway/routing/MfaasRoutingConfig.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ca/mfaas/client/api/PageRedirectionController.java 100% <100%> (ø) 0 <0> (?)
...a/com/ca/mfaas/product/routing/RoutedServices.java 96.87% <100%> (+0.1%) 0 <0> (ø) ⬇️
...ava/com/ca/mfaas/gateway/config/GatewayConfig.java 100% <100%> (ø) 0 <0> (?)
...va/com/ca/mfaas/client/model/RedirectLocation.java 100% <100%> (ø) 0 <0> (?)
...as/product/routing/transform/TransformService.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...main/java/com/ca/mfaas/product/utils/UrlUtils.java 76.92% <50%> (-11.97%) 0 <0> (ø)
...as/gateway/filters/post/PageRedirectionFilter.java 87.93% <87.93%> (ø) 0 <0> (?)
.../main/java/com/ca/mfaas/security/HttpsFactory.java 72.22% <0%> (-8.83%) 0% <0%> (ø)
...og/services/cached/CachedProductFamilyService.java 86.52% <0%> (-1.66%) 0% <0%> (ø)
... and 9 more

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 b4bf185...85c4f5f. Read the comment docs.

@CindyZX

This comment has been minimized.

Copy link
Author

commented Apr 2, 2019

Hi @JirkaAichler , sure, I will add some integration test

@CindyZX

This comment has been minimized.

Copy link
Author

commented Apr 3, 2019

I just modified the PR to add integration test. I added a new controller: PageRedirectionController to discoverable-client for the integration test. It accepts POST request, gets a url from the POST data, then set the url to response's Location header, and return status code 307

@ilkinabdullayev

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Hi Cindy,
Thanks for supporting to the api-layer!
We implemented new simple service(com.ca.mfaas.product.routing.transform.TransformService) which is transforming service urls into gateway urls. You will see this service(TransformService) once you pull the master branch.
I think you could use this service instead of your own code.

@taban03

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Hello @CindyZX , as @ilkinabdullayev said we have applied some changes which can be helpful for you and which is now in master. One of those changes is also involving the getBestMatchingServiceUrl that you're using in foundMatchedUrlInService, since we have improved it.

@CindyZX

This comment has been minimized.

Copy link
Author

commented May 15, 2019

Hi @arxioly ,
Thank you very much for the comments, I've modified all files according to your suggestions

@arxioly
Copy link
Collaborator

left a comment

Thank you, @CindyZX, for applying my suggestions. This is last one.

zhuxun
set maximum entries for cache
Signed-off-by: zhuxun <zhuxun@cn.ibm.com>
@JirkaAichler
Copy link
Collaborator

left a comment

Great work Cindy!

Just one last idea. Please put the max size constant into the original class (filter). ApimlConstants should be used for constants which are reusable by other classes.

I would also increase it to 1000. But honestly, I don't know what is the ideal size :-)

Thanks!

zhuxun
move max entry to filter class
Signed-off-by: zhuxun <zhuxun@cn.ibm.com>
@CindyZX

This comment has been minimized.

Copy link
Author

commented May 16, 2019

@JirkaAichler Thanks Jirka, I've put max entry constant back to the filter class and set it to 1000

JirkaAichler and others added some commits May 20, 2019

Fixed checkstyle
Fixed checkstyle
@JirkaAichler
Copy link
Collaborator

left a comment

Awesome work. Thank you Cindy!

@ilkinabdullayev
Copy link
Collaborator

left a comment

Thanks, Cindy!

@JirkaAichler JirkaAichler merged commit 0fb72e6 into zowe:master May 20, 2019

5 checks passed

DCO DCO
Details
WIP Ready for review
Details
codecov/patch 84.33% of diff hit (target 60%)
Details
codecov/project 69.42% (target 55%)
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@CindyZX CindyZX deleted the CindyZX:bug264 branch May 21, 2019

cZikos pushed a commit that referenced this pull request May 23, 2019

CindyZX Zikos, Christos
Fix for bug 264: API Gateway is bypassed when server return 302 (#276)
* Add support for page redirection to Gateway
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.