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: response handling for session store (bearer + cookie) #916

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

till
Copy link

@till till commented Feb 9, 2022

  • Fix: response lost
  • Chore: include tag/commit (instead of master)
  • Fix: gzip handling

@till till requested a review from aeneasr as a code owner February 9, 2022 11:45
@CLAassistant
Copy link

CLAassistant commented Feb 9, 2022

CLA assistant check
All committers have signed the CLA.

Makefile Outdated Show resolved Hide resolved
pipeline/authn/authenticator_bearer_token.go Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #916 (9ea1270) into master (0b5f6e6) will decrease coverage by 12.22%.
The diff coverage is 25.00%.

❗ Current head 9ea1270 differs from pull request most recent head 4a4fd23. Consider uploading reports for the commit 4a4fd23 to get more accurate results

@@             Coverage Diff             @@
##           master     #916       +/-   ##
===========================================
- Coverage   77.87%   65.66%   -12.22%     
===========================================
  Files          80      104       +24     
  Lines        3924     4636      +712     
===========================================
- Hits         3056     3044       -12     
- Misses        589     1316      +727     
+ Partials      279      276        -3     
Impacted Files Coverage Δ
pipeline/authn/authenticator_bearer_token.go 62.50% <0.00%> (-11.75%) ⬇️
pipeline/authn/authenticator_cookie_session.go 78.31% <33.33%> (-1.87%) ⬇️
rule/rule.go 74.13% <0.00%> (-6.10%) ⬇️
api/decision.go 91.11% <0.00%> (-4.45%) ⬇️
rule/rule_migrator.go 71.18% <0.00%> (-3.39%) ⬇️
pipeline/mutate/mutator_hydrator.go 65.09% <0.00%> (-2.74%) ⬇️
rule/fetcher_default.go 76.65% <0.00%> (-2.52%) ⬇️
metrics/prometheus.go 93.33% <0.00%> (-2.42%) ⬇️
credentials/fetcher_default.go 64.89% <0.00%> (-0.89%) ⬇️
... and 102 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@till till changed the title debug build fix: response handling for session store (bearer + cookie) Feb 9, 2022
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

Makefile Outdated Show resolved Hide resolved
pipeline/authn/authenticator_bearer_token.go Show resolved Hide resolved
pipeline/authn/authenticator_bearer_token.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
@till
Copy link
Author

till commented Feb 10, 2022

@aeneasr tried to address everything, let me know what you think.

@till
Copy link
Author

till commented Apr 10, 2022

@aeneasr we are relying on this to work, is it possible to get this integrated soon? As we are now having issues with websockets (see ory/network#76), it's getting messy to maintain this downstream. We'd also like to update of course.

@aeneasr
Copy link
Member

aeneasr commented Apr 17, 2022

Sorry for dropping the ball on this - I'm currently on my holidays but will try to find some time to review it :)

pipeline/authn/authenticator_bearer_token.go Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
till added 9 commits June 23, 2022 15:08
I am not sure where to start, we run against ory platform and noticed
that the response "from" oathkeeper did not contain a subject when it
got to the backend service.

I managed to find out that the "body" in `forwardRequestToSessionStore()`
was nil/empty in some cases and the gjson calls failed silently.

I started log.Printf() debugging in these two service files and simplified
the code a bit to make it more readable. And that seemed to have fixed it.
@till
Copy link
Author

till commented Jun 23, 2022

@aeneasr everything but lint works, when I run make format it changes all the markdown files. I hope you can fix that in another PR? I know you probably have tons on your plate, but I would really like to get this into the main branch so we can stop maintaining this? Any thoughts on how soon this is possible?

@aeneasr
Copy link
Member

aeneasr commented Sep 10, 2022

Hi @till - sorry that I missed your comment. If you resolve the conflicts with master everything should pass and we can take this for another review spin :)

@till
Copy link
Author

till commented Feb 19, 2023

@aeneasr seems like you implemented most of what we had (around preserving headers, settings, etc.) so this PR just adds test coverage for the original issue and fixes a bug where subject is not returned (where it is expected).

aeneasr
aeneasr previously approved these changes Feb 21, 2023
- refactors test helpers into middleware_test.go
- provides a "complete" response from the test server
- complete response is necessary in order to provide subject (to allow the request)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants