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

oauth2: support disabling redirects for AJAX requests #32655

Merged
merged 6 commits into from
Mar 7, 2024

Conversation

samo1
Copy link
Contributor

@samo1 samo1 commented Mar 1, 2024

Commit Message: oauth2: support disabling redirects for AJAX requests

Additional Description: Added new parameter ajax_request_matcher to optionally deny OAuth2 authorization redirect when all tokens are expired. It usually redirects the user to a login page (in authorization code flow) and this behavior is not desired in Ajax requests.

Risk Level: Low
Testing: Unit tests
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features:
Fixes #30102

Added new parameter `ajax_request_matcher` to optionally not allow OAuth2 authorization redirect when all tokens are expired. Such redirect usually redirects the user to a login page (in authorization code flow) and this behavior is not desired in Ajax requests.
Fixes envoyproxy#30102

Signed-off-by: Samuel Valis <[email protected]>
Copy link

Hi @samo1, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #32655 was opened by samo1.

see: more, trace.

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #32655 was opened by samo1.

see: more, trace.

Signed-off-by: Samuel Valis <[email protected]>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

One question, thanks.

/wait-any

Comment on lines 141 to 144
// Requests that match any of the provided matchers will immediately return unauthorized response
// when tokens are not valid (instead of redirecting to authorization server as usual).
// This behavior is useful for AJAX and asset requests to avoid the OAuth flood.
repeated config.route.v3.HeaderMatcher ajax_request_matcher = 14;
Copy link
Member

Choose a reason for hiding this comment

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

I don't know much about this stuff but are there other cases where it would be desirable to have this behavior? I.e., is this AJAX specific? Would it be better to just call this immediate_unauthorized_response_matcher or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. You are right that there might be other cases where this would be desirable. I have changed the name and description to describe the functionality better, I hope.

Signed-off-by: Samuel Valis <[email protected]>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM with small comment nit. Can you merge main?

/wait


// Any request that matches any of the provided matchers won't be redirected to OAuth server when tokens are not valid.
// Automatic access token refresh will be performed for these requests, if enabled.
// This behavior cam be useful for AJAX requests.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// This behavior cam be useful for AJAX requests.
// This behavior can be useful for AJAX requests.

Signed-off-by: Samuel Valis <[email protected]>
# Conflicts:
#	changelogs/current.yaml

Signed-off-by: Samuel Valis <[email protected]>
# Conflicts:
#	changelogs/current.yaml

Signed-off-by: Samuel Valis <[email protected]>
@repokitteh-read-only repokitteh-read-only bot removed the api label Mar 7, 2024
@mattklein123 mattklein123 merged commit 8318716 into envoyproxy:main Mar 7, 2024
50 of 52 checks passed
mattjo added a commit to mattjo/envoy that referenced this pull request Mar 7, 2024
* origin:
  mobile: Clean up Java and Kotlin code (envoyproxy#32773)
  [mobile] Increase stream response buffer watermark size to 2MB (envoyproxy#32754)
  oauth2: support disabling redirects for AJAX requests (envoyproxy#32655)
  http2: Switch the value of envoy.reloadable_features.http2_use_oghttp2 to false (envoyproxy#32751)
  envoy: reverting unnecessary exception E-M macros (envoyproxy#32745)
  dfp: reverting 31433 (envoyproxy#32743)
  opentelemetrytracer: Allow sampler to set variant type span attribute… (envoyproxy#32681)
  mobile: Java code cleanup (envoyproxy#32747)
  mobile: fix quic_test_server_test (envoyproxy#32755)
  Common TunnelResponseHeadersOrTrailers for UDP & TCP tunneling (envoyproxy#32619)
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.

[OAuth2] return 401 instead of 302 on xhr requests
2 participants