supertokens / supertokens-node

Node SDK for SuperTokens core
https://supertokens.com
Other
292 stars 79 forks source link

Refactor appInfo #342

Open rishabhpoddar opened 2 years ago

rishabhpoddar commented 2 years ago
bhumilsarvaiya commented 2 years ago

If type is string:

If type is string[]:

If type is function (userContext) => Promise:

bhumilsarvaiya commented 2 years ago

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 commented 1 year ago

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

rishabhpoddar commented 1 year ago

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

nkshah2 commented 11 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

zaosoula commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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