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

TypeError with CacheLocation #95

Open
jodonnell opened this issue May 31, 2019 · 34 comments
Open

TypeError with CacheLocation #95

jodonnell opened this issue May 31, 2019 · 34 comments
Labels

Comments

@jodonnell
Copy link

Describe the bug
I followed the documentation,
const config = {
auth: {
authority: (process.env.REACT_APP_AUTHORITY as string),
clientId: (process.env.REACT_APP_AAD_APP_CLIENT_ID as string),
},
cache: {
cacheLocation: "localStorage",
storeAuthStateInCookie: true
},
};

and got a TypeError

Argument of type '{ auth: { authority: string; clientId: string; }; cache: { cacheLocation: string; storeAuthStateInCookie: boolean; }; }' is not assignable to parameter of type 'Configuration'.
Types of property 'cache' are incompatible.
Type '{ cacheLocation: string; storeAuthStateInCookie: boolean; }' is not assignable to type 'CacheOptions'.
Types of property 'cacheLocation' are incompatible.
Type 'string' is not assignable to type '"sessionStorage" | "localStorage" | undefined'. [2345]

I fixed it by import CacheLocation and casting my string
cacheLocation: ("localStorage" as CacheLocation),

@AndrewCraswell
Copy link
Contributor

Could you share your tsconfig.json and your TypeScript version? I'm not seeing the same TypeError be thrown so I'd like to replicate the environment as close as possible to test again.

@jodonnell
Copy link
Author

version 3.4.5

{ "compilerOptions": { "target": "es5", "lib": [ "dom", "dom.iterable", "esnext" ], "allowJs": true, "skipLibCheck": true, "esModuleInterop": true, "allowSyntheticDefaultImports": true, "strict": true, "forceConsistentCasingInFileNames": true, "module": "esnext", "moduleResolution": "node", "resolveJsonModule": true, "isolatedModules": true, "noEmit": true, "noUnusedLocals": true, "jsx": "preserve", "baseUrl": "src" }, "include": [ "src" ] }

@skyshine999
Copy link

I am also facing same issue, can you help me how can i resolve cacheLocaation error

const config = {
auth: {
authority: 'https://login.microsoftonline.com/',
clientId: '3324324324g-4233-54545-2345a-3453453d’,
redirectUri: ’sampleApp://sample.apps.ap'
},
cache: {
cacheLocation: "localStorage",
storeAuthStateInCookie: true
}
};

const authenticationParameters = {
scopes: [
'https://graph.microsoft.com/user.read'
]
}

export default class App extends Component {

loginCallback(){

}
logoutCallback(){

}
printAccountInfo(){

}

render() {
return (
<AzureAD
provider={
new MsalAuthProviderFactory(config, authenticationParameters, LoginType.Popup)
}
unauthenticatedFunction={this.loginCallback}
authenticatedFunction={this.logoutCallback}
accountInfoCallback={this.printAccountInfo}
/>

);

}
}

@jodonnell
Copy link
Author

import { CacheLocation } from "react-aad-msal";
cacheLocation: ("localStorage" as CacheLocation),

this works. i think this is because type widening, the cacheLocation key "localStorage" is widenened to type string so you need to force the cast to be of CacheLocation. I'm no expert though!

@AndrewCraswell
Copy link
Contributor

After a bit of investigation, it seems all the types involved are defined on msal, so I believe this issue originates from there. I myself do not get any of these errors, so it's a little strange to me. Can anyone repro this issue by just running the sample?

I think at the least it's fair to update the documentation since there's clearly an issue being experienced.

This should be a good first issue to fix unless someone can submit a fix for it I will see if I can find some time in the next few weeks. I'll be out of town frequently though so it will be delayed.

@AndrewCraswell
Copy link
Contributor

