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

Implement the SignInAndUp Feature component #5

Closed
rishabhpoddar opened this issue Oct 15, 2020 · 12 comments
Closed

Implement the SignInAndUp Feature component #5

rishabhpoddar opened this issue Oct 15, 2020 · 12 comments
Labels
enhancement New feature or request

Comments

@rishabhpoddar
Copy link
Member

rishabhpoddar commented Oct 15, 2020

  • import { SignInAndUp } from "supertokens-auth-react/recipe/emailpassword"
  • The default implementation should be in ${websiteBasePath}/
  • The default theme should be SignInAndUpTheme
  • Props for this component (all optional, all must have a default implementation):
  • Will pass the following down to the Theme component (along with whatever the user passes):
  • They must be responsible to render a shadow-root to prevent CSS clashes
  • Should have its own config in the init function as explained in this comment
  • On sign in / up, this feature will first apply the frontend validators. If they pass, it will call the API. This means that the user does not need to call the validators themselves in case they are building thier own theme
@rishabhpoddar rishabhpoddar added the enhancement New feature or request label Oct 15, 2020
@rishabhpoddar
Copy link
Member Author

rishabhpoddar commented Oct 15, 2020

SignUpForm object passed down to theme

signUpForm: {
   callAPI: (formFields: {id: string, value: string}[]) => Promise<{
      status: "FIELD_ERROR"
      fields: {
         id: string,
         error: string
      }[]
   } | {
      status: "GENERAL_ERROR", message: string
   } | {
      status: "OK"
   }>,
   onSuccess: () => void,
   formFields: {
      id: string,
      label: string,
      placeholder?: string
      validate?: (value) => Promise<string | undefiend>,
      optional: boolean // default is false
   }[],
   styleFromInit: {...},
   privacyPolicyLink?: string,
   termsAndConditionsLink?: string
}

@rishabhpoddar
Copy link
Member Author

rishabhpoddar commented Oct 15, 2020

SignIn object passed down to the theme

signInForm: {
   callAPI: (formFields: {id: "email" | "password", value: string}[]) => Promise<{
      status: "FIELD_ERROR"
      fields: {
         id: "email" | "password",
         error: string
      }[]
   } | {
      status: "GENERAL_ERROR", message: string
   } | {
      status: "OK" | "WRONG_CREDENTIALS_ERROR"
   }>,
   onSuccess: () => void,
   formFields: {
      id: "email" | "password",
      label: string,
      placeholder?: string
      validate?: (value) => Promise<string | undefiend>,
   }[],
   styleFromInit: {...},
   resetPasswordURL?: string
}

@rishabhpoddar
Copy link
Member Author

rishabhpoddar commented Oct 15, 2020

Init configs for this feature

signInAndUpFeature: {
  defaultToSignUp: boolean, // default to true
   disableDefaultImplementation?: boolean,
   onSuccessRedirectURL?: string, // default is `/`
   signUpForm?: {
      formFields?: {
         id: "email" | "password" | string,
         label: string,
         placeholder?: string,
         validate?: (value) => Promise<string | undefined>,
         optional: boolean // default is false
      }[],
      style?: {...},
      privacyPolicyLink?: string,
      termsAndConditionsLink?: string
   },
   signInForm?: {
      formFields?: {
         id: "email" | "password",
         label: string,
         placeholder?: string
      }[],
      style?: {...},
      resetPasswordURL?: string
   }
}
  • If no validate function is given for id: "email", then we have our own which will:

    • Check that the email has the correct syntax
    • Anything else @NkxxkN ?
  • If no validate function is given for id: password, then we have our own which will:

    • Check a minimum of 8 characters, and a number
    • Has a maximum length of 100
    • Spaces are treated as special characters, just like * or ~
    • Anything else @NkxxkN ?
  • The validator functions for signInForm will be taken from signUpForm

  • How onSuccessRedirectURL will be used:

    • If onHandleSuccess is not defined or returns false after an action, then the browser is redirected to this URL.
    • We will use: history.pushState to redirect without reloading in an SPA.
  • How resetPasswordURL will be used:

    • If the onHandleForgotPasswordClicked is not defined or returns false, then the user will be redirected to resetPasswordURL

@rishabhpoddar
Copy link
Member Author

rishabhpoddar commented Oct 16, 2020

Props to Feature component

