-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrading node-auth0 from v3 to v4 #887
base: master
Are you sure you want to change the base?
Conversation
const allResourceServers = await context.mgmtClient.resourceServers.getAll({ | ||
paginate: true, | ||
// TODO: Bring back paginate: true | ||
const { data: { resource_servers: allResourceServers } } = await context.mgmtClient.resourceServers.getAll({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: This could be an opportunity to improve the pagination. I think that with the v4 SDK bringing types that monkey-patching the client like this could be problematic but if you're able to get it work then great. Otherwise may need to break out into a separate function or some other abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update 2: To be clear, the existing code should/will work, the only considerations is to get the types to match up. Because the SDK wasn't typed in v3, we had to create our own BaseAuth0APIClient
type and augment with the pool
property. The proposed code in this PR should actually be removed and probably instead replace the BaseAuth0APIClient
with the v4 SDK's native management client type. There may be additional type mismatches but that should get this to a mostly-functioning state.
audience: config.AUTH0_AUDIENCE | ||
? config.AUTH0_AUDIENCE | ||
: `https://${config.AUTH0_DOMAIN}/api/v2/`, | ||
}); | ||
return clientCredentials.access_token; | ||
return clientCredentials.data?.access_token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should introduce some checks to prevent an undefined access_token
from being persisted through the rest of the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommendation: Ensure ample amount of undefined checks where appropriate.
@@ -167,7 +167,7 @@ export default function pagedClient(client: BaseAuth0APIClient): Auth0APIClient | |||
frequencyLimit: API_FREQUENCY_PER_SECOND, | |||
frequencyWindow: 1000, // 1 sec | |||
}), | |||
}; | |||
} as unknown as Auth0APIClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very weird. Can the unknown
be removed or this type cast otherwise cleaned up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// TODO: Should we keep this? | ||
delete (addAction as any).deployed; | ||
delete (addAction as any).status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// TODO: Should we keep this? | ||
(action as any).id = createdAction.id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably keep this so we can have reference of the ID. Should double check through testing though.
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { data } = await this.client.actions.getAll(); | |
const { data: {actions} } = await this.client.actions.getAll(); |
if (Object.keys(emailProvider).length === 0) { | ||
if (this.config('AUTH0_ALLOW_DELETE') === true) { | ||
await this.client.emailProvider.delete(); | ||
// await this.client.emails.delete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this type be applied to the construction of the emailProvider
value above rather than smoothing-over as a type cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean by this is that the typing can probably be improved. I don't immediately see how but I'm sure it can to avoid this messy type casting.
// TODO: Should we revisit this? | ||
delete (updated as any ).body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frederikprijck What is there to revisit?
// TODO: Should we revisit this? | ||
delete (created as any).body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, @frederikprijck what is there to revisit?
// 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(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify if there are any other providers. It looks like all are covered here but it should be verified.
getTriggerBindings: () => Promise<Asset>; | ||
}; | ||
}; // TODO: replace with a more accurate representation of the Auth0APIClient type | ||
export type BaseAuth0APIClient = ManagementClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove the BaseAuth0APIClient
type altogether.
@@ -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(',') }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a breaking change but the SDK types fields
as a string so this doesn't precludes the need to type cast. This should be a no-op.
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import log from '../../../logger'; | ||
|
||
export const schema = { | ||
type: 'object', | ||
}; | ||
|
||
export type Tenant = Asset & { enabled_locales?: Language[]; flags: { [key: string]: boolean } }; | ||
export type Tenant = TenantSettings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would rather we just get rid of the Tenant
type altogether in favor of the TenantSettings
.
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These recordings should probably be reverted once the pagination functionality is added back in.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically everthing below this line is an innocuous testing change 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
🔧 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
🔬 Testing
📝 Checklist