supertokens-auth-react icon indicating copy to clipboard operation
supertokens-auth-react copied to clipboard

feat(sessiongrants): add support for session grants

Open porcellus opened this issue 3 years ago • 8 comments

Summary of change

Add support for session grants

Related issues

  • supertokens/core-driver-interface#36
  • supertokens/supertokens-core#390
  • supertokens/supertokens-node#278

Test Plan

TODO

Documentation changes

  • [ ] All examples in the docs for adding claimValidators to SessionAuth, we should have useMemo.
  • [ ] Need to show how to modify / override the default claims

Checklist for important updates

  • [ ] Changelog has been updated
  • [ ] frontendDriverInterfaceSupported.json file has been updated (if needed)
    • Along with the associated array in lib/ts/version.ts
  • [ ] Changes to the version if needed
    • In package.json
    • In package-lock.json
    • In lib/ts/version.ts
  • [ ] Had run npm run build-pretty
  • [ ] Had installed and ran the pre-commit hook
  • [ ] Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.
  • [ ] If added a new recipe interface, then make sure that the implementation of it uses NON arrow functions only (like someFunc: function () {..}).
  • [ ] If I added a new recipe, I also added the recipe entry point into the size-limit section of package.json with the size limit set to the current size rounded up.

Remaining TODOs for this PR

  • [ ] Check interface

porcellus avatar Mar 19 '22 01:03 porcellus

size-limit report 📦

Path Size
lib/build/index.js 31.72 KB (+12.53% 🔺)
recipe/session/index.js 32.04 KB (+21.79% 🔺)
recipe/thirdpartyemailpassword/index.js 122.23 KB (-3.03% 🔽)
recipe/thirdparty/index.js 105.83 KB (-4.87% 🔽)
recipe/emailpassword/index.js 109.41 KB (-2.58% 🔽)
recipe/passwordless/index.js 205.63 KB (+2.03% 🔺)
recipe/thirdpartypasswordless/index.js 220.26 KB (-0.63% 🔽)

github-actions[bot] avatar Mar 19 '22 01:03 github-actions[bot]

Should we make changes to the supertokens-website SDK first before making changes here?

rishabhpoddar avatar Mar 19 '22 09:03 rishabhpoddar

We should definitely make changes to supertokens-website first when implementing, this PR is more about the interface we are planning.

porcellus avatar Mar 19 '22 11:03 porcellus

Feedback (WIP):

  • [x] We should rename “ACCESS_TOKEN_UPDATED” to “ACCESS_TOKEN_PAYLOAD_UPDATED”
  • [x] Make role keys in access token smaller -> st-email-verification -> st-ev
  • [x] All examples in the docs for adding claimValidators to SessionAuth, we should have useMemo.
  • [x] Update the example to use SessionAuth everywhere.
  • [x] In SessionAuth, render null while the checks are happening if nothing has already been rendered.
  • [x] React 18 related update?
  • [x] Add a new SuperTokens HOC which the user can use to wrap their whole app in (we need to ensure SSR support before this). This would be equal to:
    <SessionAuth requireAuth={false} />
    

rishabhpoddar avatar Apr 05 '22 09:04 rishabhpoddar

Instead of adding an elaborate API right at the start, what if we do the following:

  • Ask users to handle missing claims for every API call they make. We can provide helper functions to extract the missing claims from the API calls like:

    try {
       await axios(...);
    } catch (e) {
       let missingClaim = supertokens.getMissingClaim(e); // this should also take a response from `fetch`
       if (missingClaim !== undefined) {
          // handle missing claim yourself... If you don't handle this, you can also treat this as a generic Something went wrong error as in most cases, it shouldn't come here if your app flow is setup correctly, and if the user is not purposely trying to manipulate things.
       }
    }
    
    • One question here is for things like missing role, how will they know which role is required and which is missing?
    • What is the structure of missingClaim?
  • Instead of making SessionAuth take a requiredClaim array, we can ask users to check for required claims in their component. This is simpler API for us to explain and document + for them to be able to handle:

    let {accessTokenPayload} = useSessionContext();
    if (supertokens.hasClaimInAccessTokenPayload(accessTokenPayload, RoleClaim.hasRole("admin")) {
      // TODO..
    } else {
       // TODO..
    } 
    
  • For things like Email verification where we want to handle it automatically, we can make the SessionAuth wrapper check the accessTokenPayload for if email verification claim is present or not. If it isn't we will navigate to the email verification page. Sure, we will miss cases of if their API throws missing email verification error, but that should never happen if they want the user to use their app only if their email is verified (cause the SessionAuth would always redirect).

  • We do not need to fire events for missing claims in supertokens-website repo as the above API would be good enough.


One problem with the above is that there are two new functions:

  • getMissingClaim
  • hasClaimInAccessTokenPayload

Do you think it's possible to combine them into one function? What would such a function be called?

rishabhpoddar avatar Apr 06 '22 07:04 rishabhpoddar

  1. I think we'd need to do this whatever API we provide. The requests should return an error (or throw) anyway; adding a helper function to get the id of the missing claim is a nice extra thing.
  2. I think this might lead to code repetition, but it could work out with good recommendations. I think this provides a bit more control/granularity at the cost of implementing things in the app instead of the SDK.
  3. getMissingClaim and hasClaimInAccessTokenPayload have different params, so I don't think it would make sense to combine them.
  • I'd rename hasClaimInAccessTokenPayload to checkClaims (and have it on the session recipe) because it may call their backend during checking.
  • I'd rename getMissingClaim to getMissingClaimFromResponse (or Error)

porcellus avatar Apr 06 '22 07:04 porcellus

  1. Structure of missing claims returned by the backend:
  • We should clarify that the id is the id of the claim validator (and maybe rename the id of the claim class to key)
  • We should have a metadata obj in the invalid claim response header (and in the body)
  • Change the isValid function of claim checkers (-> validators):
    • Rename to validate
    • It should return an object with the shape:
    type ClaimValidationResult = {isValid: true} || {isValid: false, reason?: JSONObject}
    
    • rename to ClaimValidator
  1. Session context:
  • should provide invalidClaim?: { id: string; reason?: JSONObject}
  • remove handleMissingClaim
  1. Session recipe:
  • provide async validateClaims(...)
  • fire API_INVALID_CLAIM event
  1. Figure out a way to add default handlers (and claim validators) from other recipes (e.g., EmailVerification)
  2. Renaming:
  • claimCheckers
  1. Remove:
  • defaultClaimValidators

porcellus avatar Apr 06 '22 11:04 porcellus

More feedback

  • [ ] All claim validators should have an ID which the user can use to know about the missing id, therefore we some claim validators require value like RoleClaim.hasRole(), those should become RoleClaim.hasRole.withValue("admin")
  • [ ] We get rid of all recipe specific auth wrappers:
    • [ ] requireAuth in SessionAuth is true by default
    • [ ] onRedirectToLogin should be optional and by default, it will redirect the user to websiteDomain/websiteBasePath
  • [ ] Since we have gotten rid of onMissingClaim from the SessionAuth wrapper, does the user have to redirect to email verification screen manually in the child component if email verification claim is invalid?
  • [ ] Should we have a default claims concept here too (just like we have on the backend)

rishabhpoddar avatar Apr 07 '22 13:04 rishabhpoddar