{
   onHandleSuccess(context: {action: "SESSION_ALREADY_EXISTS"} 
      | {action: "SIGN_IN_COMPLETE" | "SIGN_UP_COMPLETE", user: {id: string, email: string}, responseJson}): Promise<boolean>,
   onCallSignUpAPI(requestJson, headers): Promise<responseJson>,
   onCallEmailExistsAPI(value: string, headers): Promise<responseJson>,
   onCallSignInAPI(requestJson, headers): Promise<responseJson>, 
   doesSessionExist(): Promise<boolean>,
   onHandleForgotPasswordClicked(): Promise<boolean>,
}

@NkxxkN , as discussed, maybe it's better to ask them to send the response object instead of the JSON from the response. This way, we can handle API errors for them more naturally... However, this may have an issue: If they want to extract the JSON object from the response first to do something custom.. then how will they handle errors in those cases? We will have to tell them to throw an error anyway.. So perhaps it's best to keep it JSON and tell them to throw an error in case of status code >= 300.

One more thing I noticed is that we do not pass the URL as a function param. perhaps we should and they could use that if they want, else they could use their own URL in case they have implemented their own custom API.

@kant01ne
Copy link
Contributor

kant01ne commented Oct 16, 2020

Email validation

Check that the email has the correct syntax

Sounds good. A regexp should do the trick.

Password validation

Check a minimum of 5 characters

I think 8 is a minimum.

Is not some of the most common passwords (we can fetch a list of 100 top common ones?)

Interesting idea. Here is the most trusted list of common password (top 100): https://github.com/danielmiessler/SecLists/blob/master/Passwords/Common-Credentials/10-million-password-list-top-100.txt

On the same note, I've seen websites showing an equivalent of what is done in https://random-ize.com/how-long-to-hack-pass/ while you add your password. I thought it was a really cool experience and it was a nice way to encourage having a strong password.

Anything else ?

Do we enforce special characters, Uppercase and numbers by default? This is a very opinionated decision but it shows that SuperTokens takes authentication seriously.

Login redirection

how does the redirection work in an SPA? We do not want to fully reload the page right?

We will use: history.pushState to redirect without reloading in an SPA.

Note: Please feel free to remove this comment to keep the github issue clean. I wasn't sure how to collaborate properly here.

@rishabhpoddar
Copy link
Member Author

@NkxxkN I think password can be 8 chars with a number by default. Have changed the comment above to reflect that

@rishabhpoddar
Copy link
Member Author

@NkxxkN , the suggestions for password validation:

  • Common password
  • Time to break password UI
    can be added in a separate theme perhaps.

@kant01ne
Copy link
Contributor

  • Updated validateUserInput to validate to make it more consistent with the rest.

@kant01ne
Copy link
Contributor

One more thing I noticed is that we do not pass the URL as a function param. perhaps we should and they could use that if they want, else they could use their own URL in case they have implemented their own custom API.

In default case we do fetch(this.appInfo.apiDomain + "/signin", ...) for instance, but that is done internally (in an HttpRequest module). No need to pass the path (/signin), we could pass the apiDomain, but if they are about to manage the call themselves, it might be because they are calling another API? Anyway, they have initialised SuperTokens with this same URL so they will likely have it in a const in the same file right?

@rishabhpoddar
Copy link
Member Author

Actually, they could be using this function to just modify the request or do something else maybe and still want to call our default API. In that case, while they may know the apiDomain, they may not know it's /signin. Anyway, this is just a hypothetical and I'm not sure it even makes sense. So for now we don't need to pass the URL.

@kant01ne
Copy link
Contributor

kant01ne commented Nov 1, 2020

how does the redirection work in an SPA? We do not want to fully reload the page right?

I think we should actually reload the full page on login.

A lot of applications do load bunch of business related data on page load. If we do not reload the page, it will likely result in error states for most applications because there will be a session but the SaaS dashboards will not load properly.
We need to reload the page fully to make sure that these side effects do not happen. I think it's totally fine to do so as most applications do it.

@rishabhpoddar
Copy link
Member Author

A lot of applications do load bunch of business related data on page load. If we do not reload the page, it will likely result in error states for most applications because there will be a session but the SaaS dashboards will not load properly.
We need to reload the page fully to make sure that these side effects do not happen. I think it's totally fine to do so as most applications do it.

I'm OK with this, however, wouldn't people have this login in the componentDidMount function or using useEffect? Will those not get called if the page is shown (without a page reload)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants