Skip to content

Conversation

@porcellus
Copy link
Collaborator

@porcellus porcellus commented Mar 18, 2023

Summary of change

(A few sentences about this PR)

Related issues

Test Plan

TODO

Documentation changes

TODO

Checklist for important updates

  • Changelog has been updated
  • coreDriverInterfaceSupported.json file has been updated (if needed)
    • Along with the associated array in lib/ts/version.ts
  • frontendDriverInterfaceSupported.json file has been updated (if needed)
  • 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 have added a new web framework, update the add-ts-no-check.js file to include that
  • If added a new recipe / api interface, then make sure that the implementation of it uses NON arrow functions only (like someFunc: function () {..}).
  • If added a new recipe, then make sure to expose it inside the recipe folder present in the root of this repo. We also need to expose its types.

Remaining TODOs for this PR

  • Tests
  • Base PR merge

@porcellus porcellus mentioned this pull request Mar 18, 2023
12 tasks
@porcellus porcellus marked this pull request as ready for review March 24, 2023 03:02

getHandle(userContext?: any): string;

getTokensDangerously(): {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
getTokensDangerously(): {
getAllSessionTokensDangerously(): {

userContext,
});

if (res.status === "OK" && res.session !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (res.status === "OK" && res.session !== undefined) {
if (res.status === "OK") {

Base automatically changed from feat/access_token_to_jwt to feat/access_token_to_jwt_base March 26, 2023 20:50
- Added support for new access token version

### Added

Copy link
Contributor

Choose a reason for hiding this comment

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

For the getSessionWithoutModifyingResponse function, if it return an invalid claim error, the err object from that response can directly be sent to the frontend as a JSON serialisable with 403? Or does something else need to be done

CHANGELOG.md Outdated

### Added

- Added `createNewSessionWithoutModifyingResponse`, `getSessionWithoutModifyingResponse`, `refreshSessionWithoutModifyingResponse` to the Session recipe.
Copy link
Contributor

Choose a reason for hiding this comment

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

can you share code snippet here of how to actually use getSessionWithoutModifyingResponse and handle all the errors + success state from it?

const res = yield recipeInterfaceImpl.getSessionWithoutModifyingResponse({
accessToken,
antiCsrfToken,
options,
Copy link
Contributor

Choose a reason for hiding this comment

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

related to dynamic vs static keys, we decided to remove the useDynamicKey arg from createNewSession (so just on a global level), and to check for the kid in the JWT post session verification based on the value of the global variable (do this check if the token is >= v3). Note that if the kid is a UUID only, then we treat it as a static key.

If the setting vs the kid format doesn't match, then we throw try refresh token error.

options,
userContext,
});
if (res.status === "OK" && res.session !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • we need to create just one recipe function for createNewSession which doesn't take a req / res (but the request will be passed to it via userContext)
  • The createNewSession(req, res, ..) will also call this function, and set the userContext correctly.
  • The createNewSessionWithoutReqeustResponse will also call this same function, but will not have req in userContext

}): Promise<SessionContainerInterface | undefined>;
}): Promise<
| { status: "OK"; session: SessionContainerInterface }
| { status: "TOKEN_VALIDATION_ERROR"; error: any }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call it "UNAUTHORISED"

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's evaluate this based on the snippet you provide: Snippet should compare how to use this function vs using getSession function

Copy link
Collaborator Author

@porcellus porcellus Mar 29, 2023

Choose a reason for hiding this comment

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

async function getSessionWithoutRequestOrErrorHandler(
    req: express.Request,
    resp: express.Response,
) {
    const accessToken = req.headers.authorization?.replace(/^Bearer /, "");

    // We only need to split this declaration (and have the else statement) if the session is optional
    let session;
    if (!accessToken) {
        // This means that the user doesn't have an active session
        return resp.status(401).json({ message: "try again " }); // Or equivalent...
    } else {
        const result = await Session.getSessionWithoutRequestResponse(accessToken, undefined, { antiCsrfCheck: false });

        // Note that the return type forces the user to have (or at least notice) error handling
        if (result.status === "CLAIM_VALIDATION_ERROR") {
            return resp.status(403).json(result); // Or equivalent...
        } else if (result.status !== "OK") {
            // We could need separate handling here if we expect non-ST access tokens
            return resp.status(401); // Or equivalent...
        }
        session = result.session;
    }
    // API code...
    
    if (session) {
        const tokens = session.getAllSessionTokensDangerously();
        if (tokens.accessAndFrontTokenUpdated) {
            resp.set("st-access-token", tokens.accessToken);
            resp.set("front-token", tokens.frontToken);
        }
    }
}

async function getSessionWithoutRequestWithErrorHandler(
    req: express.Request,
    resp: express.Response,
) {
    const accessToken = req.headers.authorization?.replace(/^Bearer /, "");

    // We only need to split this declaration (and have the else statement) if the session is optional
    let session;
    if (!accessToken) {
        // This means that the user doesn't have an active session
        return resp.status(401).json({ message: "try again " });
    } else {
        const result = await Session.getSessionWithoutRequestResponse(accessToken, undefined, { antiCsrfCheck: false });

        // Note that the return type forces the user to have (or at least notice) error handling
        if (result.status === "CLAIM_VALIDATION_ERROR") {
            throw new Session.Error({
                type: "INVALID_CLAIMS",
                payload: result.claimValidationErrors,
                message: "INVALID_CLAIMS",
            });
        } else if (result.status !== "OK") {
            // We could need separate handling here if we expect non-ST access tokens
            throw result.error;
        }
        session = result.session;
    }
    // API code...
    if (session) {
        const tokens = session.getAllSessionTokensDangerously();
        if (tokens.accessAndFrontTokenUpdated) {
            resp.set("st-access-token", tokens.accessToken);
            resp.set("front-token", tokens.frontToken);
        }
    }
}

Copy link
Contributor

@rishabhpoddar rishabhpoddar Mar 30, 2023

Choose a reason for hiding this comment

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

  • check that this all actually works
  • we decided to introduce result.response for invalid claim error, which matches the fdi json being sent. And also keep result.error in case we want to throw.
  • Add: How to use createNewSessionWithoutRequestResponse and how to use refreshSessionWithoutRequestResponse

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

async function createNewSessionWithoutRequestResponse(req: express.Request, resp: express.Response) {
    const userId = "user-id"; // This would be fetched from somewhere

    const result = await Session.createNewSessionWithoutRequestResponse(userId);

    const tokens = result.session.getAllSessionTokensDangerously();
    if (tokens.accessAndFrontTokenUpdated) {
        resp.set("st-access-token", tokens.accessToken);
        resp.set("front-token", tokens.frontToken);
        resp.set("st-refresh-token", tokens.refreshToken);
        if (tokens.antiCsrfToken) {
            resp.set("anti-csrf", tokens.antiCsrfToken);
        }
    }
}

async function refreshSessionWithoutRequestResponse(req: express.Request, resp: express.Response) {
    const refreshToken = req.headers.authorization?.replace(/^Bearer /, "");

    if (!refreshToken) {
        // This means that the user doesn't have an active session
        return resp.status(401);
    } else {
        const result = await Session.refreshSessionWithoutRequestResponse(refreshToken, true);
        if (result.status !== "OK") {
            return resp
                .status(401)
                .set("st-access-token", "")
                .set("set-refresh-token", "")
                .set("front-token", "remove"); // Or equivalent...
        }
        const tokens = result.session.getAllSessionTokensDangerously();
        if (tokens.accessAndFrontTokenUpdated) {
            resp.set("st-access-token", tokens.accessToken);
            resp.set("front-token", tokens.frontToken);
            resp.set("st-refresh-token", tokens.refreshToken);
            if (tokens.antiCsrfToken) {
                resp.set("anti-csrf", tokens.antiCsrfToken);
            }
        }
    }
}

static async getSessionWithoutRequestResponse(
accessToken: string,
antiCsrfToken?: string,
options?: VerifySessionOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

omit sessionRequired from VerifySessionOptions

}): Promise<SessionContainerInterface>;
}): Promise<
| { status: "OK"; session: SessionContainerInterface }
| { status: "UNAUTHORISED"; clearTokens: boolean }
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add an err to all of the non-ok status types + remove clearTokens

