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

v4: resources not reloading after login or auth change #7527

Closed
RWOverdijk opened this issue Apr 12, 2022 · 9 comments · Fixed by #7539
Closed

v4: resources not reloading after login or auth change #7527

RWOverdijk opened this issue Apr 12, 2022 · 9 comments · Fixed by #7539

Comments

@RWOverdijk
Copy link
Contributor

RWOverdijk commented Apr 12, 2022

What you were expecting:

When logging in I expect the resources to reload. The reason for this is that the auth state change comes with a permissions change, which decides which resources the user can view.

What happened instead:

The ready screen:

image

Steps to reproduce:

I followed this: https://marmelab.com/react-admin/doc/4.0/Admin.html#declaring-resources-at-runtime

It's really pretty simple to reproduce. Create a custom AdminContext and based on the auth state either render null or the resources (pretending you don't have permissions to see the other pages).

I made a sandbox based on the example in the repo (bnase sandbox: https://codesandbox.io/s/github/marmelab/react-admin/tree/next/examples/simple) but it's pretty difficult to get it to run on codesandbox (a dependency issue somewhere?). What I did:

  1. Download the sandbox
  2. delete vite.config.js
  3. npm i --force (force is needed depending on your node version, there's a conflict somewhere in the tree)
  4. npm run start

Then, when in the browser follow these steps:

  1. You have to log out
  2. You have to hard-refresh the browser before logging in
  3. Log in
  4. You see the screen from my screenshot up 👆

This is an example. I tried other things like forcing a re-render. It does rerender, it just doesn't register the resources.

Video of the issue demonstrated:

stuk.mov

Related code:

https://codesandbox.io/s/strange-greider-8eieup

Specifically in index.js:

    if (authLoading || !authenticated) return null;

    return (
      <>
        <Resource name="posts" {...posts} />
        <Resource name="comments" {...comments} />
        <Resource name="users" {...users} />
        <Resource name="tags" {...tags} />
      </>
    );

This is the most basic example of what I'm actually trying to do which is load in permissions async and show resources based on them. Users are allowed to change their role while logged in (different profiles, like for example netflix). But something as basic as logging in already doesn't work and properly illustrates my issue I think.

Environment

  • React-admin version: react-admin@4.0.0-rc.1
  • React version: 17.0.2
  • Browser: Chrome
  • Stack trace (in case of a JS error): N/A
@RWOverdijk RWOverdijk changed the title v4: resource not reloading after login or auth change v4: resources not reloading after login or auth change Apr 12, 2022
@slax57
Copy link
Contributor

slax57 commented Apr 12, 2022

Hi!

Once again, thank you for submitting this.

I am not sure this is really an issue with react admin, but rather with the actual implementation you used to test this feature.
According to my tests, the Admin is not refreshing once you are logged in because the authenticated state in your component is not refreshed and is kept to false, hence leading to seeing the ready screen.
This is probably due to the underlying implementation of the useAuthState hook, that uses a custom hook called useSafeSetState that prevents applying effects when the component is unmounted.
I am not 100% sure but I believe this is what happens in your example.

Anyway, I think that with a "real world" implementation, where your resources are not conditioned to the auth state but rather to a permission system (that you query after your are logged in), you wouldn't have this problem.

But since it's a quite advanced mechanism, I would be happy to have @fzaninotto , @djhi or @WiXSL give their advice on this 🙂

@RWOverdijk
Copy link
Contributor Author

@slax57 Hi! 😄

I just checked usePermissions and it also uses useSafeSetState. Whatever that is, based on what I know so far I'd say that it's doing something wrong. usePermissions and useAuthState offer information. When that information changes they should let me know. If my navitgation is based on those permissions and the auth state, how else should it be implemented?

that you query after your are logged in

I do this in the authProvider. It has a dedicated method for it that allows me to use existing systems (usePermissions). I'm also not sure how I'd write a permission system that checks permissions on login if I'm never being notified of changes in the auth state. Putting that logic in the login screen would be weird. Putting logic that triggers a rerender in a provider also doesn't strike me as usual. Anyway for reference here's a slightly larger code snippet (I can't share it all because contracts):

function GuardedResources() {
  const { isLoading, guard } = usePermissionGuard();

  const composedResources = useMemo(() => {
    if (isLoading || !guard.hasPermissions()) return null;

    return resources.map(({ permissionResource: r, components, ...props }) => {
      if (!guard.isAllowed(r)) return null;

      return <Resource key={`r_${r}`} {...guard.filterComponents(r, components)} {...props} />;
    });
  }, [isLoading, guard]);

  return (
    <AdminUI layout={MyLayout} loginPage={SignIn} disableTelemetry={true} requireAuth>
      {composedResources}
    </AdminUI>
  );
}

function App() {
  return (
    <AdminContext
      authProvider={authProvider}
      dataProvider={withUpload}
      i18nProvider={defaultI18nProvider}
      store={store}
    >
      <GuardedResources />
    </AdminContext>
  );
}

Where usePermissionGuard is:

import { useEffect, useMemo, useState } from 'react';
import { usePermissions } from 'react-admin';
import { PermissionGuard } from './PermissionGuard';

export function usePermissionGuard() {
  const guard = useMemo(() => new PermissionGuard(), []);
  const { isLoading, permissions } = usePermissions();
  const [permissionGuard, setPermissionGuard] = useState({ isLoading, guard });

  useEffect(() => {
    guard.setPermissions(isLoading ? null : permissions?.permissions);

    setPermissionGuard({ guard, isLoading });
  }, [guard, isLoading, permissions]);

  return permissionGuard;
}

Some notes to help reading this:

  • .filterComponents() returns the resource components (list, create, etc) based on the permissions.
  • resources is an array of resource objects (icon, name, components, permission resource name). Here's what one of those looks like:
export default {
  permissionResource: 'Role',
  name: 'roles',
  icon: BadgeIcon,
  components: new GuardedComponents([
    ['list', { action: 'GetList', component: RoleList }],
    ['show', { action: 'GetOne', component: RoleShow }],
    ['edit', { action: 'Update', component: RoleEdit }],
    ['create', { action: 'Create', component: RoleCreate }],
  ]),
} as ResourceDefinition;

@slax57
Copy link
Contributor

slax57 commented Apr 13, 2022

Okay.

So I took some time to dive back into this issue.

Here's what I've been able to find:

  • Actually we can reproduce this issue (or at least what I think is its root cause) straight with the simple codesandbox example
    • Indeed the simple demo uses permissions to display the "users" Resource or not
    • The "admin" profile see it, while the "login" profile does not
  • With this sandbox
    • When I add a new Resource to the list, for instance by logging in with "login" and then with "admin", it works fine
    • When I remove a Resource from the list, for instance by logging in with "admin" and then with "login", then the "users" resource stays in the menu

So this other example showcases that there is a refresh problem with the list of resources.

I've been able to narrow this down to the useConfigureAdminRouterFromChildren and useRegisterResource hooks, which together can only register new resources but never unregister them.
More globally, I think there is a desynchronization between the resources state from useConfigureAdminRouterFromChildren and the resources stored in the ResourceDefinitionContext.

So yeah, there probably is a bug somewhere around there, or at least something we could improve.
But I believe this quite minor, since it's a pretty advance scenario to be able to change your permissions on the fly, added to the fact that a simple page reload fixes it.

I'll have a chat with the team to gather their opinion on this.

Anyway, thanks for your submission 🙂

@fzaninotto
Copy link
Member

The bug is still there, let me reopen this issue.

@fzaninotto fzaninotto reopened this Apr 14, 2022
@slax57
Copy link
Contributor

slax57 commented Apr 14, 2022

Hi!

So, I took some time to dive in this issue, again 😅.

Here are my conclusions:

  • According to me, the problem comes from a misuse of RA rather than a proper bug. Let me explain.
  • Normally, you are supposed to provide the <Admin> (or any of its internal children such as <AdminContext> or <AdminUI>) either of the following:
    • one or more <Resource>
    • a function taking permissions as its argument and returning a list of <Resource> (as an array or as a React element)
    • custom routes and stuff, but let's not bother with them for now 😅
  • This means you are normally not supposed to pass in a FC here, which you would be tempted to do in order to use hooks (like useAuthState in your example)
  • I agree that this can be restrictive, but also note that this works in most cases. Indeed:
    • Your app runs flawlessly if you have a "default" resource that is always declared (even when you are not logged in)
    • Your app also runs flawlessly if you provide a function (register) => { return []; } as the default (I'm not 100% sure on this one, I did the test a few yours ago and I already can't remember for sure it worked 😅, but anyway... )

According to me, the reason why it doesn't work in your test is because:

  • Your component GuardedResources actually wraps the AdminUI, which means is will not be unmounted at logout
  • You are using the useAuthState hook, which actually only gives you the state when you call it, but does not trigger a rerender if the state changes
    • this might not be the wanted behavior, I'm not 100% sure, but I do believe that is how the hook works today at least
  • Because of the 2 above reasons, your component never re-renders, leading to the Admin staying at the 'empty' state
  • I actually managed to get it working by calling the useCheckAuth hook manually each time, see the code below.
function GuardedResources() {

    useEffect(() => {
        console.log('GuardedResources - mounted');
        return () => console.log('GuardedResources - unmounted!');
    }, []);

    const [state, setState] = useSafeSetState({
        isLoading: true,
        authenticated: true, // optimistic
    });
    const checkAuth = useCheckAuth();
    checkAuth({}, false)
        .then(() =>
            setState(prev => {
                if (!prev.authenticated || prev.isLoading)
                    return { isLoading: false, authenticated: true };
                else return prev;
            })
        )
        .catch(() =>
            setState(prev => {
                if (prev.authenticated || prev.isLoading)
                    return { isLoading: false, authenticated: false };
                else return prev;
            })
        );

    const { isLoading: authLoading, authenticated } = state;
    console.log(`checkAuth result: { ${authLoading}, ${authenticated} }`);

    if (authLoading || !authenticated) {
        console.log('rendering empty');
        return <AdminUI></AdminUI>;
    }

    console.log('rendering resources');
    return (
        <AdminUI>
            <Resource name="posts" {...posts} />
            <Resource name="comments" {...comments} />
            <Resource name="users" {...users} />
            <Resource name="tags" {...tags} />
        </AdminUI>
    );
}

In conclusion, I believe there are several way to fix this (I mentioned some above), but I do not thing this deserves a fix at RA's side. Unless of course it turns out the useAuthState hook actually is supposed to trigger a re-render at logout, in which case we would need to fix it.

Feel free to let me know what you think about this, especially if you disagree with my analysis 😅
Cheers

@RWOverdijk
Copy link
Contributor Author

Hooks is literally named as a reason to use the context in the docs I linked.

Anyway it's fine, I just forked and fixed it for myself. It's why I closed this issue.

@slax57
Copy link
Contributor

slax57 commented Apr 15, 2022

@RWOverdijk you are, once again, totally right about the docs, I totally missed the part where you are encouraged to unplug the AdminContext and the AdminUI, sorry about that.

Still, what I said about the useAuthState hook remains true, it currently only works with components being unmounted at logout, and this is because there is no way to know otherwise that the auth state has changed (you need to call the authProvider to know).

Anyway, I'm glad you found a way to do it. If you believe it's worth sharing, feel free to add more details about your implementation here (or to do a PR) 🙂

Closing issue for now.

Thanks again, and cheers

@howardjm
Copy link

howardjm commented Sep 8, 2023

It should be basic functionality to be able to secure your App and not obscenely complicated

@slax57
Copy link
Contributor

slax57 commented Sep 12, 2023

@howardjm It is, and we already provide several built-in solutions for that:

This issue is about a rather specific use-case if my recollection is good.

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

Successfully merging a pull request may close this issue.

4 participants