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

[FEAT] Adding an optional params properties to the Authenticated component so that the useIsAuthenticated and AuthProvider.Check can use the custom params #6309

Open
reedwane opened this issue Sep 9, 2024 · 6 comments · May be fixed by #6483
Assignees
Labels
enhancement New feature or request up for grabs

Comments

@reedwane
Copy link

reedwane commented Sep 9, 2024

Is your feature request related to a problem? Please describe.

I am working on a multitenancy app, and my authentication logic requires calling the Auth.check function with some custom properties like the dynamic domain name, so that I can do some required validation

Describe alternatives you've considered

The work around I have for now is to create a local clone of the Authenticated component and made it accept an optional param in its properties, then passed that to the useIsAuthenticated hook in the component.

I think it's simple enough to consider allowing an optional param object be passed, since the Authprovider.check itself readily allows for it

Additional context

Sample usage of my workaround:

 <DomainAuthenticated
      params={params}
      key={"client-onboaring-layout"}
      loading={<LoadingPage />}
    >
      <>{children}</>
    </DomainAuthenticated>


const DomainAuthenticated = ({
  params,
  children,
  loading,
  redirectOnFail,
  fallback,
  appendCurrentPathToQuery,
}: {
  children: ReactNode;
  fallback?: ReactNode;
  loading?: ReactNode;
  redirectOnFail?: string;
  appendCurrentPathToQuery?: boolean;
  params: { domain?: string };
}) => {
  const parsed = useParsed();
  const go = useGo();
  const {
    isFetching,
    data: {
      authenticated: isAuthenticatedStatus,
      redirectTo: authenticatedRedirect,
    } = {},
  } = useIsAuthenticated({ params });

  // Authentication status
  const isAuthenticated = isAuthenticatedStatus;

  const appliedRedirect =
    redirectOnFail || (authenticatedRedirect as string | undefined);

  // when checking authentication status
  if (isFetching) {
    return <>{loading ?? null}</>;
  }

  // when user is authenticated return children
  if (isAuthenticated) {
    return <>{children ?? null}</>;
  }

  // render fallback if it is exist
  if (typeof fallback !== "undefined") {
    return <>{fallback ?? null}</>;
  }

  // Current pathname to append to the redirect url.
  // User will be redirected to this url after successful mutation. (like login)
  const pathname = `${parsed.pathname}`.replace(/(\?.*|#.*)$/, "");

  return (
    <Redirect
      config={{
        to: appliedRedirect,
        query: appendCurrentPathToQuery
          ? {
              to: parsed.params?.to
                ? parsed.params.to
                : go({
                    to: pathname,
                    options: { keepQuery: true },
                    type: "path",
                  }),
            }
          : undefined,
        type: "replace",
      }}
    />
  );
};

Describe the thing to improve

Allow an optional param property on the Authenticated component

@reedwane reedwane added the enhancement New feature or request label Sep 9, 2024
@aliemir
Copy link
Member

aliemir commented Sep 11, 2024

Hey @reedwane, thank you for the issue! I think we should definitely have this feature. Do you think having a generic type for the component can improve the DX? Something like below:

type CheckParams = { domain: string; };

<Authenticated<CheckParams>
  params={{ domain: "refinedev" }}
>
  {/* ... */}
</Authenticated>

// Or

const DomainAuthenticated = Authenticated<CheckParams>;

<DomainAuthenticated params={{ domain: "refinedev" }}>{...}</Authenticated>

The implementation can be same with the one you've shared. Let us know if you want to work on this, we'll be happy to see your contribution! 🙏

@aliemir aliemir added this to the October Release milestone Sep 11, 2024
@reedwane
Copy link
Author

Thanks for the feedback @aliemir

While I think it'd be nice to have a type, I opine it should be a generic object Record<string, string> since the check function can integrate with that

There might still be other similar use-case for others.

@BatuhanW BatuhanW removed this from the November Release milestone Nov 11, 2024
@BatuhanW
Copy link
Member

Hey @reedwane would you like to create a PR for this one?

@reedwane
Copy link
Author

@BatuhanW yeah. Lemme see if I can figure how to do that now.

Can you please point me to get started

@BatuhanW
Copy link
Member

@reedwane You can start from Authenticated component in core package. You can find it here

Also, see our contribution guide

It uses useIsAuthenticated, basically you would need to pass additional params. And you can also update tests for Authenticated component to assert the behaviour.

@reedwane
Copy link
Author

@BatuhanW I've created a PR here: #6483

I hope I did all well

@BatuhanW BatuhanW added this to the December '24 Release milestone Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request up for grabs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants