feat(admin): Create 'Relying Parties' page & display RPs from db #13123

LZoog · 2022-06-02T00:37:14Z

Because:

We want a single source of truth to view FxA / Subplat relying parties

This commit:

Adds a new RP model and resolver, uses a new query to read desired rows from clients table in fxa_oauth database
Displays this data in the UI with a new /relying-parties page
Adds RP guard to existing admin panel guards and to server/client
Contains a Nav account icon update, CSS tweaks

There's 2-3 follow up issues I'm going to file around our HTML/CSS as well as some consistency displaying stuff, and not having an AppErrorBoundary (not sure if we want errors reported to Sentry). I could probably also file one for Storybook since I haven't even ran that in the admin-panel in a while and haven't (yet) while working on this PR. 😬

LZoog · 2022-06-03T01:06:42Z

packages/fxa-admin-panel/src/components/PageRelyingParties/index.tsx

+  );
+};
+
+export default PageRelyingParties;


The SVGs shown below are rendering funny in GH because icon-account has an explicit width/height set on it that matches its viewbox, and icon-key has a larger viewbox with no explicit width/height. I tried making them more consistent but ran into at least one of them looking funky and didn't think it was worth spending more time on since they look just fine in the UI and it's an internal tool anyway.

LZoog · 2022-06-03T01:26:08Z

packages/fxa-admin-panel/src/styles/tailwind.css

