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

add documentation for remote_json authenticator #827

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

Conversation

vladr11
Copy link

@vladr11 vladr11 commented May 24, 2022

Adds documentation for the remote_json authenticator.

Related Issue or Design Document

ory/oathkeeper#841

Copy link
Collaborator

@piotrmsc piotrmsc left a comment

Choose a reason for hiding this comment

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

Thank you for providing the docs for changes you'd like to introduce in oathkeeper :) I went through your code PR and have two open questions :)

- `preserve_path` (boolean, optional - defaults to `false`) - If set to `true`, any path in `service_url` will be preserved instead of replacing the path with the path of the request being checked.
- `extra_from` (string, optional - defaults to `extra`) - A [GJSON Path](https://github.com/tidwall/gjson/blob/master/SYNTAX.md) pointing to the `extra` field. This defaults to `extra`, but it could also be `@this` (for the root element), `session.foo.bar` for `{ "subject": "...", "session": { "foo": {"bar": "whatever"} } }`, and so on.
- `subject_from` (string, optional - defaults to `subject`) - A [GJSON Path](https://github.com/tidwall/gjson/blob/master/SYNTAX.md) pointing to the `subject` field. This defaults to `subject`. Example: `identity.id` for `{ "identity": { "id": "1234" } }`.
- `method` (string, optional) - The method to pass to the authenticator service. If set, the method of the original request is overwritten with the specified method. If not set, the method of the original request is used.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Open question : does it make sense to use the original request method as a fallback instead of the default method set to POST? For example, let's say we do a DELETE request and have set this authentication in our rule but haven't overridden the config method propoperty.

Copy link
Author

@vladr11 vladr11 Jun 6, 2022

Choose a reason for hiding this comment

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

I think it makes sense because it would be the only way to pass the original method. Now it is true that usually an authentication server does not perform the authentication based on the request method, but I believe that it would be useful to cover such a case as well.

Another possibility would be to make the default behaviour to send a POST request to the authn server if no method is specified, and have another config flag that enables the users to simply forward the original method as well, which by default is disabled, so one needs to explicitly enable the current behaviour which preserves the method.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another possibility would be to make the default behaviour to send a POST request to the authn server if no method is specified, and have another config flag that enables the users to simply forward the original method as well, which by default is disabled, so one needs to explicitly enable the current behaviour which preserves the method.

Makes sense to me

Copy link
Author

Choose a reason for hiding this comment

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

Changed the behaviour of the authenticator and updated the docs


## `remote_json`

The `remote_json` authenticator will forward the HTTP request (method, path, headers and body) to an upstream service. If the service returns `200 OK` and body `{ "subject": "...", "extra": {} }`, then the authenticator will set the subject appropriately.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you extend a bit the description to point out that it aims to send both request body & headers? At least this is how I understood from the root issue description and would be nice to give here also a small note about the difference to bearer_token authenticator which you have pointed in the issue comment :)

Copy link
Author

Choose a reason for hiding this comment

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

Added a note to point out the difference between the remote_json & bearer_token authenticators

@vinckr vinckr added the upstream Issue is caused by an upstream dependency. label May 3, 2023
@vinckr
Copy link
Member

vinckr commented May 3, 2023

added the upstream label to make clear this is waiting for the PR in oathkeeper.

@aeneasr
Copy link
Member

aeneasr commented Feb 29, 2024

upstream is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream Issue is caused by an upstream dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants