Skip to content
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

fix(admin-ui): Fix tax rate permissions so product variants do not need access to customer groups #1274

Conversation

vrosa
Copy link
Contributor

@vrosa vrosa commented Dec 9, 2021

As per discussion on Slack:

TaxRateEntityResolver.customerGroup() kicks in to resolve the TaxRate.customerGroup field and requires either ReadCustomer or ReadCustomerGroup permissions. I found it curious that a field resolver needs additional permissions on top of what its parent query (TaxRateResolver.taxRates()) does.
Is my assumption to just rely on ReadProduct for a full read-only view of products associated to a channel correct or are these added permissions really necessary?

I think the real issue here is that there is actually no need to even access the TaxRate.customerGroup field when listing ProductVariants. I just re-used an existing graphql fragment which is used in the main TaxRate list view. So the solution would be to alter the query to omit that field for the ProductVariants view.

I've leveraged the same convention found in other places, e.g. GetProductSimple and GetProductVariantListSimple, and created a new query for tax rates that returns less fields - in this case, customerGroup - and uses the Simple suffix. I couldn't find other examples in the codebases so please let me know if I should take a different approach.

@michaelbromley michaelbromley merged commit 0a49fea into vendure-ecommerce:master Dec 9, 2021
@michaelbromley
Copy link
Member

Yes this is good. Good job for taking the time to get familiar with the code base and discern an existing convention 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants