Bug: Token is saved with " to SessionStorage

[brumik]

Steps To Reproduce

I am using the session storage to retrieve the tokens when need to call the API. This returns the token correctly but with extra quotation marks.

Expected token to be returned: this is my super secret token

Returned token: "this is my super secret token"

[brumik]

This is due to the fact that useBrowserStorage stores everything with JSON.stringyfy(value). I am not sure if this is needed, and if so how to go around it?

1. We can add a variable stringify: true to the hook
2. We can check in the hook if it is an object
3. We can just not use a hook for setting a session variable
4. We can construct a new hook?

[sebastianvitterso]

Does the API return "this is my super secret token", with quotations? That seems like a bug in the auth API, and something that should be resolved on the server side.

Sadly, we (the library) can't be tasked with handling every non-compliant response and every irregularity, as that would cause bloat.

[sebastianvitterso]

Sorry, I might have misunderstood slightly. Are you retrieving the token value directly from SessionStorage? If so, then yes it's JSON-stringified there, but that's an implementation detail of the library which you aren't required to think about.

If you need the token value, then that's available through:

import { AuthContext } from 'react-oauth2-code-pkce'
{ token } = useContext(AuthContext)

For a larger code snippet, check out the example in the README 🥳

Let me know if this does not resolve the problem for you!

[brumik]

Hello, yes, that is correct. I see the value to get the token on fly outside of react (I use it constricting headers dynamically with the token). Is there any other recommendation to use the access token in an API call outside of react?

I think it is fairly often that you use the access token in a library or setup that is outside of react (it can be anything from custom fech, react sagas or pre setup rtk query).

[sebastianvitterso]

I don't think I agree that access tokens fetched via a react library are often used outside of react, and even then, there are ways to use the token value from the library, without retrieving via BrowserStorage manually.

We have a usecase in one of our codebases, actually! I'm working on an internal tool for our client, and it's an electron application. Now, since the frontend is react-based, we use this library for our auth. However, since we also want to fetch data in the "backend"/main-thread of the electron application (if you're not familiar, the short version is that you can't access react from there), we do a trick that I think could work for you as well.

We have a global variable in the backend context called AUTH, that will hold the token inside it. Then, every time the library updates the token (inside react), we have a useEffect that calls a function to update that global variable.

In your case, I think it could be this simple:

// auth.js (outside React-land)
export const AUTH = {
  token: ""
}

// App.jsx (React-land)
import { useContext, useEffect } from 'react'
import { AuthContext } from 'react-oauth2-code-pkce'
import { AUTH } from '../auth'

export const App = () => {
  useEffect(() => {
    AUTH.token = token
  }, [token])

  return <>...</>
}

I'm not sure how you're packaging this, but this AUTH object could also just be stuck right onto the global window object, which would give you similar results.

[brumik]

Sure, this is an option, however I would prefer to read it from session instead of managing it from react and through global memory variables.

My use case is something like this:

const MyAPIClient = new API({
  baseUrl: baseUrl,
  middlewares: [
    includeAuth(
      () => sessionStorage.getItem('ROCP_token')?.slice(1, -1) ?? '',
      'X-Auth-Token'
    )
  ],
});

export const includeAuth = (
  getToken: GetTokenFnc,
  headerName = 'Bearer',
): FetchMiddleware =>
  (next: FetchFunction) => async (url, options) => {
    // Modify the headers to include the token for auth
    const newOptions: RequestInit = {
      ...options,
      headers: {
        ...options?.headers,
        [headerName]: getToken()
      }
    };
    return next(url, newOptions);
}

which is pretty standard for wrappers around fetch in my experience.

May I ask, what is the problem supporting either a read from storage (with a supported function) or just saving the string to the storage correctly without trying to serializing it? I do not completely understand the resistance. It may not be a "bug" as per say, but could be a handy feature still.

[sebastianvitterso]

The reasoning behind my/our stance on this is that we can't attempt to support every user's specialized usecase without bloating the package with logic that doesn't really relate to its responsibilities. This is one of those specialized usecases.

---

Now, more specifically, the reason why the useBrowserStorage hook works as it does, is that unless we are consistent, it can be hard to know what type to decode from storage. Let's say change it so that strings aren't wrapped with "'s:

If we send in const value: string = "my token", and it gets encoded to my token in storage (no quotes), then I guess we'll just have to read it directly from storage, and we'll get a string back.

But what will be the difference (inside the storage) between a const value: string = "42"and a const value: number = 42? Both will be encoded as 42 (no quotes), and when we retrieve that from storage, we'll have to decide if we want to treat it as a number or a string.

And our typescript types can't really help us, since they're transpiled away before the code is processed.

We could add another argument to the function containing the type in a string, something like:

function useBrowserStorage<T>(
  key: string, 
  initialValue: T, 
  valueType: 'string' | 'number' | 'object',
  type: 'session' | 'local'
)

But in addition to adding the aforementioned bloating complexity, this would also sort of "duplicate" the typing we already have in typescript.

So it would be extra "work" for us, as well as extra code for every user of the library, instead of adding this code to the project that needs it.

---

Now, in closing, I think adding a const authTokenHolder = { token: '' } above your const MyAPIClient, and updating from inside the root React component, is easily just as quick. And honestly, it's more idiomatic for React code, in my opinion.

_{Now I'm sure you can find a way around the typescript issues I described with clever engineering, and maybe the solution can be quite simple and sleek, but the initial point still stands. The package already provides necessary APIs with simple (enough) access to the token, given the packages constrained scope (it is a React-package after all). So in short, the initial point is the argument.}

[brumik]

Fair point(s).

We use usually session storage for storing the token so I will just have use JSON.parse on it for now.

Would you maybe willing to add "getFromStorage(name: 'token' | 'refreshToken' )" helper function to the lib or even that is out of scope for you?

I will close the PR that I have open, since that will definitely not happen.

[soofstad]

I agree on the conclusion to this. But also want to add that if you read the token directly from storage, the value will not update when the token refreshes. That's another reason I don't want a "getFromStorage" function, it's easy to use it wrong.

[brumik]

As you can see I am reading when making the request, so that's why I suggested the helper function. Outside of react you do not expect much auto update magic to happen usually (especially with storages)

[sebastianvitterso]

Yeah, I think the getFromStorage function, which I assume you want to be a globally available, could potentially get messy considering we have options to change the storage prefix. Since the library is built around using an AuthContext, and because of this storage prefix, we techincally allow users to have two completely separate applications in a single app. The getFromStorage function would then not be obvious to me.

But again, reasoning above is just a specific side of the more general reason, which is that I believe your usecase can just as neatly be solved by either using the given react-solution, or just doing something like JSON.parse(window.LocalStorage.get("ROCP_token")).

Now, I will concede that you are right that the token doesn't need to be wrapped in quotes by the library. The reason is simply that this is an implementation detail that we don't intend for users to use, but if you want to use it, you are more than welcome to.

_{Just be aware that this is not part of the exposed API, so we don't provide any guarantees that the behaviour will remain unchanged within major versions. Now I don't see any reason why we would change this, but we don't guarantee that we won't.}

[brumik]

I was not aware that you support multiple auth providers with different prefixes inside the same app. That would make global functions indeed messy.

I think we have our conclusion. Thank you for your time.

[omikader]

Hello, thanks for the awesome library! I read through this discussion as I was also curious how one could imperatively load the token for a non-hook use case, say for a React Router loader function which is not able to invoke useContext(AuthContext).

Is @sebastianvitterso's snippet the recommended way to do this?

[brumik]

I went in the end with JSON.parse(sessionStorage.getItem('ROCP_token')) and defined the prefix explicitly in the config for a bit more future-proofing and readability.