Skip to content

Commit

Permalink
Manually apply microsoft-healthcare-madison@d95c3b5
Browse files Browse the repository at this point in the history
  • Loading branch information
@vlad-ignatov
vlad-ignatov committed Jul 21, 2022
1 parent f3a3c85 commit 3ad1d83
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 8 deletions.
7 changes: 3 additions & 4 deletions src/smart.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,15 +324,14 @@ export async function authorize(
redirectParams.push("launch=" + encodeURIComponent(launch));
}

if ((pkceMode === 'required') && (!(extensions.codeChallengeMethods.includes('S256')))) {
if (pkceMode === 'required' && !extensions.codeChallengeMethods.includes('S256')) {
throw new Error("Required PKCE code challenge method (`S256`) was not found.");
}

if ((pkceMode !== 'disabled') && (extensions.codeChallengeMethods.includes('S256'))) {
if ((pkceMode === "unsafeV1" || pkceMode !== 'disabled') && extensions.codeChallengeMethods.includes('S256')) {

This comment has been minimized.

Sign in to view

Copy link
@madaster97

madaster97 Jul 29, 2022

Hi @vlad-ignatov , this line isn't treating the unsafeV1 quite right. Can we do something like this for this line?

if ( 
        ((pkceMode !== 'disabled') && (extensions.codeChallengeMethods.includes('S256'))) ||
        (pkceMode == 'unsafeV1')
    )

I wrote up this table on what I think the desired behavior is (Error, Skip PKCE or Include PKCE), which the above + the required/supported check should cover. The latter 4 columns are running the logic above and showing the result (I realize there are duplicate columns):

Setting Found S256 Outcome notDisabled FoundS256 isUnsafe Result
ifSupported No Skip TRUE FALSE FALSE FALSE
disabled No Skip FALSE FALSE FALSE FALSE
unsafeV1 No Include TRUE FALSE TRUE TRUE
Undefined No Skip TRUE FALSE FALSE FALSE
required Yes Include TRUE TRUE FALSE TRUE
ifSupported Yes Include TRUE TRUE FALSE TRUE
disabled Yes Skip FALSE TRUE FALSE FALSE
unsafeV1 Yes Include TRUE TRUE TRUE TRUE
Undefined Yes Include TRUE TRUE FALSE TRUE

This comment has been minimized.

Sign in to view

Copy link
@vlad-ignatov

vlad-ignatov Jul 29, 2022

Author Collaborator

I'm not a big fan of those IFs anyway. Do you think something like this would be correct?

function shouldIncludeChallenge(S256supported: boolean, pkceMode?: string) {
    if (pkceMode === "disabled") {
        return false;
    }
    if (pkceMode === "unsafeV1") {
        return true;
    }
    if (pkceMode === "required") {
        if (!S256supported) {
            throw new Error("Required PKCE code challenge method (`S256`) was not found.");
        }
        return true;
    }
    return S256supported;
}

This comment has been minimized.

Sign in to view

Copy link
@madaster97

madaster97 Jul 29, 2022

Perfect! Agreed that's more readable than before
let codes = await security.generatePKCEChallenge()
Object.assign(state, codes);
await storage.set(stateKey, state); // note that the challenge is ALREADY encoded properly

await storage.set(stateKey, state); // note that the challenge is ALREADY encoded properly
redirectParams.push("code_challenge=" + state.codeChallenge);
redirectParams.push("code_challenge_method=S256");
}
Expand Down
11 changes: 7 additions & 4 deletions src/types.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ declare namespace fhirclient {
function WindowTargetFunction(): Promise<WindowTargetVariable>;
type WindowTarget = WindowTargetVariable | typeof WindowTargetFunction;

type PkceMode = 'ifSupported' | 'required' | 'disabled';
type PkceMode = 'ifSupported' | 'required' | 'disabled' | 'unsafeV1';

type storageFactory = (options?: Record<string, any>) => Storage;

Expand Down Expand Up @@ -541,7 +541,7 @@ declare namespace fhirclient {
* `clientPrivateJwk` here. **Note: ONLY use this on the server**, as
* the browsers are considered incapable of keeping a secret.
*/
clientPrivateJwk?: { kid: string; kty: string, [k: string]: string } & (
clientPrivateJwk?: { kid: string; kty: string, [k: string]: string } & (
{kty: "EC", alg: "ES384", crv: "P-384"} |
{kty: "RSA", alg: "RS384" }
);
Expand Down Expand Up @@ -596,9 +596,12 @@ declare namespace fhirclient {
* - `ifSupported` Use if a matching code challenge method is available (**default**)
* - `required` Do not attempt authorization to servers without support
* - `disabled` Do not use PKCE
* - `unsafeV1` Use against Smart v1 servers. Smart v1 does not define
* conformance, so validate your server supports PKCE before using
* this setting
*/
pkceMode?: PkceMode;
}
pkceMode?: PkceMode;
}

/**
* Additional options that can be passed to `client.request` to control its
Expand Down

0 comments on commit 3ad1d83

Please sign in to comment.