admin/users table with update-claims #242 #325

lefnire · 2022-03-07T23:26:42Z

WIP for admin/users knex/documentation#242. Pushing now to get eyes on obvious shortcomings, and to put a fire under my butt to stop impostering.

admin/users lists users & allows updating roles
search
roles are currently check-boxes. Also, I'm considering roles == Firebase.claims; LMK if that assumption should be reconsidered
how do we want /admin to look? Do we want tabs with inline content (admin/transactions, admin/users); or side-bar tabs?

dallashuggins

Awesome work! Looking great. Added some comments, let me know if you'd like to further discuss anything.

For the style, I think the individual pages look great. Not sure how best to layout the main admin page. Could be good to sync with Nat and see if he has some input. A sidebar similar to the profile would be great, though not sure it'd work well with a table.

Re: the roles - sorry not quite understanding. Do you mean that we're not validating which roles are available to use as the claims?

apps/web/src/pages/api/v1/admin/list-users.ts

dallashuggins · 2022-03-14T20:34:03Z

apps/web/src/pages/admin/users/index.tsx

+  const updateClaim = useCallback(async () => {
+    // show in UI before it goes through
+    setChecked(!checked)
+    const updatedClaims = await adminService.updateClaims(
+      user.externalId,
+      role.id,
+      !checked
+    )
+    // Then update with server-side result
+    setChecked(updatedClaims.claims.includes(role.id))
+  }, [checked])


It'd be nice to have a confirmation warning when you change the value. Wondering if a dropdown or something more intentional may work well - just concerned with accidental clicks. Though a warning would hopefully prevent that also. Also, there may be additional roles down the road, so a dropdown could be more flexible, but also that hasn't been discussed and may not be needed. Curious to hear what you think on that.

I'd recommend just using window.confirm until we have our own confirmation component/context built.

For now as window.confirm. Will move to dropdown if y'all prefer.

Will also pull the PR changes to this repo if all's kosher; just realized I made those on another project

Edited comment from @lefnire

dallashuggins · 2022-03-14T20:36:52Z

apps/web/src/pages/api/v1/admin/update-claims.ts

-      } catch {
-        throw new BadRequest('Unable to set claims')
-      }
+    const firebaseUser = await admin.auth().getUser(userExternalId)


Perhaps we should add a check to see if the user is editing their own account, here and/or in the UI, since that probably should be prevented.

dallashuggins · 2022-03-14T22:11:50Z

apps/web/src/pages/admin/users/index.tsx

+  const submitSearch = useCallback(
+    debounce((value) => {
+      setSearchDebounced(value)
+    }, 200),
+    []
+  )


The search works great currently, but wondering if it's worth adding a new library. We could wait until the search term is submitted, or possibly build a similar utility ourselves. Curious how others feel though.

Cool cool, I'll follow y'all's lead. With lodash.get in there, would we be interested in just lodash - since I'm adding lodash.debounce? Lotsa handies in Lodash. And we'd just double-ensure tree-shaking (eg npm i lodash-es, or import x from 'lodash/x') so no added bloat

I'm not really familiar with all the different tools lodash offers, but open to it if it seemed worthwhile, though I'm not sure it's needed to add for this purpose. @smonn Did you had lodash originally? Do you think this would be a benefit to the application?

If we can ensure lodash doesn't bloat our JS bundle too much I'm okay with adding some utilities from there. Check output sizes for the web project before and after referencing it.

From my experience though, lodash haven't always played well with tree-shaking features and importing just a single module gives you a lot more than you bargained for. There's also a risk a future developer that is less familiar with these concepts imports all of lodash, if we allow it here.

While on the subject of bundle sizes, assuming you use VS Code, I recommend this extension: https://marketplace.visualstudio.com/items?itemName=wix.vscode-import-cost

@smonn that's an awesome extension! I'll install it asap

GTK on destructuring, looks good if direct lodash import. ~~Also verified I wasn't tripping with icons [1]~~. This plugin will be a boon, glad you pointed it out. You never know which modules handle restructuring well, and it's a PITA to direct-import every little thing.

[1] actually, getting different results in PyCharm's Import Cost. Will investigate some on how these extensions shake out via Next.js Bundle Analyzer

Here's w lodash.get/debounce -> lodash (plain) + tree-shook imports. More than happy to not do this route btw, and just add "search" button

lefnire · 2022-03-14T22:26:42Z

Thanks for the review @dallashuggins! I'll tackle these bits.

Re: the roles - sorry not quite understanding. Do you mean that we're not validating which roles are available to use as the claims?

Just mean that currently (AFAIK), Firebase custom claims is synonymous with Roles (because there's only one claim I see in the codebase: and that's {admin: <bool>}). Ie, this PR assumes that to remain so: that FB custom-claims means Object(<role:string>=<bool>). If we anticipate using FB claims for non-role meta-data downstream, I'll want to reconsider this PR

dallashuggins · 2022-03-15T12:58:09Z

Just mean that currently (AFAIK), Firebase custom claims is synonymous with Roles (because there's only one claim I see in the codebase: and that's {admin: <bool>}). Ie, this PR assumes that to remain so: that FB custom-claims means Object(<role:string>=<bool>). If we anticipate using FB claims for non-role meta-data downstream, I'll want to reconsider this PR

@lefnire Aw okay, gotcha - thanks for clarifying! No plans to use claims for anything outside of roles, I think it makes sense to have those be synonymous.

afraser · 2022-03-16T15:31:31Z

apps/web/src/pages/admin/users/index.tsx

