Skip to content

examples: Add single page (React) app with OAuth#31534

Merged
phlax merged 11 commits intoenvoyproxy:mainfrom
phlax:examples-oauth-react
Jan 16, 2024
Merged

examples: Add single page (React) app with OAuth#31534
phlax merged 11 commits intoenvoyproxy:mainfrom
phlax:examples-oauth-react

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Dec 27, 2023

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:]

@phlax phlax marked this pull request as draft December 27, 2023 13:06
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 27, 2023

cc @mmorel-35 this will need the dependatool npm update we discussed

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 27, 2023

/docs

@repokitteh-read-only
Copy link
Copy Markdown

Docs for this Pull Request will be rendered here:

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

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

🐱

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

see: more, trace.

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 27, 2023

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 27, 2023

requires #31499

@phlax phlax force-pushed the examples-oauth-react branch 3 times, most recently from be659e9 to 34c48dc Compare December 27, 2023 16:50
@mmorel-35
Copy link
Copy Markdown
Contributor

cc @mmorel-35 this will need the dependatool npm update we discussed

I see, I'm going to provide a PR on toolshed before the end of the week to fix this.

@phlax phlax force-pushed the examples-oauth-react branch from 34c48dc to 52d0cd6 Compare December 27, 2023 17:00
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 27, 2023

I see, I'm going to provide a PR on toolshed before the end of the week to fix this.

i made an attempt at it here envoyproxy/toolshed#1374 - it needs some tests, cleanups and to be tested yet

@phlax phlax changed the title [WIP] examples: Add single page (React) app with oAuth [WIP] examples: Add single page (React) app with OAuth Dec 27, 2023
@phlax phlax force-pushed the examples-oauth-react branch from 52d0cd6 to 7f60e27 Compare December 27, 2023 20:55
@phlax phlax force-pushed the examples-oauth-react branch 2 times, most recently from 3c66725 to 4de9997 Compare December 28, 2023 11:00
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Dec 28, 2023

@phlax phlax force-pushed the examples-oauth-react branch 2 times, most recently from 9b90c11 to 5fcc6a6 Compare December 28, 2023 16:21
Comment thread examples/single-page-app/verify.sh Outdated
Comment thread examples/single-page-app/ui/src/components/Home.tsx Outdated
Comment thread docs/root/start/sandboxes/single-page-app.rst Outdated
Comment thread examples/single-page-app/verify.sh Outdated
Comment thread examples/single-page-app/verify.sh Outdated
Comment thread examples/single-page-app/ui/routes.jq Outdated
Comment thread examples/single-page-app/ui/routes.jq Outdated
Comment thread examples/single-page-app/ui/src/@types/app.d.ts Outdated
Comment thread docs/root/start/sandboxes/single-page-app.rst Outdated
Comment thread docs/root/start/sandboxes/single-page-app.rst Outdated
Comment thread docs/root/start/sandboxes/single-page-app.rst Outdated
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 11, 2024

ill try and get to this today - been a bit snowed under with release stuff

@phlax phlax force-pushed the examples-oauth-react branch from 573267e to 70fd8d4 Compare January 11, 2024 15:34
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 11, 2024

@htuch this should be ready for another review, apologies for force-push, commits were preserved after rebase

@phlax phlax added this to the v1.29.0 milestone Jan 11, 2024
Comment thread examples/single-page-app/myhub/api.py Outdated
ravenblackx
ravenblackx previously approved these changes Jan 12, 2024
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Drop this stanza.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
means the provided access token will be forwarded to upstreams proxied by Envoy unless explicitly excluded.
means the provided access token will be forwarded to any cluster / upstreams proxied by Envoy for this HTTP filter chain. In general, upstreams should be trusted in this scenario. If untrusted upstreams are present, care will need to be taken to not only disable the OAuth2 filter on a per-route basis but to also remove sensitive cookies with bearer tokens, etc. This scenario is outside the scope of this sandbox.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ill work this in

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i may be wrong but i think the important thing is removing the cookie/header - not sure that enabling/disabling the oauth filter would achieve this or help - besides in this situation you would need to disable it for given upstreams anyway (whether by pass_through or disabling the filter) just to get it to work correctly

ive updated on this basis

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does production mean when we're using a dummy OAuth backend as per previous paragraph?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it means not served by a dev backend - static assets essentially - i added gzip/tls as pointers to what else you might add

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

its hard not to call it production becase that is what js/node calls it when you build the assets rather than serve them

eg - it reads .env.production in that circumstance

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I fully follow - shouldn't the access token creation obtain sufficient scope to be able to reuse the OAuth cookie in future accesses to other paths?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

if you dont restrict what triggers the oauth flow then its triggered on the assets

this can cause redirect loops which take over the client (browser) machine and flood the proxy

not knowing this when i started i really struggled to get anything working so i think we need to say something - here im trying to highlight the strategy/config used to avoid that (ie inverse matching)

Comment thread docs/root/start/sandboxes/single-page-app.rst Outdated
phlax and others added 11 commits January 15, 2024 09:13
Signed-off-by: Ryan Northey <ryan@synca.io>
Co-authored-by: htuch <htuch@users.noreply.github.com>
Signed-off-by: phlax <phlax@users.noreply.github.com>
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>
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>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@ravenblackx
Copy link
Copy Markdown
Contributor

Guess I should have said - my approval is typescript-specific and continues to apply so long as no typescript files are changed. I'm not going to re-approve since other reviewers are still reviewing other stuff, but feel free to assume from here on that the typescript is already reviewed.

@ravenblackx ravenblackx removed their assignment Jan 16, 2024
Copy link
Copy Markdown
Contributor

@ravenblackx ravenblackx left a comment

Choose a reason for hiding this comment

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

With other comments mostly addressed, approving as on-call to unblock it for release.

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 16, 2024

@htuch landing this to get it in - we can follow as required

@phlax phlax merged commit 7d047cd into envoyproxy:main Jan 16, 2024
@htuch
Copy link
Copy Markdown
Member

htuch commented Jan 17, 2024

I think my main concerns are addressed. I haven't walked through the Github production parts but the core guidance seems generally correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants