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

Refactor appInfo #342

Open
5 of 37 tasks
rishabhpoddar opened this issue Jul 5, 2022 · 10 comments
Open
5 of 37 tasks

Refactor appInfo #342

rishabhpoddar opened this issue Jul 5, 2022 · 10 comments

Comments

@rishabhpoddar
Copy link
Member

rishabhpoddar commented Jul 5, 2022

  • Allow multiple websiteDomains. Right now we accept just one value, which causes issues when sharing the same backend for multiple frontends (dev env, testing env)

    • We should accept an array of domains and determine the cookie settings according to that.
    • This causes issues with knowing which domain to use for email verification and reset password links - how can we fix that?
    • What if we want to set sameSite for cookie depending on the origin server? For example, if it's querying from localhost, we set it to none, else we set it to lax (if it's a same top level domain)
  • The name websiteDomain is confusing for use where we have mobile apps. Maybe we can change the name to frontendDomain?

  • We should accept non https / http URLs for frontend native apps.

  • Issue with multiple sub domains:

    • If login is on one sub domain (auth.example.com), value of websiteDomain can be https://auth.example.com, but then the name websiteDomain is confusing. It has to imply that this is the URL in which the login UI is shown.
    • If showing a login page per sub domain:
      • What will be the value of websiteDomain array? We can't just hardcode all the domains since they might be dynamically generated (when a new user signs up, they will get a new sub domain automatically)
      • How will the reset password / email verification links be generated in this case?
  • This has affect on the docs (the appInfo form)

  • This has effect on analytics API -> it will change based on changes to the structure of websiteDomain.

  • Do we need to also have multiple apiDomains? Not sure.. We should check issues on discord about people expressing issues with just one apiDomain.

  • Usage of apiGatewayPath is not at all obvious. Maybe this is a documentation issue, but is there something we can do on the backend to eliminate this config and just automatically infer it?

  • Should we allow regex for websiteDomain? Why or why not?

    • We could do it, but the user can also just define a function and do the regex check in there and return the right websiteDomain - so we decided not to do it cause it's simple enough for the user to do
  • Need to investigate if there are any other issues with the appInfo object?

  • Should we allow multiple values for apiDomain? For example, if someone is using one single backend process which can be reached via many hosts, how do we handle that?

  • If a function is defined for origin, what should happen if the request is from an origin that's not a part of the whitelist in the function?

  • We should also discuss how we can enable network interception on the frontend SDK related to several backends having the same domain but different ports (for example localhost:8080 and localhost:8081). In this case we have decided to ignore the port in the api domain when deciding to do interception

  • Redo example apps for (for deeplinks):

    • electron app
    • mobile apps
    • Multi tenant app with different sub domains per domain
    • capacitor app
  • Add tests for normalisedurldomain and normlaisedurlpath to work with any protocol (deep links)

  • Add tests where we pass in deep link to supertokens.init and check for session token properties.

  • Add tests for how the anti csrf stuff behaves for without request / response session functions

  • Apple redirection stuff - get origin from state instead of origin var

  • One edge case to think about here: If the sign in etc APIs are called in an m2m situation, how will the function method work? Cause in that case, returning undefined may not be the right thing to do in the function for the user.

  • Changes to the dashboard APIs where in we generate email verification emails etc.. how will those be affected?

    • For example, now the email verification UI on the dashboard will have to let users pick from a set of domains?
  • The APIs for dashboard will need to take into account that the request object is not one of the allowed origins perhaps?

  • Always doing based on req does not work. For example, in case of multi tenancy, we want to get the domain based on tenant id as well to avoid mismatch of origin and tenant.. In case of email verification emails, we want to do it based on which domain the user has access to as well?

  • Fix this as well: cookieSameSite resolution issue #506

  • Change the apple redirect POST API to be able to handle non http protocol links as well. Currently we try to convert the value in state to a URL object which fails for custom protocols, ideally it should just use the value set in the state. One more thing is to be able to attach query params in a custom way, for example the library we recommend for apple sign in for Flutter requires that the query is added in a specific place. Maybe we can check for a specific pattern/string in the state url and if present attach the query params there instead of the end

  • Frontend should ignore the port of the api domain when deciding to do interception. This is for all FE SDKs

  • It should be possible to decide between cookie and header based aurth depending on the request origin

  • In docs:

    • when all SDKs have this feature, change multi tenant, sub domain flow part to say that use this instead of confguring websiteDomain
    • Do not use websiteDomain anywhere in the docs and change appinfo reference to also use origin.
    • The appinfo form in the docs needs to change to use origin instead of websiteDomain. It also needs to allow users to select multiple origins and put a function in there.
  • Once all the backend SDKs have origin in app info, mark the websiteDomain property as deprecated

  • Make sure that our email sending service on api.supertokens.com works for deep links too.

  • Modify multi tenancy example app in auth-react (for sub domain one) to use the new origin function on the backend instead of overriding sendEmail functions

@bhumilsarvaiya
Copy link
Contributor

bhumilsarvaiya commented Jul 11, 2022

  • have two functions:

    • get origin value for request: determine websiteDomain
  • allow user to set allowed origins: function | string | string[]

If type is string:

  • It's the same as right now.

If type is string[]:

  • The default implementation of sameSite:
    • read from origin header, and if it matches any of these string[], then determine value based on origin
    • if it doesn't match or is undefined, then determine value based on all the values in string[].
  • For reset password / email verification links:
    • read from origin header, and if it matches any of these string[], then use that domain.
    • if it doesn't match or is undefined, then we use the 0th element of the string[]

If type is function (userContext) => Promise:

  • The default implementation of sameSite:
    • then you just call the function and use the result to determine value of sameSite.
  • For reset password / email verification links:
    • then you just call the function and use the result to determine value of sameSite then use that domain.

@bhumilsarvaiya
Copy link
Contributor

How the above solution solves the above problem:

We should accept an array of domains and determine the cookie settings according to that.

  • we accept array of allowed website domains

This causes issues with knowing which domain to use for email verification and reset password links - how can we fix that?

  • the domain for email verification and reset password links will be dependent on the origin of the incoming request
  • if the origin is undefined, we will use the allowed website domain. if allowed website domain is just a string, we'll directly use that value. If allowed website domain is array of string, we'll use the first element of the array

What if we want to set sameSite for cookie depending on the origin server? For example, if it's querying from localhost, we set it to none, else we set it to lax (if it's a same top level domain)

  • The above solution calculates the cookie sameSite value dynamically, i.e. based on the origin of the incoming request

The name websiteDomain is confusing for use where we have mobile apps. Maybe we can change the name to frontendDomain?

  • We should instead call this allowedOrigins. This is far more intuitive and people are already familiar with term from the CORS package. The usage here is kind of similar too

We should accept non https / http URLs for frontend native apps.

  • User can simply add them as allowed origin

If login is on one sub domain (auth.example.com), value of websiteDomain can be https://auth.example.com, but then the name websiteDomain is confusing. It has to imply that this is the URL in which the login UI is shown.

  • Based on the above solution, the name of the parameter is allowedOrigins. Not sure if it less confusing or not. I have a bias now and I think it is less confusing. Also, this doesn't really need to imply now that this is the domain where the login UI is shown as long as it is able to make user to add all the domains that they are gonna use (which would include the auth domain also)

What will be the value of websiteDomain array? We can't just hardcode all the domains since they might be dynamically generated (when a new user signs up, they will get a new sub domain automatically)

  • Fetching the origin from the request solves the problem where the allowed website domains can be dynamic and not hardcoded. Also, user will pass allowed origins as a function instead of string | string[]. In the function, user (using the library) can check for the origin themselves and validate if they support that origin or not

How will the reset password / email verification links be generated in this case?

  • Just as discussed above

This has affect on the docs (the appInfo form)

  • Instead of asking users for website domain, we ask for allowed origins

Do we need to also have multiple apiDomains? Not sure.. We should check issues on discord about people expressing issues with just one apiDomain.

  • No changes to apiDomain for now

This has effect on analytics API -> it will change based on changes to the structure of websiteDomain.

  • One way to solve this would be to store a list of distinct website domains encountered while the server is running. Whenever a new origin is inserted in this list, we should send a telemetry ping

@whydinkov
Copy link

Hi guys, great project! Wanted to ask about the status of this one, and will it be coming to Python backend soon?

@rishabhpoddar
Copy link
Member Author

hey @whydinkov this will be worked on in the coming 2-3 months

@nkshah2
Copy link
Contributor

nkshah2 commented Oct 10, 2023

For those who are following this issue, we have resumed work on this and are approaching this in separate stages:

  1. Allowing for multiple website domains to be usable for a single backend
  2. Allowing for non http protocols to be usable (currently custom protocols result in invalid url errors)
  3. Further evaluating changes to the app info object to improve the experience (based on feedback, this will be a more longer term stage compared to the rest)

@zaosoula
Copy link

zaosoula commented Feb 27, 2024

@nkshah2 Hi, any news on point n°2? I use capacitorJs and my origin scheme is capacitor:// I set the origin in the appInfo but the normaliseURLDomainOrThrowError prevents me from using supertokens with capacitorjs

@rishabhpoddar
Copy link
Member Author

jeu @zaosoula we havent had time to work on this. You could consider making a PR for it :)

@zaosoula
Copy link

@rishabhpoddar No problem that's why I asked, I need to fix this ASAP, can you provide me with explanations on why this function is needed/used

@rishabhpoddar
Copy link
Member Author

This function is required to normalise any input url to only get the domain (and protocol) part of it for further use. For example, if you give it example.com/hello, it will return https://example.com. Notice that it added https, and also removed the path. This can then later on be used anywhere like normalisedDomain.getAsDangerousString() + "/sompath". We do not have to worry if the domain has a trailing / or not.

Also, it acts as an enforcer everywhere that whenever we are passing around a domain / path in our SDK, we must normalise is first cause in all places, we accept the object of type NormaliseURLDomain instead of a string.

@zaosoula
Copy link

Okay thank you, i will be back if a fix in 20 minutes

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

No branches or pull requests

6 participants