Fix type errors in lib/auth.ts when using a non-RBAC dbAuth setup

[Philzen]

Closes #5038

No breaking change, no codemods or other updates needed to existing projects.

---

This fixes a typescript error that has been bugging me for some time. Whenever generating the dbAuth setup, but not implementing any role-based access control (RBAC), vscode goes full on red on auth.ts:

I know one may debate whether this is actually a problem or not (imho generated code should always pass 100 % of all rules in order maximize trust in the framework, i know e.g. @dac09 has a different stance on this), but this one is that severe that it comes up when running yarn rw type-check

Don't know if this is the perfect solution and am open to better ones. But i feel it's just good enough, in my test the rules-type got properly overwritten (becoming mandatory) as soon as one changes getCurrentUser to also return it.

[netlify]

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	fb9109c
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62c44aeec659150008f20cef

[dac09]

Thanks @Philzen again! I need to dig into this a little more, but from an initial glance, this looks like it will overwrite the "roles" type if you return it from your current user function.

So for example if you do this:

export const getCurrentUser = async (session) => {
  return await db.user.findUnique({
    where: { id: session.id },
    select: { id: true, email: true, roles: true },
  })
}

roles will remain as "any"

I stand by my opinion on this:

❌ Making things green when they're not actually true
✅ Point out pieces of the code that you still need to fix/change after a generate, forcing you to look into the generated code.

Remember generators are not library code. They're boiler plate code we give you - but you own this code, not the framework.

---

The reason we don't want to include roles by default is:
a) Not everyone wants RBAC out of the box, and they may have a different way of implementing all together
b) Using a string[] is preferable for roles, but isn't supported by sqlite

[dac09]

On another note, #5491 should handle some of the non-strict errors here!

[Philzen]

looks like it will overwrite the "roles" type if you return it from your current user function

@dac09 You're right... fixed it: 5e9b442

Point out pieces of the code that you still need to fix/change after a generate, forcing you to look into the generated code.

I'll see if i can also add some JSDOC hinting users that this will always be undefined unless they add it to the select in getCurrentUser.

Making things green when they're not actually true

BTW – when looking at mockCurrentUser() intellisense already suggests the roles object and gives no user hint that it's undefined whatsoever. But it think that's down to the fact it resolves to CurrentUser defined in AuthProvider and is a separate issue as such.

[dac09]

Super cool 😍! Yes I'm also looking at the useAuth thing now, see if I can fix that in this PR too :)

[Philzen]

27182fb now adds clear advice whenever accessing context.currentUser.roles on the /api-side:

I'd be tempted to say this is an overall improvement... dbAuth on rails 😉

BTW as a non-native english speaker i'm not sure if everybody's happy with the exact working, hence i'm completely agnostic to any changes regarding that. Maybe @cannikin or others can to throw in some of the redwood-style gleeful cheekyness? 🧐

[Philzen]

Yes I'm also looking at the useAuth thing now, see if I can fix that in this PR too :)

Nice! Though my gut feeling is that may require a larger type refactoring, but by any means i am happy if that happens 🚀
I was thinking on starting an RFC on that after this PR is merged b/c i can see some more requirements here, such as:

• matching the types with the actual implementation (i.e. by using the ones generated from the SDL)
• therefore allowing mockCurrentUser to accept optional types w/o triggering a typescript error
• and of course, making sure there is only one set of types (one "truth"), no matter whether accessing using currentUser from api or web side context or getting it from the useAuth hook

From my part i'd be really happy for the PR to go in as it is. I can offer to start an RFC where you could then tackle the overall type-harmonization.

[dac09]

I can offer to start an RFC where you could then tackle the overall type-harmonization.

This will be much appreciated @Philzen - appreciate you taking such an interest in the "fun" parts of typescript in Redwood!

[Philzen]

Off-Topic but a real thing.

I wish everybody on github would just stop doing that in feature branches. Rebasing is just so much cleaner – that's one thing where Gitlab still has an advantage, b/c they allow to rebase on main at the click of a button. Wish @github did that someday to show people that rebasing is no rocket science but actually the better practice to pull updates into your branch.

Luckily, the update merge practice has no impact on the RedwoodJS repo commit history b/c all PR's are squashed anyway currently, so at least there's no drawback here – other than it's ugly to work with my feature branch on the CLI b/c i'm used to always see the commit's that i'm working on top, so i can easily identify them and craft --fixup commits.

I've rebased now, check out the history.

</rantingModeOff> 😉

---

Oh i just discovered this:

Guess i'll have to take back part of my rant 😇 – now @github should only made that the default selection in order to make the world a little bit better. 🌻

Until that happens, everybody, pleeeeease use it 🧐

[Tobbe]

I wish everybody on github would just stop doing that in feature branches. Rebasing is just so much cleaner

In general I'm a huge rebase proponent. But, as you noted:

Luckily, the update merge practice has no impact on the RedwoodJS repo commit history b/c all PR's are squashed anyway currently

And because of that I think merging is the better choice. The reason being that it has a much greater probability of succeeding. When merging there's only one commit you need to care about, so only one commit that can have merge conflicts. When rebasing every single commit in the feature branch is a potential commit with a merge conflict.

Now, if this was a closed-source project with a controlled set of contributors I'd advocate for not doing a squash-and-merge of feature branches, but rather just rebase them on top of the main branch. But that requires every contributor to manually clean up the commit history of their feature branches before they go in on main. For a company project that's just a matter of educating all employees. But since this is an open source project and we want to make it as easy as possible for people to contribute, even those who might not be as well versed with git, I think squash-and-merge is the better choice. I hope I don't come off as too condescending here. I know you wouldn't have an issue with keeping a tidy feature branch Philzen 🌷