+
+export const getServerSideProps: GetServerSideProps = async (context) => {
+  // Check if the user is admin (check again on render, to prevent caching of claims)
+  const user = await isAuthenticatedUserAdmin(context)


We should be able to drop getServerSideProps here entirely and just use useAdmin({ redirectIfNotAdmin: true }) in the component.

Since this is an admin page though, shouldn't we kick a user out ASAP? Can also explore Next's new middleware feature: https://nextjs.org/docs/middleware

When we tried to do this before it was way too aggressive (often kicked me out even though I was logged in as an admin). I'm not sure why this happened but based on the behavior, my guess would be something to do with refresh tokens.

Ideally, we'd be able to do this on the backend but if it's actually not possible (ie: firebase needs to look at cookies or something), I don't really see a problem with a client-side redirect.

smonn

Left some more comments.

smonn · 2022-03-16T15:52:21Z

apps/api/src/modules/accounts/accounts.service.ts

+
+    // Find payer
+    if (search?.length > 0) {
+      const ilikeSearch = `%${search}%`


Do we need to do any sanitizing here?

I assumed any value in a non-raw is escaped, so even though it's interpolated here it's passed to .where(.., ilikeSearch) and .orWhere. Some poking around https://github.com/knex/documentation/issues/73#issuecomment-572482153 , I'm not completely sure now...

smonn · 2022-03-16T15:54:14Z

apps/web/src/pages/admin/users/index.tsx

+  const submitSearch = useCallback(
+    debounce((value) => {
+      setSearchDebounced(value)
+    }, 200),
+    []
+  )


While on the subject of bundle sizes, assuming you use VS Code, I recommend this extension: https://marketplace.visualstudio.com/items?itemName=wix.vscode-import-cost

apps/web/src/pages/admin/users/index.tsx

…sting roles (#242)

lefnire · 2022-03-17T20:25:37Z

Updated PR with changes requested. The bit I'm not sure about is getServerSideProps -> useAdmin, as the page/table shows for anon after that change (albeit empty of users)

afraser · 2022-03-17T20:36:38Z

@lefnire I actually don't see useAdmin({ redirectIfNotAdmin: true }) anywhere in the code right now BUT I also took a look at the other admin pages and I see that we're actually using isAuthenticatedUserAdmin in getServerSideProps there the same as you were, and also that it does in fact check the cookies. That's my bad, I must be remembering an earlier iteration of this before we pinned down the redirect behavior.

lefnire · 2022-03-17T20:58:59Z

@afraser sorry, forgot to mention - <AdminLayout/> (which AdminUsersPage uses) has useAdmin. Also tried useAdmin directly inside AdminUsersPage, same. To verify, you thinking I should switch back to getServerSideProps then (per the other admin pages)?

Just noticed some changes I need to make; I was assuming axios (still on ky).

afraser · 2022-03-21T13:19:34Z

@lefnire Ah I forgot about the layout. And yes, I think it makes the most sense to keep it consistent with the other pages. Sorry for the run around. It's been a bit since I hooked up the admin views.

lefnire · 2022-03-23T00:06:54Z

added getServerSideProps back, fixed ky v axios

afraser · 2022-03-23T20:21:20Z

@lefnire looks like CI isn't going to run until we get this into a branch. Everything looks good to me though. Appreciate the attention to detail on the sorting and filtering.

dallashuggins reviewed Mar 14, 2022

View reviewed changes

afraser reviewed Mar 16, 2022

View reviewed changes

smonn reviewed Mar 16, 2022

View reviewed changes

afraser mentioned this pull request Mar 17, 2022

WIP: admin page for users #283

Closed

deptagency deleted a comment from lefnire Mar 17, 2022

lefnire added 4 commits March 17, 2022 13:10

admin/users table with update-claims, currently only check-box on exi…

21cd29f

…sting roles (#242)

admin/users add username/email search (#242)

fccad27

admin/users axios fixes

993d0f1

admin/users review fixes

643e317

admin/users add getServerSideProps back; ky v axios fixes

df66879

afraser approved these changes Mar 23, 2022

View reviewed changes

lefnire mentioned this pull request Mar 23, 2022

admin/users table with update-claims #375

Merged

lefnire closed this Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

admin/users table with update-claims #242 #325

admin/users table with update-claims #242 #325

lefnire commented Mar 7, 2022 •

edited

dallashuggins left a comment

dallashuggins Mar 14, 2022

afraser Mar 16, 2022

smonn Mar 17, 2022

dallashuggins Mar 14, 2022

dallashuggins Mar 14, 2022

lefnire Mar 14, 2022

dallashuggins Mar 15, 2022

smonn Mar 16, 2022

smonn Mar 16, 2022

lefnire Mar 16, 2022

lefnire Mar 16, 2022 •

edited

lefnire Mar 17, 2022

lefnire commented Mar 14, 2022

dallashuggins commented Mar 15, 2022

afraser Mar 16, 2022

smonn Mar 16, 2022

afraser Mar 16, 2022

afraser Mar 16, 2022

smonn left a comment

smonn Mar 16, 2022

lefnire Mar 17, 2022

smonn Mar 16, 2022

lefnire commented Mar 17, 2022

afraser commented Mar 17, 2022

lefnire commented Mar 17, 2022

afraser commented Mar 21, 2022

lefnire commented Mar 23, 2022

afraser commented Mar 23, 2022

admin/users table with update-claims #242 #325

admin/users table with update-claims #242 #325

Conversation

lefnire commented Mar 7, 2022 • edited

dallashuggins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lefnire Mar 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lefnire commented Mar 14, 2022

dallashuggins commented Mar 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lefnire commented Mar 17, 2022

afraser commented Mar 17, 2022

lefnire commented Mar 17, 2022

afraser commented Mar 21, 2022

lefnire commented Mar 23, 2022

afraser commented Mar 23, 2022

lefnire commented Mar 7, 2022 •

edited

lefnire Mar 16, 2022 •

edited