-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: include new validation on validateAdminUserAccess #181
base: master
Are you sure you want to change the base?
Conversation
Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖 Please select which version do you want to release:
And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.
|
Beep boop 🤖 I noticed you didn't make any changes at the
In order to keep track, I'll create an issue if you decide now is not a good time
|
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.
Nice! I think that is the idea! I just left some small comments.
graphql/schema.graphql
Outdated
@validateAdminUserAccess(orgPermission: "buyer_organization_view") | ||
@validateStoreUserAccess(orgPermission: "buyer_organization_view") |
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.
We should have just one validate directive per API (in this case we should keep only the @validateStoreUserAccess
graphql/directives.graphql
Outdated
directive @validateAdminUserAccess on FIELD | FIELD_DEFINITION | ||
directive @validateStoreUserAccess on FIELD | FIELD_DEFINITION | ||
directive @validateAdminUserAccess( | ||
orgPermission: String |
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 think we need to change orgPermission
to something more meaningful... maybe requiredPermission
?
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 agree
graphql/schema.graphql
Outdated
@@ -11,6 +11,7 @@ type Query { | |||
sortedBy: String = "created" | |||
): OrganizationRequestResult | |||
@cacheControl(scope: PRIVATE, maxAge: SHORT) | |||
@validateAdminUserAccess(orgPermission: "buyer_organization_view") |
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.
for now lets not add the directive to APIs that don't have it. Lets just add the parameter to APIs that already have either validateAdminUserAccess
or validateStoreUserAccess
.
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.
but if we don't add in getOrganizations for example the permissions will pass. Will it really be like that?
node/clients/LMClient.ts
Outdated
userEmail: string, | ||
resourceCode: string | ||
) => { | ||
const productCode = B2B_LM_PRODUCT_CODE // resource name on lincense manager = B2B |
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 actual name of this resource is Buyer Organization
actually (I gave you the wrong info before). So lets update the comment here
const productCode = B2B_LM_PRODUCT_CODE // resource name on lincense manager = B2B | |
const productCode = B2B_LM_PRODUCT_CODE // resource name on lincense manager = Buyer Organization |
node/clients/LMClient.ts
Outdated
@@ -24,6 +26,24 @@ export default class LMClient extends ExternalClient { | |||
}) | |||
} | |||
|
|||
public checkUserAdminPermission = async ( |
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.
public checkUserAdminPermission = async ( | |
public checkAdminUserRequiredPermission = async ( |
node/clients/LMClient.ts
Outdated
const checkOrgPermission = await this.get<boolean>( | ||
`/api/license-manager/pvt/accounts/${account}/products/${productCode}/logins/${userEmail}/resources/${resourceCode}/granted` | ||
) | ||
|
||
if (!checkOrgPermission) { | ||
throw new ForbiddenError('Unauthorized Access') | ||
} | ||
|
||
return checkOrgPermission |
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 checkOrgPermission = await this.get<boolean>( | |
`/api/license-manager/pvt/accounts/${account}/products/${productCode}/logins/${userEmail}/resources/${resourceCode}/granted` | |
) | |
if (!checkOrgPermission) { | |
throw new ForbiddenError('Unauthorized Access') | |
} | |
return checkOrgPermission | |
const checkRequiredPermission = await this.get<boolean>( | |
`/api/license-manager/pvt/accounts/${account}/products/${productCode}/logins/${userEmail}/resources/${resourceCode}/granted` | |
) | |
if (!checkRequiredPermission) { | |
throw new ForbiddenError('Unauthorized Access') | |
} | |
return checkRequiredPermission |
node/resolvers/directives/helper.ts
Outdated
@@ -2,7 +2,8 @@ import { isUserPartOfBuyerOrg } from '../Queries/Users' | |||
|
|||
export const validateAdminToken = async ( | |||
context: Context, | |||
adminUserAuthToken: string | |||
adminUserAuthToken: string, | |||
orgPermission?: 'buyer_organization_edit' | 'buyer_organization_view' |
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.
(lets change orgPermission
everywhere to requiredPermission
orgPermission?: 'buyer_organization_edit' | 'buyer_organization_view' | |
requiredPermission?: 'buyer_organization_edit' | 'buyer_organization_view' |
node/utils/constants.ts
Outdated
@@ -14,3 +14,5 @@ export const ORGANIZATION_REQUEST_STATUSES = { | |||
export const MARKETING_TAGS = { | |||
VBASE_BUCKET: 'b2b_marketing_tags', | |||
} | |||
|
|||
export const B2B_LM_PRODUCT_CODE = '97' // resource name on lincense manager = B2B |
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 comment here as well.
What problem is this solving?
Currently, when accessing organization management, any user can make any change. The goal is to implement a minimum control of viewing/changing.
VTEX already has two features in the license manager that are not used, called:
Buyer Organization View
Buyer Organization Edit
These two features could be used to facilitate the requested changes.
How to test it?
query { getOrganizations(search: "giu") { data { id name } pagination { page pageSize total } } }
mutation { createOrganization(input: { name: "Nome da Organização" tradeName: "Nome Comercial" b2bCustomerAdmin: { email: "admin@exemplo.com" firstName: "Nome" lastName: "Sobrenome" } defaultCostCenter: { name: "Centro de Custo Padrão" address: { country: "BRA" postalCode: "12345-678" city: "Cidade" state: "Estado" street: "Rua Exemplo" number: "123" } } }) { id status message } }
Workspace
Screenshots or example usage:
Describe alternatives you've considered, if any.
Related to / Depends on
How does this PR make you feel?
![https://giphy.com/gifs/Friends-season-5-episode-111-the-one-where-everybody-finds-out-YnBntKOgnUSBkV7bQH](put .gif link here - can be found under "advanced" on giphy)