-
Notifications
You must be signed in to change notification settings - Fork 37
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
[CF-447] Use paginated requests #61
Conversation
Code Climate has analyzed commit 467349a and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 93.5% (50% is the threshold). This pull request will bring the total coverage in the repository to 89.9%. View more on Code Climate. |
server/routes/api.js
Outdated
@@ -39,11 +39,13 @@ export default (storage) => { | |||
})); | |||
|
|||
const auth0 = middlewares.managementApiClient({ | |||
domain: config('AUTH0_DOMAIN') | |||
domain: config('AUTH0_DOMAIN'), | |||
clientId: config('AUTH0_CLIENT_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.
Why did this work without a clientID/clientSecret? It shouldn't had: https://github.com/auth0-extensions/auth0-extension-tools/blob/c8d9b88e37c3174cf4eaeb7bf7e3c3c0f20550bf/src/auth0/managementApi.js#L72
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 is using the express middleware that uses the token issued to the user to call API2 so there is no need to have a client ID https://github.com/auth0-extensions/auth0-extension-express-tools/blob/master/src/middlewares/managementApiClient.js#L7-L11
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.
Thanks, missed the access token was being resolved from the req.user
🙇 .
@@ -4,10 +4,10 @@ import config from '../lib/config'; | |||
import * as authorization from '../lib/authorization'; | |||
import { requireScope } from '../lib/middlewares'; | |||
|
|||
export default (storage) => { | |||
export default (auth0, storage) => { |
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.
Follows the same patter as in applications
, where the middleware is passed as parameter.
@@ -183,7 +192,7 @@ export const deleteResourceServer = req => | |||
}); | |||
|
|||
const getGrantId = req => | |||
makeRequest(req, 'client-grants', 'GET') | |||
makeRequest(req, 'client-grants', 'GET', { client_id: config('AUTH0_CLIENT_ID'), audience: 'urn:auth0-authz-api' }) |
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.
Pass the options supported in https://auth0.com/docs/api/management/v2#!/Client_Grants/get_client_grants to filter instead of getting all results.
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.
Are we fine not doing pagination here?
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.
I believe so, because this will only return 1 or 0 results (depending on if there's a grant for the client_id/API combination or not).
@@ -151,7 +160,7 @@ const makeRequest = (req, path, method, payload) => | |||
); | |||
|
|||
export const getResourceServer = (req, audience) => | |||
makeRequest(req, 'resource-servers', 'GET') | |||
multipartRequest(req.auth0, 'resourceServers') |
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.
req.auth0
injected by middlewares.
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.
The PR diff size of 24305 lines exceeds the maximum allowed for the inline comments feature.
✏️ Changes
Use paginated requests for resource servers and client grants.
The call to resource servers was verified by enabling
ALLOW_AUTHZ
and checking that it fetched results correctly.🎯 Testing
✅ This change has been tested in a Webtask
✅ This change has unit test coverage
✅ This change has integration test coverage
🚫 This change has been tested for performance
🚀 Deployment
✅ This can be deployed any time