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

Should rootAuthLoader support the loadUser option? #1043

Closed
2 tasks done
tmcw opened this issue Apr 6, 2023 · 14 comments
Closed
2 tasks done

Should rootAuthLoader support the loadUser option? #1043

tmcw opened this issue Apr 6, 2023 · 14 comments
Assignees
Labels
prioritized This issue has been triaged and the team is working on it

Comments

@tmcw
Copy link

tmcw commented Apr 6, 2023

I recently integrated Clerk into my application, which also uses a relational database, so it needed to use Clerk's externalId setting to cross-reference. So, I then used rootAuthLoader with Remix, and the loadUser option to get the user object, to get the externalId.

This worked in development but is a hazard for production: Clerk's rate limit is 5 reqs/second, so once any website goes above that, you start randomly showing users logged-out states and throwing errors.

The eventual solution - helpfully shown by Clerk's support, which has been great - is to store the necessary data in the user session to limit database access. However, I think it's probably a good idea to guard against other users falling into this trap: 5 reqs / second is a very surprisingly low rate limit, and it very much makes it so that you shouldn't ever require a request to clerk for each pageload.

@dimkl dimkl self-assigned this Apr 10, 2023
@dimkl
Copy link
Contributor

dimkl commented Apr 10, 2023

Hello @tmcw, thank you for making this comment.
We have an internal ticket to tackle some issues with the options passed in our middlewares/ loaders / ... across all of our supported SDKs. It seems that there are some inconsistencies based on framework limitations and other reasons.
I leave this issue open and reply when we have finalized our decisions and our API designs to provide a better answer to this.

@tmcw
Copy link
Author

tmcw commented Apr 10, 2023

Thanks. Trying to dodge the rate limit is really our main struggle with Clerk right now. I totally understand why it'd be a good idea to rate-limit login attempts, but for a method like getting a user object or a jwt token, such a brutal rate limit means that we're needing to add caches and workarounds that we didn't anticipate having to build.

@perkinsjr
Copy link
Contributor

Spoke to tmcw regarding the rate limiting so we should be good on that front.

@clerk-cookie
Copy link
Collaborator

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@tmcw
Copy link
Author

tmcw commented Jun 14, 2023

Unless I'm missing something, I still think this issue is valid: if you use rootAuthLoader with the loadUser option, your site will break quickly because of the rate limit. We switched to not using loadUser and trying to rely on the session object, but if loadUser is a footgun, it should probably be documented as such or removed.

@dimkl dimkl removed the Stale label Jun 14, 2023
@dimkl
Copy link
Contributor

dimkl commented Jun 14, 2023

It's still missing. I have marked it as not stale.

@clerk-cookie
Copy link
Collaborator

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@tmcw
Copy link
Author

tmcw commented Jul 14, 2023

Keeping this open for the same reasons as above.

@clerk-cookie
Copy link
Collaborator

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@tmcw
Copy link
Author

tmcw commented Aug 14, 2023

I… wish that this bot would be turned off or this issue would be solved, it's annoying to have to comment to keep this very clear issue open, and have to advocate for a basic feature/doc improvement on a paid product.

@jescalan
Copy link
Contributor

I'm so sorry @tmcw - we're going to look into getting this one closed out soon!

@jescalan jescalan added the prioritized This issue has been triaged and the team is working on it label Aug 25, 2023
@dimkl dimkl removed their assignment Sep 21, 2023
@tmcw
Copy link
Author

tmcw commented Jan 5, 2024

Is this still prioritized?

@jescalan
Copy link
Contributor

jescalan commented Jan 5, 2024

Hey @tmcw! It is, but it got lost in the backlog for ~2.5 months unfortunately. It was recovered about 2 weeks ago and is marked as a documentation improvement at the moment along with a couple deprecation warnings.

@anagstef
Copy link
Member

Thanks @tmcw for mentioning this issue! We have added a deprecation warning on these properties and will be removed in the next major version of @clerk/remix.

The correct solution is the one that is already proposed in the original post: Add the external id info to the session claims and not use any of the loadX functions.

How to setup session claims: https://clerk.com/docs/backend-requests/making/custom-session-token

Example:
Screenshot 2024-05-24 at 19 09 34

Then use it in the rootAuthLoader like this:

export const loader = (args: LoaderFunctionArgs) => rootAuthLoader(args, ({ request }) => {
  const claims = request.auth.sessionClaims;
  if (claims?.extid === null) {
    // do something
  }
  return {
    // data
  };
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prioritized This issue has been triaged and the team is working on it
Projects
None yet
Development

No branches or pull requests

6 participants