req = frameworks[SuperTokens.getInstanceOrThrowError().framework].wrapRequest(req);
}
userContext = setRequestInUserContextIfNotDefined(userContext, req);
logDebugMessage("getSession: Wrapping done");
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong message

}
if (result.status === "UNAUTHORISED") {
throw new SessionError({
message: "asdf",
Copy link
Contributor

Choose a reason for hiding this comment

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

why asdf?

});
}

static async getSessionWithoutRequestResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment about anticsrf checks (and failure returns TRY_REFRESH_TOKEN_ERROR):

  • if anti-csrf value in access token and input antiCsrfToken don't match
  • if we are supposed to do anti-csrf check (based on the options and the anti-csrf setting in the config), and input antiCsrfToken is undefined. This implies that if someone is using header based auth, they should set the options to have antiCsrf check as false.

Comment on lines 270 to 273
/*
In all cases: if sIdRefreshToken token exists (so it's a legacy session) we clear it.
Check http://localhost:3002/docs/contribute/decisions/session/0008 for further details and a table of expected behaviours
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/*
In all cases: if sIdRefreshToken token exists (so it's a legacy session) we clear it.
Check http://localhost:3002/docs/contribute/decisions/session/0008 for further details and a table of expected behaviours
*/

res = frameworks[SuperTokens.getInstanceOrThrowError().framework].wrapResponse(res);
}
logDebugMessage("createNewSession: Wrapping done");
setRequestInUserContextIfNotDefined(userContext, req);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setRequestInUserContextIfNotDefined(userContext, req);
userContext = setRequestInUserContextIfNotDefined(userContext, req);

@porcellus porcellus merged commit 9312d6c into feat/access_token_to_jwt_base Mar 30, 2023
@porcellus porcellus deleted the feat/session_functions_with_no_req branch March 30, 2023 10:46
rishabhpoddar added a commit that referenced this pull request May 4, 2023
* feat!: rename sessionData handling functions (#519)

* feat\!: rename sessionData handling functions

* feat: rename sessionData in param and return types to sessionDataInDatabase

* feat: access token to jwt (#512)

* feat(access token to jwt): initial changes

* feat(access_token_to_jwt): initial implementation

* refactor: removed unused code/fixed names

* test: add/fix tests

* fix: implement review comments

* chore: deprecate jwt config option

* feat: implement review comments

* test: update&expand tests

* test: expand testing + changelog

* feat: re-add openId overrides into session config + moved protected prop filtering

* chore: add migration guide to changelog

* fix: delete protected props before merging claim updates during assertClaims

* fix: more consistent payload handling

* fix: update tests and add separate handling for v2 tokens

* feat: add new functions based on session ADR 0030 (#520)

* feat(access token to jwt): initial changes

* feat(access_token_to_jwt): initial implementation

* refactor: removed unused code/fixed names

* test: add/fix tests

* fix: implement review comments

* chore: deprecate jwt config option

* feat: add new functions based on session ADR 0030

* chore: update CHANGELOG

* feat: implement review comments

* test: update&expand tests

* test: expand testing + changelog

* feat: re-add openId overrides into session config + moved protected prop filtering

* chore: add migration guide to changelog

* fix: delete protected props before merging claim updates during assertClaims

* feat: add tests + small fixes/exports

* fix: more consistent payload handling

* refactor: implement review comments

* feat: make the invalid claims response and the return value of getSessionWithoutModifyingResponse match

* feat: single override for session handling funcs with&without request

* feat: remove useDynamicAccessTokenSigningKey from createNewSession and check it in verify

* feat: review comments

* refactor: renamed TOKEN_VALIDATION_ERROR to UNAUTHORISED

* chore: update changelog

* refactor: self-review fixes

* feat: make it easy to both throw or build response if claim validation failed

* fix: update createJWT in openId and session recipes + CDI version (#538)

* refactor: minor cleanup +fixes from other SDKs for access token to jwt (#542)

* fix: update createJWT in openId and session recipes + CDI version

* docs: add comment to explain the combinedJWKS func

* fix: fix migration guide in changelog

* refactor: introduce constant for 100 years + fix comment

* feat: changed withoutreqres methods to throw instead of returning status + test fixes

* feat: rename accessTokenPayload to customClaimsInAccessTokenPayload in session info

* Update CHANGELOG.md

---------

Co-authored-by: Rishabh Poddar <rishabh.poddar@gmail.com>

* feat: add iss into access tokens + update/fix tests (#546)

* fix: type fixes + small test fixes/extension (#547)

* feat: add iss into access tokens + update/fix tests

* test: further test fixes

* fix: fix types of both getSession versions

* refactor: smaller cleanup/minor fixes (#556)

* feat: add iss into access tokens + update/fix tests

* test: further test fixes

* fix: fix types of both getSession versions

* chore: fix CDI version in changelog

* chore: fix typo in changelog

* fix: fix access token payload for legacy tokens + minor consistency fixes

* docs: update comment to match new interface

* chore: update changelog

* fix: add missing return

* refactor: move v2 specific code to the appropriate branch

---------

Co-authored-by: Rishabh Poddar <rishabh.poddar@gmail.com>
rishabhpoddar added a commit that referenced this pull request Jun 30, 2023
* feat!: rename sessionData handling functions (#519)

* feat\!: rename sessionData handling functions

* feat: rename sessionData in param and return types to sessionDataInDatabase

* feat: access token to jwt (#512)

* feat(access token to jwt): initial changes

* feat(access_token_to_jwt): initial implementation

* refactor: removed unused code/fixed names

* test: add/fix tests

* fix: implement review comments

* chore: deprecate jwt config option

* feat: implement review comments

* test: update&expand tests

* test: expand testing + changelog

* feat: re-add openId overrides into session config + moved protected prop filtering

* chore: add migration guide to changelog

* fix: delete protected props before merging claim updates during assertClaims

* fix: more consistent payload handling

* fix: update tests and add separate handling for v2 tokens

* feat: add new functions based on session ADR 0030 (#520)

* feat(access token to jwt): initial changes

* feat(access_token_to_jwt): initial implementation

* refactor: removed unused code/fixed names

* test: add/fix tests

* fix: implement review comments

* chore: deprecate jwt config option

* feat: add new functions based on session ADR 0030

* chore: update CHANGELOG

* feat: implement review comments

* test: update&expand tests

* test: expand testing + changelog

* feat: re-add openId overrides into session config + moved protected prop filtering

* chore: add migration guide to changelog

* fix: delete protected props before merging claim updates during assertClaims

* feat: add tests + small fixes/exports

* fix: more consistent payload handling

* refactor: implement review comments

* feat: make the invalid claims response and the return value of getSessionWithoutModifyingResponse match

* feat: single override for session handling funcs with&without request

* feat: remove useDynamicAccessTokenSigningKey from createNewSession and check it in verify

* feat: review comments

* refactor: renamed TOKEN_VALIDATION_ERROR to UNAUTHORISED

* chore: update changelog

* refactor: self-review fixes

* feat: make it easy to both throw or build response if claim validation failed

* fix: update createJWT in openId and session recipes + CDI version (#538)

* refactor: minor cleanup +fixes from other SDKs for access token to jwt (#542)

* fix: update createJWT in openId and session recipes + CDI version

* docs: add comment to explain the combinedJWKS func

* fix: fix migration guide in changelog

* refactor: introduce constant for 100 years + fix comment

* feat: changed withoutreqres methods to throw instead of returning status + test fixes

* feat: rename accessTokenPayload to customClaimsInAccessTokenPayload in session info

* Update CHANGELOG.md

---------

Co-authored-by: Rishabh Poddar <rishabh.poddar@gmail.com>

* feat: add iss into access tokens + update/fix tests (#546)

* fix: type fixes + small test fixes/extension (#547)

* feat: add iss into access tokens + update/fix tests

* test: further test fixes

* fix: fix types of both getSession versions

* removes import from url

* tries to use node fetch

* tries to use node fetch

* refactor: replaced node-fetch and axios with cross-fetch

* refactor: remove verify-apple-id-token, jsonwebtoken, jwks-rsa deps

* build: remove unused deps

* refactor: replace co-body and body-parser deps in lib code

* fix: fix migration to fetch

* fix: fix serialization issue + tests

* test: revert merge error

* refactor: test fixes

* refactor: self-review fixes

* fix: fix minor issues from e2e tests

* fix: fix apple client secret generation

* refactor: fix review comments

---------

Co-authored-by: Rishabh Poddar <rishabh.poddar@gmail.com>
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.

3 participants