Upgrading node-auth0 from v3 to v4 #887

willvedd · 2024-02-02T20:51:23Z

🔧 Changes

Upgrading node-auth0 from v3 to v4. This is a necessary upgrade because all new Auth0 features and dependency patches are getting released on v4 now.

This was originally attempted in #852 but had a few tricky changes that need to be reviewed. This should not require a major version bump for Deploy CLI despite the breaking changes in v4. There will however be a few awkward situations that will need to be smoothed-over.

📚 References

Original PR: [DO NOT MERGE] Upgrade auth0 to v4 #852

🔬 Testing

📝 Checklist

All new/changed/fixed functionality is covered by tests (or N/A)
I have added documentation for all new/changed functionality (or N/A)

willvedd · 2024-02-07T16:44:48Z

src/context/directory/handlers/clientGrants.ts

-  const allResourceServers = await context.mgmtClient.resourceServers.getAll({
-    paginate: true,
+  // TODO: Bring back paginate: true
+  const { data: { resource_servers: allResourceServers } } = await context.mgmtClient.resourceServers.getAll({


Yes, this is important to include. Before Node v4, Auth0 Node SDK didn't natively support pagination and I don't believe that v4 added it either but this needs to be confirmed. The getAll function is mutated by the Deploy CLI to support the paginate property.

Update: Yes, we need to bring this back and account for the pagination monkey-patch. Perhaps we should extract as a function instead?

willvedd · 2024-02-07T16:48:05Z

src/context/index.ts

      audience: config.AUTH0_AUDIENCE
        ? config.AUTH0_AUDIENCE
        : `https://${config.AUTH0_DOMAIN}/api/v2/`,
    });
-    return clientCredentials.access_token;
+    return clientCredentials.data?.access_token;


Should introduce some checks to prevent an undefined access_token from being persisted through the rest of the process.

willvedd · 2024-02-07T16:50:01Z

src/tools/auth0/client.ts

@@ -167,7 +167,7 @@ export default function pagedClient(client: BaseAuth0APIClient): Auth0APIClient
      frequencyLimit: API_FREQUENCY_PER_SECOND,
      frequencyWindow: 1000, // 1 sec
    }),
-  };
+  } as unknown as Auth0APIClient;


Looks very weird. Can the unknown be removed or this type cast otherwise cleaned up?

Update: Looking at this with Frederik again, we should probably find a way to exclude the pool property or facilitate pagination some other way that can retain the Auth0APIClient type.

willvedd · 2024-02-07T16:54:28Z

src/tools/auth0/handlers/actions.ts

+    // TODO: Should we keep this?
+    delete (addAction as any).deployed;
+    delete (addAction as any).status;


Yes because when you create an action you cannot specify the deploy or status in the payload. However, this should be accomplished with the stripCreateFields: ['deployed', 'status'], in the handler definition above.

willvedd · 2024-02-07T16:55:08Z

src/tools/auth0/handlers/actions.ts

+    // TODO: Should we keep this?
+    (action as any).id = createdAction.id;


Probably keep this so we can have reference of the ID. Should double check through testing though.

willvedd · 2024-02-07T16:55:41Z

src/tools/auth0/handlers/actions.ts

@@ -202,7 +208,9 @@ export default class ActionHandler extends DefaultAPIHandler {
    // Actions API does not support include_totals param like the other paginate API's.
    // So we set it to false otherwise it will fail with "Additional properties not allowed: include_totals"
    try {
-      const actions = await this.client.actions.getAll({ paginate: true });
+      // TODO: bring back paginate: true
+      const { data } = await this.client.actions.getAll();


Suggested change

const { data } = await this.client.actions.getAll();

const { data: {actions} } = await this.client.actions.getAll();

willvedd · 2024-02-07T16:58:56Z

src/tools/auth0/handlers/emailProvider.ts

    if (Object.keys(emailProvider).length === 0) {
      if (this.config('AUTH0_ALLOW_DELETE') === true) {
-        await this.client.emailProvider.delete();
+        // await this.client.emails.delete();


~~This could technically be a breaking change but it also seems like this never worked to begin with. We may be able to replicate a delete-like functionality to the best of our ability.~~

This function was removed because the endpoint wasn't officially supported, so it wasn't included in the SDK. We still might be able to smooth this over by removing email provider configurations.

willvedd · 2024-02-07T17:00:03Z

src/tools/auth0/handlers/emailProvider.ts

      this.updated += 1;
      this.didUpdate(updated);
    } else {
-      const provider = { name: emailProvider.name, enabled: emailProvider.enabled };
-      const created = await this.client.emailProvider.configure(provider, emailProvider);
+      const created = await this.client.emails.configure(emailProvider as PostProviderRequest);


Can this type be applied to the construction of the emailProvider value above rather than smoothing-over as a type cast?

willvedd · 2024-02-07T17:00:22Z

src/tools/auth0/handlers/emailTemplates.ts

+      // TODO: Should we revisit this?
+      delete (updated as any ).body;


@frederikprijck What is there to revisit?

willvedd · 2024-02-07T17:00:35Z

src/tools/auth0/handlers/emailTemplates.ts

+        // TODO: Should we revisit this?
+        delete (created as any).body;


Similarly, @frederikprijck what is there to revisit?

willvedd · 2024-02-07T17:01:22Z

src/tools/auth0/handlers/guardianFactorProviders.ts

+        // TODO: This is quite a change, needs to be validated for sure.
+        if (m.name === 'phone' && m.provider === 'twilio') {
+          provider = await this.client.guardian.getPhoneFactorProviderTwilio();
+        } else if (m.name === 'sms' && m.provider === 'twilio') {
+            provider = await this.client.guardian.getSmsFactorProviderTwilio();
+        } else if (m.name === 'push-notification' && m.provider === 'apns') {
+            provider = await this.client.guardian.getPushNotificationProviderAPNS();
+        } else if (m.name === 'push-notification' && m.provider === 'sns') {
+          provider = await this.client.guardian.getPushNotificationProviderSNS();
+        }


Verify if there are any other providers

willvedd · 2024-02-07T17:02:26Z

src/types.ts

-    getTriggerBindings: () => Promise<Asset>;
-  };
-}; // TODO: replace with a more accurate representation of the Auth0APIClient type
+export type BaseAuth0APIClient = ManagementClient;


Should remove the BaseAuth0APIClient type altogether.

willvedd · 2024-02-09T13:24:53Z

src/tools/auth0/handlers/emailProvider.ts

@@ -16,7 +17,8 @@ export default class EmailProviderHandler extends DefaultHandler {

  async getType(): Promise<Asset> {
    try {
-      return await this.client.emailProvider.get({ include_fields: true, fields: defaultFields });
+      const { data  } = await this.client.emails.get({ include_fields: true, fields: defaultFields.join(',') });


This looks like a breaking change but the SDK types fields as a string so this doesn't precludes the need to type cast.

willvedd · 2024-02-09T13:29:26Z

src/tools/auth0/handlers/index.ts

@@ -20,7 +20,6 @@ import * as guardianPhoneFactorMessageTypes from './guardianPhoneFactorMessageTy
 import * as roles from './roles';
 import * as branding from './branding';
 import * as prompts from './prompts';
-import * as migrations from './migrations';


Migrations not part of the public spec. Need to understand if this was an intentional deprecation or if it is even necessary.

Removing this would be a breaking change so we should consider adding-in back in unless we create a new major for Deploy CLI.

willvedd · 2024-02-09T13:31:49Z

src/tools/auth0/handlers/tenant.ts

 import log from '../../../logger';

 export const schema = {
  type: 'object',
 };

-export type Tenant = Asset & { enabled_locales?: Language[]; flags: { [key: string]: boolean } };
+export type Tenant = TenantSettings;


Would rather we just get rid of the Tenant type altogether in favor of the TenantSettings.

willvedd · 2024-02-09T13:35:06Z

test/e2e/recordings/should-deploy-without-throwing-an-error.json

@@ -2,7 +2,7 @@
    {
        "scope": "https://deploy-cli-dev.eu.auth0.com:443",
        "method": "GET",
-        "path": "/api/v2/rules?include_totals=true&page=0&per_page=100",


These recordings should probably be reverted once the pagination functionality is added back in.

willvedd · 2024-02-09T13:36:14Z

test/e2e/e2e.test.ts

+import fetch from 'node-fetch';
+
+// Use node.http for recording library
+global.fetch = fetch;

 const shouldUseRecordings = process.env['AUTH0_HTTP_RECORDINGS'] === 'lockdown';
 const AUTH0_DOMAIN = shouldUseRecordings


Basically everthing below this line is an innocuous testing change 🙌

willvedd

Outstanding items with this PR:

DELETE email provider SDK function – Review the impact of this. Can this be smoothed-over? Should this be re-added to the SDK?
Migrations removal – Review the impact of this. Do we actually need or want migrations? However, it is a large enough breaking change where if this was removed from the Deploy CLI, you would need to create a new major version
Pagination – Bring back and probably favor a method of implementation that doesn't monkey-patch the SDK. Also look into relocating the pool construct.
Types – Node SDK v4 introduces comprehensive types, which should take precedence over the types defined in this repo. Lots of weird type casting going on, should review and used canonical types from SDK wherever possible.
Node Compatibility – The Node SDK v4 drops support for v16 w/ introduction of fetch. We've dropped node version support in the past without much noise but need to review the usage decide the gravity if this change and if it warrants a new major release or not.

There are some other minor things to review in this PR but otherwise the above are the major chunks of work. IMO, we should attempt to not introduce a new major to facilitate this new SDK because there are many improvements that can be made with a new major that should be provided to developers.

frederikprijck and others added 9 commits August 30, 2023 23:00

Upgrade auth0 to v4

5014c10

Update tests

9fd5767

Ensure e2e tests run against a live tenant

4e37522

Fix linter

437b580

fix tests

ce80361

fix package-lock

da248d0

fix e2e tests

0c70611

Ignore missing client overload error for now

82db2ee

fix unit test

0811df7

willvedd commented Feb 7, 2024

View reviewed changes

willvedd commented Feb 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrading node-auth0 from v3 to v4 #887

Upgrading node-auth0 from v3 to v4 #887

willvedd commented Feb 2, 2024

willvedd Feb 7, 2024 •

edited

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 9, 2024

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 7, 2024 •

edited

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 7, 2024

willvedd Feb 9, 2024

willvedd Feb 9, 2024 •

edited

willvedd Feb 9, 2024

willvedd Feb 9, 2024

willvedd Feb 9, 2024

willvedd left a comment

		// TODO: Should we keep this?
		(action as any).id = createdAction.id;

	const { data } = await this.client.actions.getAll();
	const { data: {actions} } = await this.client.actions.getAll();

		// TODO: Should we revisit this?
		delete (updated as any ).body;

		// TODO: Should we revisit this?
		delete (created as any).body;

Upgrading node-auth0 from v3 to v4 #887

Are you sure you want to change the base?

Upgrading node-auth0 from v3 to v4 #887

Conversation

willvedd commented Feb 2, 2024

🔧 Changes

📚 References

🔬 Testing

📝 Checklist

willvedd Feb 7, 2024 • edited

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

willvedd Feb 7, 2024 • edited

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

willvedd Feb 9, 2024 • edited

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

willvedd left a comment

Choose a reason for hiding this comment

willvedd Feb 7, 2024 •

edited

willvedd Feb 7, 2024 •

edited

willvedd Feb 9, 2024 •

edited