Skip to content

oauth2: Add default expiry for RFC compliance#31499

Merged
phlax merged 4 commits intoenvoyproxy:mainfrom
phlax:oauth2-expiry
Jan 3, 2024
Merged

oauth2: Add default expiry for RFC compliance#31499
phlax merged 4 commits intoenvoyproxy:mainfrom
phlax:oauth2-expiry

Conversation

@phlax
Copy link
Copy Markdown
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:]

@repokitteh-read-only
Copy link
Copy Markdown

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
Copy Markdown
Member Author

phlax commented Dec 22, 2023

cc @terorie @williamsfu99

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 22, 2023

/docs

@repokitteh-read-only
Copy link
Copy Markdown

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
Copy Markdown
Member Author

phlax commented Dec 22, 2023

/retest

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 23, 2023

this appears to work as expected in local testing

Copy link
Copy Markdown
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.

Comment thread api/envoy/extensions/filters/http/oauth2/v3/oauth.proto Outdated
Copy link
Copy Markdown
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

Comment thread source/extensions/filters/http/oauth2/filter.h Outdated
adisuissa
adisuissa previously approved these changes Jan 3, 2024
Copy link
Copy Markdown
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
Copy Markdown
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

phlax added 4 commits January 3, 2024 17:35
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
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