oauth2: add "default_expires_in" config option by riptl · Pull Request #14625 · envoyproxy/envoy

riptl · 2021-01-11T01:30:52Z

Signed-off-by: Richard Patel me@terorie.dev

Commit Message: oauth2: add "default_expires_in" config option
Additional Description: Fixes compatibility with OAuth 2.0 authorization servers that don't provide the "expires_in" property in the token response
Risk Level: Low
Testing: Added OAuth client tests for two new branches (default expires_in, missing default or explicit expires_in)
Docs Changes: Inline API proto
Release Notes: oauth2 filter: added "default_expires_in" property for authorization servers that don't provide the "expires_in" token response property
Fixes #14254

repokitteh-read-only · 2021-01-11T01:30:56Z

Hi @terorie, 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: #14625 was opened by terorie.

see: more, trace.

repokitteh-read-only · 2021-01-11T01:31:00Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14625 was opened by terorie.

see: more, trace.

Signed-off-by: Richard Patel <me@terorie.dev>

markdroth · 2021-01-11T16:47:08Z

/lgtm api

rgs1 · 2021-02-15T14:37:55Z

cc: @williamsfu99

Signed-off-by: Richard Patel <me@terorie.dev>

riptl · 2021-02-16T11:25:07Z

I merged in the latest changes of main. I think the code is fine but CI is failing in unrelated parts.

github-actions · 2021-03-18T12:03:46Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-03-25T16:02:34Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

riptl · 2021-03-25T18:37:35Z

@rgs1 @williamsfu99 Can we re-open this? The underlying issue still remains. I will rebase asap

williamsfu99 · 2021-03-26T00:54:58Z

+  if (response.has_expires_in()) {
+    expires_in = std::chrono::seconds{response.expires_in().value()};
+  }
+  if (expires_in <= 0s) {


This inherently adds a check that the response's expires_in field doesn't contain some logically invalid negative duration. Probably a good thing to send a local reply instead of setting client cookies etc.

williamsfu99 · 2021-03-26T00:56:05Z

+    expires_in = std::chrono::seconds{response.expires_in().value()};
+  }
+  if (expires_in <= 0s) {
+    ENVOY_LOG(debug, "No default or explicit access token expiration after asyncGetAccessToken");


nit - "No valid default or explicit token expiration value found after asyncGetAccessToken"

williamsfu99

Can you also add some comments in docs/root/configuration/http/http_filters/oauth2_filter.rst explaining/giving examples of the new feature?

dio · 2021-09-23T22:05:06Z

@terorie @williamsfu99 @rgs1 Do you think we need to revive this to solve #14542? It seems like there are small nits to be resolved to bring this in. I can try to help if that makes sense.

cc. @orHayoun

riptl · 2021-09-23T22:15:15Z

@dio Sorry I've completely forgot about this. I'm not using the OAuth2 filter anymore so feel free to close from my side. I'm happy to finish up the PRs if the functionality is wanted though.

dio · 2021-09-24T00:51:46Z

@terorie no worries 🙂. From my reading, I think this PR is solving that issue and required. Please let me know if you want me to reopen this. Thank you!

riptl · 2021-10-03T13:47:19Z

I'm struggling at the moment to revive my workspace because the dev container is currently broken: #18388
So I probably won't be able to finish this PR this week. Sorry about that.

DiamondJoseph · 2022-07-04T10:30:44Z

@dio I would appreciate it being re-opened as the issue is still causing problems
@terorie it looks like the dev container has been fixed, if it's possible to get this over the line I would really appreciate.

If it's purely nitpicks I may be able to help, but I have not used C languages before.
/retest

repokitteh-read-only · 2022-07-04T10:30:48Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14625 (comment) was created by @DiamondJoseph.

see: more, trace.

repokitteh-read-only Bot added the api label Jan 11, 2021

repokitteh-read-only Bot assigned markdroth Jan 11, 2021

riptl changed the title ~~oauth2: WIP add "default_expires_in" config option~~ oauth2: add "default_expires_in" config option Jan 11, 2021

riptl mentioned this pull request Jan 11, 2021

[OAuth2] Incompatibility with access_token responses without "expire_in" #14542

Closed

riptl commented Jan 11, 2021

View reviewed changes

Comment thread source/extensions/filters/http/oauth2/oauth_client.h Outdated

riptl commented Jan 11, 2021

View reviewed changes

Comment thread source/extensions/filters/http/oauth2/oauth_client.h Outdated

riptl marked this pull request as ready for review January 11, 2021 02:08

riptl added 2 commits January 11, 2021 05:43

oauth2: WIP add "default_expires_in" config option

4fbed20

Signed-off-by: Richard Patel <me@terorie.dev>

use implicit null instead of optional

b7854e2

Signed-off-by: Richard Patel <me@terorie.dev>

riptl force-pushed the oauth-default-expire-in branch from 90dd0a3 to b7854e2 Compare January 11, 2021 06:21

repokitteh-read-only Bot removed the api label Jan 11, 2021

Base automatically changed from master to main January 15, 2021 23:02

Merge branch 'main' into oauth-default-expire-in

25bbd97

Signed-off-by: Richard Patel <me@terorie.dev>

repokitteh-read-only Bot added the api label Feb 15, 2021

proto_format.sh fix

80ba2ff

Signed-off-by: Richard Patel <me@terorie.dev>

riptl force-pushed the oauth-default-expire-in branch from 67ca4bc to 80ba2ff Compare February 16, 2021 00:37

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 18, 2021

github-actions Bot closed this Mar 25, 2021

williamsfu99 reviewed Mar 26, 2021

View reviewed changes

riptl mentioned this pull request Jul 6, 2022

oauth2: add "default_expires_in" config option #22029

Closed

phlax mentioned this pull request Dec 22, 2023

oauth2: Add default expiry for RFC compliance #31499

Merged

Conversation

riptl commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Jan 11, 2021

Uh oh!

repokitteh-read-only Bot commented Jan 11, 2021

Uh oh!

Uh oh!

Uh oh!

markdroth commented Jan 11, 2021

Uh oh!

rgs1 commented Feb 15, 2021

Uh oh!

riptl commented Feb 16, 2021

Uh oh!

github-actions Bot commented Mar 18, 2021

Uh oh!

github-actions Bot commented Mar 25, 2021

Uh oh!

riptl commented Mar 25, 2021

Uh oh!

williamsfu99 Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

williamsfu99 Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

williamsfu99 left a comment

Choose a reason for hiding this comment

Uh oh!

dio commented Sep 23, 2021

Uh oh!

riptl commented Sep 23, 2021

Uh oh!

dio commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riptl commented Oct 3, 2021

Uh oh!

DiamondJoseph commented Jul 4, 2022

Uh oh!

repokitteh-read-only Bot commented Jul 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

riptl commented Jan 11, 2021 •

edited

Loading

dio commented Sep 24, 2021 •

edited

Loading