+  @apply font-medium text-violet-900 pl-4 pr-8 text-left;
+}
+
+.result-grey {


I'm going to file a follow up issue around cleaning up all the lists vs tables we're using as well as padding/margin/text color adjustments (I like this result-grey), colon vs no colons (or question marks for booleans), and maybe even a helper for displaying our booleans as "Yes" / "No" or true/false if that's what we prefer. Couple other things too like renaming our other pages to begin with Page to match what we're doing in Settings.

Sounds good! Consistency is always appreciated.

Filed #13164 for this bit

LZoog · 2022-06-03T17:35:04Z

packages/fxa-admin-server/src/database/database.service.ts

  async onModuleDestroy() {
    // This tear down is important for jest tests. Without this
    // the tests will hang on completion.
    await this.connectedServicesDb.cache.close();
    await this.connectedServicesDb.db.close();
+    // await this.mySqlOAuthShared.close();


Hmm, I was seeing some strange output in CI that I thought was hanging on my RP resolver tests (and they took a while locally) so I wanted to see if this helped even though I didn't think I'd need it since I didn't need knex and I'm mocking the db call with this (which should be a const that I'll also update). I didn't realize I committed this while commented out, but CI seems happy now, so I'll remove it next time I push an update.

Interesting. ConnectedServicesDb is effectively calling close on mySqlOAuthShared (since it's passed into the constructor and it has a referende to it). I wonder if calling close twice becomes an issue... Anyways removing it seems good to me.

Yeah, it was a "too long without output" error and I just figured this is what it was. I think it was something flaky or something, I've just pushed another update so I'll merge when it passes.

dschom · 2022-06-04T00:20:28Z

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

+
+import React from 'react';
+import { render, screen } from '@testing-library/react';
+import * as apollo from '@apollo/client';


apollo not used

Good catch, yeah thought I'd need to mock it and didn't need it. I'm looking forward to us using this package, looks like we need new issues filed for it so it's per package, though I'm not sure I care if it's per package personally so I'll bring it up in triage.

dschom · 2022-06-04T00:23:41Z

packages/fxa-admin-panel/src/components/Nav/index.tsx

-        </Guard>
-        <Guard features={[AdminPanelFeature.AccountLogs]}>
+export const Nav = () => {
+  const getNavLinkClassName = (isActive: boolean) =>


Random nit pick. Maybe put this in the outer scope? Seems like adding this function, resulted in a using a 'return' statement, which in turn had a big impact on the the diff.

dschom · 2022-06-04T00:31:34Z

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

-                <Route path="/account-search" element={<AccountSearch />} />
+                <>
+                  <Route path="/account-search" element={<AccountSearch />} />
+                  <Route path="/" element={<Navigate to="/account-search" />} />


Nice! This seems like a good idea to me.

dschom · 2022-06-04T00:47:47Z

packages/fxa-admin-server/src/gql/model/relying-party.model.ts

+  @Field()
+  trusted!: boolean;
+
+  @Field({ nullable: true })


In fxa-shared/db/models/auth/relying-party.ts, this value is marked as non nullable. Just double checking there's not a mismatch here.

Another good catch - this is definitely nullable, I'll change it over there.

dschom · 2022-06-04T00:56:37Z

packages/fxa-admin-server/src/database/database.service.ts

@@ -79,8 +86,9 @@ export class DatabaseService implements OnModuleDestroy {
    this.device = Device;
    this.linkedAccounts = LinkedAccount;

+    this.mySqlOAuthShared = mySqlOAuthShared;


Probably not a thing for this PR, but as I see this evolving, I wonder if we we should have more services broken out. Smaller services are usually more maintainable and modular.

So perhaps we have several supporting services here that can be injected,

MySqlOAuthSharedService

ConnectedServicesCacheService

ConnectedServicesDbService

I agree. This definitely felt a little less-than-ideal here but okay for now since it was a simple change.

dschom · 2022-06-04T00:58:16Z

@LZoog, things are looking good to me. I'll do some exploratory testing on Monday and likely r+.

dschom

LGTM! I tested this locally and this is definitely a nice addition. Any feedback I left shouldn't be seen as blocking. If there is any feedback you'd like to turn into separate follow up items, let me know and I can add them to the follow up epic.

dschom · 2022-06-06T18:50:59Z

packages/fxa-admin-panel/src/components/PageRelyingParties/index.tsx

+                    <th>Allowed Scopes</th>
+                    <td>
+                      {allowedScopes ? (
+                        allowedScopes


Just a nitpick. When I was testing this locally, I noticed that scopes are delimitted by a whitespace, which is kind of hard to read. Mabye we can split on whitespace, and display them as a list?

That WFM, I didn't think to alter these since that's how they're stored, but I like the human readability aspect we've got for other fields so I went ahead and changed it here since it was quick.

Because: * We want a single source of truth to view FxA / Subplat relying parties This commit: * Adds a new RP model and resolver, uses a new query to read desired rows from clients table in fxa_oauth database * Displays this data in the UI with a new /relying-parties page * Adds RP guard to existing admin panel guards and to server/client * Contains a Nav account icon update, CSS tweaks

LZoog force-pushed the issues/12781 branch 2 times, most recently from ff4120d to 7ee5a6f Compare June 3, 2022 01:03

LZoog marked this pull request as ready for review June 3, 2022 01:03

LZoog requested a review from a team as a code owner June 3, 2022 01:03

LZoog force-pushed the issues/12781 branch from 7ee5a6f to a3e6dd3 Compare June 3, 2022 01:22

LZoog commented Jun 3, 2022

View reviewed changes

LZoog force-pushed the issues/12781 branch 3 times, most recently from 5fd75b4 to 782b1c3 Compare June 3, 2022 16:17

LZoog commented Jun 3, 2022

View reviewed changes

dschom self-requested a review June 4, 2022 00:11

dschom reviewed Jun 4, 2022

View reviewed changes

dschom approved these changes Jun 6, 2022

View reviewed changes

LZoog force-pushed the issues/12781 branch from 782b1c3 to 174b144 Compare June 6, 2022 23:16

LZoog force-pushed the issues/12781 branch from 174b144 to c909a63 Compare June 7, 2022 15:00

LZoog merged commit f2b7c2f into main Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(admin): Create 'Relying Parties' page & display RPs from db #13123

feat(admin): Create 'Relying Parties' page & display RPs from db #13123

LZoog commented Jun 2, 2022 •

edited

Loading

LZoog Jun 3, 2022

LZoog Jun 3, 2022

dschom Jun 4, 2022

LZoog Jun 6, 2022

LZoog Jun 3, 2022

dschom Jun 4, 2022 •

edited

Loading

LZoog Jun 6, 2022

dschom Jun 4, 2022

LZoog Jun 6, 2022 •

edited

Loading

dschom Jun 4, 2022

dschom Jun 4, 2022 •

edited

Loading

dschom Jun 4, 2022

LZoog Jun 6, 2022

dschom Jun 4, 2022

LZoog Jun 6, 2022

dschom commented Jun 4, 2022

dschom left a comment

dschom Jun 6, 2022

LZoog Jun 6, 2022 •

edited

Loading

feat(admin): Create 'Relying Parties' page & display RPs from db #13123

feat(admin): Create 'Relying Parties' page & display RPs from db #13123

Conversation

LZoog commented Jun 2, 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

dschom Jun 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschom Jun 4, 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

dschom commented Jun 4, 2022

dschom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LZoog Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

LZoog commented Jun 2, 2022 •

edited

Loading

dschom Jun 4, 2022 •

edited

Loading

LZoog Jun 6, 2022 •

edited

Loading

dschom Jun 4, 2022 •

edited

Loading

LZoog Jun 6, 2022 •

edited

Loading