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

Introduce withRefreshAuth #8574

Merged
merged 6 commits into from
Feb 24, 2023
Merged

Introduce withRefreshAuth #8574

merged 6 commits into from
Feb 24, 2023

Conversation

djhi
Copy link
Contributor

@djhi djhi commented Jan 13, 2023

No description provided.

@fzaninotto
Copy link
Member

I don't like the idea of a single decorator for two very different objects. I also see that it forces you to do some borderline inspection to determine in which case you are. How about one for each?

@djhi
Copy link
Contributor Author

djhi commented Jan 26, 2023

How about one for each?

How would you name them?
addRefreshAuthToDataProvider and addRefreshAuthToAuthProvider?

@fzaninotto
Copy link
Member

yes, fine by me

docs/navigation.html Outdated Show resolved Hide resolved
docs/withRefreshAuth.md Outdated Show resolved Hide resolved
docs/addRefreshAuthToDataProvider.md Outdated Show resolved Hide resolved
docs/addRefreshAuthToAuthProvider.md Outdated Show resolved Hide resolved
Co-authored-by: Gildas Garcia <1122076+djhi@users.noreply.github.com>
docs/Authentication.md Outdated Show resolved Hide resolved
Co-authored-by: Jean-Baptiste Kaiser <jb@marmelab.com>
@slax57 slax57 added this to the 4.9.0 milestone Feb 24, 2023
@slax57 slax57 merged commit c69c86f into next Feb 24, 2023
@slax57 slax57 deleted the refresh-auth-hof branch February 24, 2023 14:27
@davidhenley
Copy link
Contributor

davidhenley commented Mar 27, 2023

I'm a little confused on this. Wrapping both the auth and data providers cause a double refresh.

Are you only supposed to wrap one or the other? I feel like the docs need a little work on this subject.

@djhi
Copy link
Contributor Author

djhi commented Mar 28, 2023

Indeed, we could do a better job on the documentation side. One way to avoid the double refresh would be to store the first promise and return it on subsequent calls until it is resolved. You can check how we did it in ra-directus for instance: https://github.com/marmelab/ra-directus/blob/main/packages/ra-directus/src/directusRefreshAuthToken.ts

Maybe we should handle this ourselves so you don't have to worry about it though

@davidhenley
Copy link
Contributor

davidhenley commented Mar 28, 2023

Hey @djhi I cannot see that link. I think that's a private repo. Do you mind sending the example to my email on file with React-Admin Enterprise if you cannot post it here?

@fzaninotto
Copy link
Member

fzaninotto commented Mar 28, 2023

I've just changed the repo visibility, you should be able to see the code.

@davidhenley
Copy link
Contributor

Thanks @fzaninotto! I see it now

@davidhenley
Copy link
Contributor

davidhenley commented Mar 28, 2023

@djhi I'm still getting double refreshes with this method.

const refreshToken = () => {
  let refreshPromise = null;

  if (refreshPromise != null) {
    return refreshPromise;
  }

  const accessToken = localStorage.getItem('authentication.access_token');
  const refreshToken = localStorage.getItem('authentication.refresh_token');

  if (!accessToken && !refreshToken) {
    return Promise.reject();
  }

  const now = new Date()
  const expired = jwtDecode(accessToken).exp < now.getTime() / 1000

  if (expired) {
    refreshPromise = (async () => {
      const request = new Request(`${process.env.REACT_APP_API_URL}/auth/refresh`, {
        method: 'POST',
        body: JSON.stringify({accessToken, refreshToken}),
        headers: new Headers({'Content-Type': 'application/json'}),
      });
      const response = await fetch(request);
      if (response.status < 200 || response.status >= 300) {
        throw new HttpError(response.statusText, response.status);
      }
      const data = await response.json();
      localStorage.setItem('authentication.access_token', data.token);
      localStorage.setItem('authentication.refresh_token', data.refreshToken);
    })().finally(() => {
      refreshPromise = null;
    });
    return refreshPromise;
  }
}

CleanShot 2023-03-28 at 10 29 31

CleanShot 2023-03-28 at 10 30 03

@djhi
Copy link
Contributor Author

djhi commented Apr 5, 2023

let refreshPromise = null;

const refreshToken = () => {

  if (refreshPromise != null) {
    return refreshPromise;
  }

  const accessToken = localStorage.getItem('authentication.access_token');
  const refreshToken = localStorage.getItem('authentication.refresh_token');

  if (!accessToken && !refreshToken) {
    return Promise.reject();
  }

  const now = new Date()
  const expired = jwtDecode(accessToken).exp < now.getTime() / 1000

  if (expired) {
    refreshPromise = (async () => {
      const request = new Request(`${process.env.REACT_APP_API_URL}/auth/refresh`, {
        method: 'POST',
        body: JSON.stringify({accessToken, refreshToken}),
        headers: new Headers({'Content-Type': 'application/json'}),
      });
      const response = await fetch(request);
      if (response.status < 200 || response.status >= 300) {
        throw new HttpError(response.statusText, response.status);
      }
      const data = await response.json();
      localStorage.setItem('authentication.access_token', data.token);
      localStorage.setItem('authentication.refresh_token', data.refreshToken);
    })().finally(() => {
      refreshPromise = null;
    });
    return refreshPromise;
  }
}

This will work. However, we're going to provide a way to do that automatically in a later minor version.

@davidhenley
Copy link
Contributor

@djhi That was it! Thank you

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

Successfully merging this pull request may close these issues.

None yet

4 participants