fix(admin-panel): Fix remote group resolution #12871

dschom · 2022-05-12T23:43:39Z

Because

The remote group was not resolved correctly which rendered the admin panel useless.

This pull request

Targets the correct header, 'remote-groups', which was previously 'REMOTE-GROUP'. This small thing was really the crux of the problem.
Does some cleanup and removes magic strings for header keys.
Makes the AdminPanelGuard aware of the current environment. This ended up being the bulk of the changes, and was necessary since user groups are environment specific, and therefore the guard needs to be able to distinguish this. I.e. A user user could be part of an ldap group that has admin privileges on stage, but not part of the ldap group that has admin privileges on prod.
Resolves the correct user group for the current environment. For example, when both stage and prod ldap groups are provided in the remote-groups header, we must resolve the group that’s most appropriate for the current environment.
Introduces a hook for GuardContext. Now that the AdminPanelGuard is aware of the current environment it is essentially contextual and should be accessed from a hook.

Issue that this pull request solves

Closes: #12744

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).

dschom · 2022-05-12T23:44:16Z

packages/fxa-admin-panel/server/lib/client-config/index.test.ts

@@ -15,8 +21,8 @@ describe('ClientConfig', () => {
      user,
    });
    const injectedHtml = ClientConfig.injectIntoHtml(html, {
-      'REMOTE-GROUP': remoteHeader,
-      'oidc-claim-id-token-email': user.email,
+      [USER_GROUP_HEADER]: remoteHeader,


cleanup - avoid magic strings

dschom · 2022-05-12T23:45:24Z

packages/fxa-admin-panel/server/lib/client-config/index.test.ts

+    });
+  });
+
+  it('handles multiple remote group headers', () => {


This is a more realistic test. Turns out a large number of ldap groups can be passed on this header.

dschom · 2022-05-12T23:46:40Z

packages/fxa-admin-panel/src/App.test.tsx


 it('renders without imploding', () => {
+  const guard = new AdminPanelGuard(AdminPanelEnv.Prod);


Guards are now aware of the environment. This is needed to determine the case when a user might be in a stage ldap group but not in a prod ldap group (or vice versa).

dschom · 2022-05-12T23:47:31Z

packages/fxa-admin-panel/src/App.tsx


 const App = ({ config }: { config: IClientConfig }) => {
+  const [guard, setGuard] = useState<AdminPanelGuard>(config.guard);


Since guards are now aware of the environment, they technically need to be part of the context.

dschom · 2022-05-12T23:48:43Z

packages/fxa-admin-server/README.md

@@ -8,12 +8,12 @@ The [GraphQL playground](https://www.apollographql.com/docs/apollo-server/testin

 The playground requires an `oidc-claim-id-token-email` authorization header. In production this is supplied through an nginx header after LDAP credentials, which have been verified but in development, a dummy email should be supplied in the bottom left-hand corner of the GQL playground labeled "HTTP Headers":

-In addition a `REMOTE-GROUP` header must also be set to indicate the user's LDAP group membership. Again, in production this will be set by nginx, but in development, a dummy value must be supplied.
+In addition a `remote-groups` header must also be set to indicate the user's LDAP group membership. Again, in production this will be set by nginx, but in development, a dummy value must be supplied.


Yep, we were targeting the wrong header. This was the main source of the issue.

dschom · 2022-05-13T00:48:56Z

packages/fxa-admin-panel/src/components/AccountSearch/Subscription/index.test.tsx

@@ -26,7 +26,9 @@ it('renders each field as expected', () => {

  screen.getByText(subscription.productName);
  screen.getByText(subscription.status);
-  expect(screen.getAllByText('1970-01-19 @', { exact: false })).toHaveLength(4);
+  expect(screen.getAllByText(/1970-01-1[89] @/, { exact: false })).toHaveLength(


This was tricky. The test was failing and I thought I was going crazy cause clearly it passed at some point. Turns out the issue is that dateFormat is taking the users timezone into account. So depending on where you are you might get a different result. For example:

> const dateFormat = require('dateformat') undefined > dateFormat(new Date(1583259953), 'UTC:yyyy-mm-dd @ HH:MM:ss Z') '1970-01-19 @ 07:47:39 UTC' > dateFormat(new Date(1583259953), 'yyyy-mm-dd @ HH:MM:ss Z') '1970-01-18 @ 23:47:39 PST'

In a PST timezone, 1583259953, would have renders with 1970-01-18 date and the subsequent test would pass. This makes the check a bit more flexible. Alternatively we could render everything in UTC. Not sure what's best.

I've never used dateFormat. moment is used in fxa-auth-server though (and in fxa-shared), could be worth looking into/switching as an alternative?

Agree that'd be nice to consistent about this. @LZoog any thoughts here?

If we're only using dateFormat in the admin-panel, agreed, we could follow a follow up to replace it with moment; less dependencies WFM.

clouserw · 2022-05-13T14:28:29Z

You pre-seeded this PR with a few good notes here. If you'll indulge me a drive-by comment: Your comments are what I'd like to see if I were modifying this file 6 months down the road. Eg. figuring out why a test regex is [89] instead of 9 feels like the kind of thing a person could rabbit-hole on, or read an inline comment and think "oh yeah, good point about timezones."

Thanks for the patch!

xlisachan

LGTM! 🎉

packages/fxa-admin-panel/src/App.tsx

xlisachan · 2022-05-13T15:28:56Z

packages/fxa-admin-panel/src/components/AccountSearch/Subscription/index.test.tsx

@@ -26,7 +26,9 @@ it('renders each field as expected', () => {

  screen.getByText(subscription.productName);
  screen.getByText(subscription.status);
-  expect(screen.getAllByText('1970-01-19 @', { exact: false })).toHaveLength(4);
+  expect(screen.getAllByText(/1970-01-1[89] @/, { exact: false })).toHaveLength(


I've never used dateFormat. moment is used in fxa-auth-server though (and in fxa-shared), could be worth looking into/switching as an alternative?

Because: - The remote group was not resolved correctly which rendered the admin panel useless. This Commit: - Targets the correct remote group header, 'remote-groups', which was previously 'REMOTE-GROUP'. - Does some clean up and removes magic starings. - Makes the guard aware of the current environment. This ended up being the bulk of the changes, and was necessary nice certain - Handles edge case where both stage and ldap groups are provided in remote-groups header. AdminPanelGuard is now aware of the current environment. - Introduces a hook for GuardContext. This hook provides a guard instance, which is aware of the current environment.

dschom force-pushed the FXA-5027-fix-remote-group-resolution branch 3 times, most recently from 380d4cc to 2f363eb Compare May 13, 2022 00:43

dschom commented May 13, 2022

View reviewed changes

dschom changed the title ~~WIP~~ fix(admin-panel): Fix remote group resolution May 13, 2022

dschom marked this pull request as ready for review May 13, 2022 01:08

dschom requested a review from a team as a code owner May 13, 2022 01:08

dschom requested review from LZoog, millmason and xlisachan and removed request for millmason May 13, 2022 14:04

dschom force-pushed the FXA-5027-fix-remote-group-resolution branch from 2f363eb to f1e91d7 Compare May 13, 2022 15:21

xlisachan approved these changes May 13, 2022

View reviewed changes

dschom force-pushed the FXA-5027-fix-remote-group-resolution branch from f1e91d7 to 6339b0b Compare May 13, 2022 16:21

dschom merged commit 9c14190 into main May 13, 2022

dschom deleted the FXA-5027-fix-remote-group-resolution branch June 15, 2022 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(admin-panel): Fix remote group resolution #12871

fix(admin-panel): Fix remote group resolution #12871

dschom commented May 12, 2022 •

edited

Loading

dschom May 12, 2022

dschom May 12, 2022 •

edited

Loading

dschom May 12, 2022 •

edited

Loading

dschom May 12, 2022

dschom May 12, 2022

dschom May 13, 2022

xlisachan May 13, 2022

dschom May 13, 2022

LZoog May 13, 2022

clouserw commented May 13, 2022

xlisachan left a comment

xlisachan May 13, 2022


		it('renders without imploding', () => {
		const guard = new AdminPanelGuard(AdminPanelEnv.Prod);


		const App = ({ config }: { config: IClientConfig }) => {
		const [guard, setGuard] = useState<AdminPanelGuard>(config.guard);

fix(admin-panel): Fix remote group resolution #12871

fix(admin-panel): Fix remote group resolution #12871

Conversation

dschom commented May 12, 2022 • edited Loading

Because

This pull request

Issue that this pull request solves

Checklist

Choose a reason for hiding this comment

dschom May 12, 2022 • edited Loading

Choose a reason for hiding this comment

dschom May 12, 2022 • edited Loading

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

clouserw commented May 13, 2022

xlisachan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschom commented May 12, 2022 •

edited

Loading

dschom May 12, 2022 •

edited

Loading

dschom May 12, 2022 •

edited

Loading