I included the changes to the documentation described in this issue with my latest PR (#107) since I was editing the docs anyway. This isn't a resolution, but hopefully, it will help others who are implementing this component for the first time.

@jodonnell
Copy link
Author

jodonnell commented Jul 19, 2019

Hi @AndrewCraswell , this definitely seems like type widening to me, as this also works for me:

  const cacheLocation: 'localStorage' = 'localStorage';
  const config = {
    auth: {
      authority: (process.env.REACT_APP_AUTHORITY as string),
      clientId: (process.env.REACT_APP_AAD_APP_CLIENT_ID as string),
    },
    cache: {
      cacheLocation,
      storeAuthStateInCookie: true,
    },
  };

https://mariusschulz.com/blog/literal-type-widening-in-typescript

this also works for me

  const config = {
    auth: {
      authority: (process.env.REACT_APP_AUTHORITY as string),
      clientId: (process.env.REACT_APP_AAD_APP_CLIENT_ID as string),
    },
    cache: {
      cacheLocation: 'localStorage',
      storeAuthStateInCookie: true,
    } as const,
  };

as described here
microsoft/TypeScript#29510

@jodonnell
Copy link
Author

the documentation looks good to me, but i suppose you could take your pick from these alternative syntaxes as well. personally i don't think i see any clarity benefits of one over another.

@jodonnell
Copy link
Author

jodonnell commented Jul 19, 2019

enum CacheStorage {
  LocalStorage = "localStorage",
  SessionStorage = "sessionStorage"
}
........
  const config = {
    auth: {
      authority: (process.env.REACT_APP_AUTHORITY as string),
      clientId: (process.env.REACT_APP_AAD_APP_CLIENT_ID as string),
    },
    cache: {
      cacheLocation: CacheStorage.LocalStorage,
      storeAuthStateInCookie: true,
    },
  };

that also works for me, so i suppose you could export this enum for usage, this definitely has a clarity benefit, but then you need to redefine this on top of msal's definition.

i think ideally the msal author might add this.

@AndrewCraswell
Copy link
Contributor

AndrewCraswell commented Jul 19, 2019

Yeah, I'm just hesitant to redefine the CacheStorage since that type comes directly from MSAL. Having a second definition could trip people up, since depending on the one they use the functionality will be different. The best case would be for MSAL library to convert their type to an enum as it should have been from the beginning.

@jodonnell
Copy link
Author

seems like at some point this will make it through to msal
AzureAD/microsoft-authentication-library-for-js#851

@sameerag
Copy link

Merged to dev today, we are releasing a patch sometime soon, the release should have this fix. We appreciate the contribution @jodonnell, Thankyou.

@jodonnell
Copy link
Author

jodonnell commented Jul 23, 2019 via email

@AndrewCraswell
Copy link
Contributor

Thanks for pushing this through, @jodonnell. I've updated my PR #107 which was already updating the msal package, but I've set it to get the latest release with your fix. I've also removed the update to the documentation I had done.

@jodonnell
Copy link
Author

Ugh, sorry @AndrewCraswell, but the change got reverted. AzureAD/microsoft-authentication-library-for-js#851

@sameerag
Copy link

Yeah. we have issues with TS projects with this change. If you have a different approach, feel free to push a new PR or one of us will get to this soon.

@scottyh527
Copy link

I'm just using the msal library in react and I am getting the same error for session storage.

How can I define the storage as a type or an enum in javascript, or do I just store it as an object?

const msalConfig = {
auth: {
clientId: process.env.REACT_APP_MS_ID,
redirectUri: process.env.REACT_APP_REDIRECT_URI,
navigateToLoginRequestUrl:false
},
cache: {
cacheLocation: "sessionStorage",
storeAuthStateInCookie: true
}
};

@AndrewCraswell
Copy link
Contributor

@scottyh527 You'd probably be better off raising with the MSAL library, as they're determining how to prioritize this fix. But to get you unblocked, have you tried @jodonnell's solution referenced above?

enum CacheStorage {
  LocalStorage = "localStorage",
  SessionStorage = "sessionStorage"
}
// ...
  const config = {
    auth: {
      authority: (process.env.REACT_APP_AUTHORITY as string),
      clientId: (process.env.REACT_APP_AAD_APP_CLIENT_ID as string),
    },
    cache: {
      cacheLocation: CacheStorage.LocalStorage,
      storeAuthStateInCookie: true,
    },
  };

@scottyh527
Copy link

@scottyh527 You'd probably be better off raising with the MSAL library, as they're determining how to prioritize this fix. But to get you unblocked, have you tried @jodonnell's solution referenced above?

enum CacheStorage {
  LocalStorage = "localStorage",
  SessionStorage = "sessionStorage"
}
// ...
  const config = {
    auth: {
      authority: (process.env.REACT_APP_AUTHORITY as string),
      clientId: (process.env.REACT_APP_AAD_APP_CLIENT_ID as string),
    },
    cache: {
      cacheLocation: CacheStorage.LocalStorage,
      storeAuthStateInCookie: true,
    },
  };

I tried his fix for adding the enum. I put this before the msalconfig object i created.

enum CacheStorage {
LocalStorage = "localStorage",
SessionStorage = "sessionStorage"
}

But it seems javascript doesn't allow for the enum.

@jodonnell
Copy link
Author

@scottyh527 this error is specific to typescript, if you aren't using typescript then you aren't getting the same error. what are you seeing exactly?

@jodonnell
Copy link
Author

assuming your are using typescript though you can try
cacheLocation: ("localStorage" as "localStorage"),

this forces the type of the string to be the string "localStorage" and not ANY string.

@AndrewCraswell
Copy link
Contributor

@scottyh527 Oh sorry, didn't catch that you aren't using TypeScript. There should be several ways to do this, my preferred is:

let CacheLocation = Object.freeze({
  SessionStorage: 'sessionStorage',
  LocalSotarge: 'localStorage'
});

Now you can use it as something closer to an Enum.

@scottyh527
Copy link

I'm using react, not typescript.

The error is ClientConfigurationError: The cache location provided is not valid. Provided value: sessionStorage. Possible values are: localStorage, sessionStorage.

@scottyh527
Copy link

let CacheLocation = Object.freeze({
  SessionStorage: 'sessionStorage',
  LocalSotarge: 'localStorage'
});

I'll give it a try thanks. Its hard for me to recreate it since a colleague caught it and I hadn't seen it until now.

@jodonnell
Copy link
Author

@AndrewCraswell i suspect what scotty and #104 are talking about is a different issue

seems to stem from the msal lib which actually defines a javascript error

https://github.com/AzureAD/microsoft-authentication-library-for-js/search?q=ClientConfigurationError&unscoped_q=ClientConfigurationError

@jodonnell
Copy link
Author

jodonnell commented Aug 26, 2019

i dug into their code a bit

// cache keys msal - typescript throws an error if any value other than "localStorage" or "sessionStorage" is passed try { this.cacheStorage = new Storage(this.config.cache.cacheLocation); } catch (e) { throw ClientConfigurationError.createInvalidCacheLocationConfigError(this.config.cache.cacheLocation); }
so that error is throw on ANY error in new Storage. my guess is something else is going in there since the cacheLocation seems to be right

reading the code i see

this.localStorageSupported = typeof window[this.cacheLocation] !== "undefined" && window[this.cacheLocation] != null; this.sessionStorageSupported = typeof window[cacheLocation] !== "undefined" && window[cacheLocation] != null; Storage.instance = this; if (!this.localStorageSupported && !this.sessionStorageSupported) { throw ClientConfigurationError.createNoStorageSupportedError(); }
so i think your browser doesn't support localstorage or session storage, that error is getting thrown but then then above code catches it and throws a different not true error.

we need a bug for https://github.com/AzureAD/microsoft-authentication-library-for-js repo

@scottyh527
Copy link

@jodonnell Any way I can get around that? My colleague was just using chrome not sure how it didn't support local or session storage.

@mledwards
Copy link

Having the same React / Not TypeScript issue out of the box when defining cacheLocation: "localStorage"

@andrewfabrizi
Copy link

@mledwards I'm having the same problem (with JS not TS) within an iframe. I've tried localStorage and sessionStorage with a Popup. Any ideas?

@mledwards
Copy link

mledwards commented Oct 30, 2019

@andrewfabrizi My issue was that I'm using next.js which has server side rendering, so when Azure brings me back to the callback the server side is firing which doesn't have access to the browser's local storage.

I haven't done it yet, but my work around will be to make the callback page simply trigger next link click to a client side page.

Not sure if this helps your problem, but may help others.

@andrewfabrizi
Copy link

@mledwards it is strictly a static client-side website and I'm only having an issue when the page is rendered within an iframe.

@gauravsbagul
Copy link

Hey, I am using this in React Native and getting same error please suggest some solution for React Native
`import { UserAgentApplication } from 'msal';
import AsyncStorage from '@react-native-community/async-storage';

export const msalApp = new UserAgentApplication({
auth: {
clientId: 'xxxxx',
authority: 'https://login.microsoftonline.com/common',
validateAuthority: true,
postLogoutRedirectUri: 'http://localhost:3000',
navigateToLoginRequestUrl: false,
},
cache: {
cacheLocation: AsyncStorage,
},

});
`

@stuarthallows
Copy link

stuarthallows commented Sep 10, 2020

Another way to resolve this is to add the config type as Configuration.

import { Configuration } from 'msal';

const signInConfig: Configuration = {
  auth: {
    authority: signInAuthority,
    clientId: applicationID,
    redirectUri: reactRedirectUri,
    validateAuthority: false
  },
  cache: {
    cacheLocation: "localStorage",
    storeAuthStateInCookie: true
  }
};

@mledwards
Copy link

If anyone has had the same issue as I did above while doing this via next.js SSR, I moved all of my code into a useEffect hook, which only runs client side, and that solved my problem 👍

Server side rendering doesn't have access to the browser / local caches.

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

No branches or pull requests

9 participants