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

Authenticator bearer_token token_from query_parameter does not forward token to check_session_url if preserve_path: true #786

Closed
hunzikerj opened this issue Jul 9, 2021 · 3 comments
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.

Comments

@hunzikerj
Copy link

Describe the bug

The bearer_token authenticator does only forward request method/path/headers. If preserve_path is set to true then the query_parameter will not get forwarded as the url is replaced by the check_session_url.

Reproducing the bug

Steps to reproduce the behavior:

Using Oathkeeper according to the example in the documentation.

# Some Access Rule Preserving Path: access-rule-2.yaml
id: access-rule-2
# match: ...
# upstream: ...
authenticators:
  - handler: bearer_token
    config:
      check_session_url: https://session-store-host/check-session
      token_from:
        query_parameter: auth-token
      preserve_path: true

Expected behavior

If preserve_path is set to true then the query_parameter should be appended to the check_session_url in the request.

Example:

https://session-store-host/check-session?auth-token=xxx

Environment

  • Version: v0.38.12-beta.1
@aeneasr
Copy link
Member

aeneasr commented Jul 14, 2021

Yup, that makes sense - tracking as a feat request :)

@aeneasr aeneasr added feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one. labels Jul 14, 2021
@grantzvolsky
Copy link
Contributor

The preserve_path flag preserves the path in check_session_url because by default it is the path in the request that is forwarded. The counterintuitive property/bug is that while the request path is forwarded by default, the request query is ignored by default, without any option to forward it.

I will submit a PR that forwards the request query by default, which is consistent with forwarding the path by default. This is a breaking change because someone may depend on the query in check_session_url being preserved as it currently is. Should I introduce a new flag, preserve_query, and document it as a breaking change?

@grantzvolsky
Copy link
Contributor

We could also make the preserve_query flag true by default, which would achieve the same effect without any breaking changes.

grantzvolsky added a commit to grantzvolsky/oathkeeper that referenced this issue Sep 6, 2021
This commit adds a new flag, preserve_query, in order to resolve issue ory#786. It
would be ideal for this flag to be false by default for consistency with
preserve_path, but it is true by default in order to preserve backwards
compatibility.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
Development

No branches or pull requests

3 participants