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: Add default expiry for RFC compliance #31499

Merged
merged 4 commits into from
Jan 3, 2024

Conversation

phlax
Copy link
Member

@phlax phlax commented Dec 22, 2023

Currently this filter does not work with Github - which would seem like one of the most likely use cases

This is a revival of #14625

Should fix #14542

Unblocks #31534

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

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 @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #31499 was opened by phlax.

see: more, trace.

@phlax
Copy link
Member Author

phlax commented Dec 22, 2023

cc @terorie @williamsfu99

@phlax
Copy link
Member Author

phlax commented Dec 22, 2023

/docs

Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/31499/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #31499 (comment) was created by @phlax.

see: more, trace.

@phlax
Copy link
Member Author

phlax commented Dec 22, 2023

/retest

@phlax
Copy link
Member Author

phlax commented Dec 23, 2023

this appears to work as expected in local testing

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Left a small API comment.
cc @derekargueta @mattklein123 oauth2 code-owners.

Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks!
/lgtm api

Assigning Matt as codeowner.
/assign @mattklein123

source/extensions/filters/http/oauth2/filter.h Outdated Show resolved Hide resolved
adisuissa
adisuissa previously approved these changes Jan 3, 2024
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Oh, I think it needs a release note as well.

@phlax
Copy link
Member Author

phlax commented Jan 3, 2024

apologies for fp - im rebasing on this pr so it was to avoid the merge commit, pr commits were not changed

Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax merged commit 3d67a3f into envoyproxy:main Jan 3, 2024
54 checks passed
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] Incompatibility with access_token responses without "expire_in"
3 participants