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

fix: update basePath config in index.js #1485

Closed
wants to merge 1 commit into from
Closed

Conversation

OryTiki
Copy link
Contributor

@OryTiki OryTiki commented Aug 3, 2023

AFAI understood, the basePath in the SDK configuration should point to the proxy and not to .projects.oryapis.com. When pointing to oryapis.com or the ORY_SDK_URL, the login works, but calling createBrowserLogoutFlow returns a logout url, which leads to an error (Unable to log out because the logout token in the URL query does not match the session cookie).
TBH I'm not sure if the basePath should point to http://localhost:4000/.ory or http://localhost:4000

AFAI understood, the basePath in the SDK configuration should point to the proxy and not to <slug>.projects.oryapis.com.
When pointing to oryapis.com or the ORY_SDK_URL, the login works, but when you want to add the logout, you get an error (Unable to log out because the logout token in the URL query does not match the session cookie.)
@@ -9,7 +9,7 @@ var sdk = require("@ory/client")
var ory = new sdk.FrontendApi(
new sdk.Configuration({
basePath:
process.env.ORY_SDK_URL || "https://playground.projects.oryapis.com",
"http://localhost:4000/.ory",
Copy link
Member

Choose a reason for hiding this comment

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

This API call doesn't need to go though the proxy - so it's easier / better / more flexible if it just points to the regular SDK url. I think this was fine the way it was, or did you encounter any issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did encounter an issue (on localhost) when trying to implement the logout method via createBrowserLogoutFlow. Redirecting the user to the provided logout_url, led to the error: Unable to log out because the logout token in the URL query does not match the session cookie.
My fix was to set the basePath to localhost:4000. After that change, the logout_url is working as desired

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe the guide is inconsistent with the example code. or it has been changed.
I don't think this example uses the integrations package to proxy requests to itself and thus you need to spin up the proxy / tunnel externally.

Using the upstream Ory APIs would thus give infinite redirect loops since the cookie is not set correctly. So i think this change is actually correct.

Copy link
Member

@aeneasr aeneasr Aug 3, 2023

Choose a reason for hiding this comment

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

Why is the CI then failing? :D

The set up for this app is ory proxy -> node app iirc. The node app calling the ory APIs will not send any cookies anyways. I don't think this makes sense to loop it again through the proxy: ory proxy->node app->ory proxy->oryapis.com

We only need the proxy for the browser to send the right cookies. The node app doesn't send cookies anyways (because it's an SDK client). It can only forward those that were included in the incoming request.

Will need to look into the reason why logout_url isn't working though

Copy link
Contributor

Choose a reason for hiding this comment

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

i'll take a look

@vinckr vinckr self-assigned this Aug 22, 2023
@vinckr
Copy link
Member

vinckr commented Sep 6, 2023

closing this but should be investigated

@vinckr vinckr closed this Sep 6, 2023
@aeneasr
Copy link
Member

aeneasr commented Sep 6, 2023

If it needs investigation please create an issue or keep this open!

@aeneasr aeneasr reopened this Sep 6, 2023
@aeneasr aeneasr closed this Feb 13, 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.

4 participants