From ef1c9cba580d7a231951feb47b23ecd63a31b0f9 Mon Sep 17 00:00:00 2001 From: Wojciech Mista Date: Mon, 5 Aug 2024 13:34:10 +0200 Subject: [PATCH] Fix required permissions in subscription query editor (#5064) * wrong permissions displayed fix * fix mock * changesets * show permission origin and optional * add test * json messages * small fixes * qa fixes * strict mode * cr fixes --- .changeset/shaggy-spoons-explode.md | 5 + locale/defaultMessages.json | 12 + .../PermissionAlert/PermissionAlert.tsx | 59 +++- .../components/PermissionAlert/utils.test.ts | 21 +- .../components/PermissionAlert/utils.ts | 268 ++++++++++-------- testUtils/mocks/introspection.ts | 35 +-- 6 files changed, 249 insertions(+), 151 deletions(-) create mode 100644 .changeset/shaggy-spoons-explode.md diff --git a/.changeset/shaggy-spoons-explode.md b/.changeset/shaggy-spoons-explode.md new file mode 100644 index 00000000000..134ea3c1d75 --- /dev/null +++ b/.changeset/shaggy-spoons-explode.md @@ -0,0 +1,5 @@ +--- +"saleor-dashboard": patch +--- + +Subscription query editor no longer shows incorrect required permissions for inserted query. diff --git a/locale/defaultMessages.json b/locale/defaultMessages.json index 5bc6c83dfdd..86568505590 100644 --- a/locale/defaultMessages.json +++ b/locale/defaultMessages.json @@ -422,6 +422,10 @@ "context": "column header", "string": "Title" }, + "0YjGFG": { + "context": "alert message", + "string": "For subscription" + }, "0a0fLZ": { "context": "countries list menu label when no countries are assigned", "string": "There are no countries assigned" @@ -2064,6 +2068,10 @@ "ByYtFB": { "string": "Invalid or expired token. Please check your token in URL" }, + "C+WD8j": { + "context": "alert message", + "string": "all of" + }, "C035fF": { "context": "grant refund, refund card explanation next to submit button", "string": "Funds will be returned in a separate step" @@ -3044,6 +3052,10 @@ "context": "is filter range or value", "string": "equal to" }, + "I/y4IU": { + "context": "alert message", + "string": "one of" + }, "I1Mz7h": { "string": "Manage Collection Channel Availability" }, diff --git a/src/custom-apps/components/PermissionAlert/PermissionAlert.tsx b/src/custom-apps/components/PermissionAlert/PermissionAlert.tsx index dd69ec9a042..d83f7a09be4 100644 --- a/src/custom-apps/components/PermissionAlert/PermissionAlert.tsx +++ b/src/custom-apps/components/PermissionAlert/PermissionAlert.tsx @@ -1,10 +1,11 @@ -import { useQuery } from "@apollo/client"; +import { gql, useQuery } from "@apollo/client"; import { Alert } from "@saleor/macaw-ui"; import { Box, Chip, Text } from "@saleor/macaw-ui-next"; +import { getIntrospectionQuery } from "graphql"; import React from "react"; import { useIntl } from "react-intl"; -import { buildPermissionMap, getPermissions, IntrospectionQuery } from "./utils"; +import { getPermissions } from "./utils"; export interface PermissionAlertProps { query: string; @@ -12,16 +13,16 @@ export interface PermissionAlertProps { const PermissionAlert: React.FC = ({ query }) => { const intl = useIntl(); - const { data } = useQuery(IntrospectionQuery, { + const introQuery = getIntrospectionQuery(); + const { data } = useQuery(gql(introQuery), { fetchPolicy: "network-only", }); - const elements = data?.__schema?.types || []; - const permissionMapping = buildPermissionMap(elements); - const permissions = getPermissions(query, permissionMapping); + const permissionInfo = getPermissions(query, data); + const hasPermissions = permissionInfo && Object.entries(permissionInfo).length > 0; return (
- {permissions.length > 0 && ( + {hasPermissions && ( = ({ query }) => { close={false} className="remove-icon-background" > - - {permissions.map(permission => ( - - {permission} - - ))} + + {Object.entries(permissionInfo).map( + ([subscription, { isOneOfRequired, permissions }]) => ( + + + {intl.formatMessage({ + id: "0YjGFG", + defaultMessage: "For subscription", + description: "alert message", + })} + + + {subscription} + + + {isOneOfRequired + ? intl.formatMessage({ + id: "I/y4IU", + defaultMessage: "one of", + description: "alert message", + }) + : intl.formatMessage({ + defaultMessage: "all of", + id: "C+WD8j", + description: "alert message", + })} + + {permissions.map(permission => ( + + {permission} + + ))} + + ), + )} )} diff --git a/src/custom-apps/components/PermissionAlert/utils.test.ts b/src/custom-apps/components/PermissionAlert/utils.test.ts index 325b2aeb20a..0d9321779b4 100644 --- a/src/custom-apps/components/PermissionAlert/utils.test.ts +++ b/src/custom-apps/components/PermissionAlert/utils.test.ts @@ -4,10 +4,10 @@ describe("Permission Parsing", () => { it("should extract permissions from the meta `description` if available", () => { // Arrange // -> Order.invoices - // https://docs.saleor.io/docs/3.x/api-reference/objects/order + // https://docs.saleor.io/api-reference/orders/objects/order const description = `List of order invoices. Can be fetched for orders created in Saleor 3.2 and later, for other orders requires one of the following permissions: MANAGE_ORDERS, OWNER.`; // Act - const permissions = extractPermissions(description); + const { permissions } = extractPermissions(description); // Assert expect(permissions).toHaveLength(2); @@ -16,12 +16,25 @@ describe("Permission Parsing", () => { it("should return empty list if the `description` doesn't mention permissions", () => { // Arrange // -> Order.number - // https://docs.saleor.io/docs/3.x/api-reference/objects/order + // https://docs.saleor.io/api-reference/orders/objects/order const description = `User-friendly number of an order.`; // Act - const permissions = extractPermissions(description); + const { permissions } = extractPermissions(description); // Assert expect(permissions).toHaveLength(0); }); + it("should correctly asses if the permissions are optional", () => { + // Arrange + // -> Order.invoices + // https://docs.saleor.io/api-reference/orders/objects/order + const description = `List of order invoices. Can be fetched for orders created in Saleor 3.2 and later, for other orders requires one of the following permissions: MANAGE_ORDERS, OWNER.`; + // Act + const { permissions, isOneOfRequired } = extractPermissions(description); + + // Assert + expect(isOneOfRequired).toBe(true); + expect(permissions).toHaveLength(2); + expect(permissions[1]).toEqual("OWNER"); + }); }); diff --git a/src/custom-apps/components/PermissionAlert/utils.ts b/src/custom-apps/components/PermissionAlert/utils.ts index 5480e77f570..b2cd862d70e 100644 --- a/src/custom-apps/components/PermissionAlert/utils.ts +++ b/src/custom-apps/components/PermissionAlert/utils.ts @@ -1,47 +1,62 @@ -// @ts-strict-ignore -import { gql } from "@apollo/client"; -import { FieldNode, parse, SelectionNode, visit } from "graphql"; - -interface IntrospectionNode { - kind: string; - name: string; - description: string; - fields: Array<{ - name: string; - description: string; - }>; -} +import { + buildClientSchema, + DocumentNode, + GraphQLField, + GraphQLList, + GraphQLNonNull, + GraphQLObjectType, + GraphQLSchema, + GraphQLType, + IntrospectionQuery, + Location, + parse, + Token, + visit, +} from "graphql"; +import { TypeMap } from "graphql/type/schema"; + +type SubscriptionQueryPermission = Record< + string, + { + isOneOfRequired: boolean; + permissions: string[]; + } +>; -interface PermissionChildNode { - permissions: string[]; - children: string[]; -} +const getStartToken = (documentNode: DocumentNode) => { + const loc = documentNode.loc as Location | undefined; -interface PermissionNode { - permissions: string[]; - children: Record; -} + return loc?.startToken; +}; -type PermissionMap = Record; - -// cannot be in `queries.ts` as codegen cannot handle `__schema` -export const IntrospectionQuery = gql` - query PermissionIntrospection { - __schema { - types { - kind - name - description - fields(includeDeprecated: false) { - name - description - } - } - } +type FlattenedTokenArray = (string | undefined)[]; + +const toFlattenedTokenArray = ( + token: Token | undefined, + tokens: FlattenedTokenArray = [], +): FlattenedTokenArray => { + if (token && token.next) { + tokens.push(token.value); + + return toFlattenedTokenArray(token.next, tokens); } -`; -const uniq = (value: T, index: number, self: T[]) => self.indexOf(value) === index; + return tokens; +}; + +const flattenedTokenArray = (token: Token | undefined, tokens: string[] = []): string[] => { + const flattened = toFlattenedTokenArray(token, tokens); + + return flattened.filter(Boolean) as string[]; +}; + +const unwrapType = (type: GraphQLType): GraphQLType => { + if (type instanceof GraphQLNonNull || type instanceof GraphQLList) { + return unwrapType(type.ofType); + } + + return type; +}; // Right now, permissions are appended at the end of `description` // for each field in the result of the introspection query. The format @@ -50,92 +65,121 @@ const uniq = (value: T, index: number, self: T[]) => self.indexOf(value) === // separated and independant, yet easily accessible export const extractPermissions = (description?: string) => { const match = (description || "").match(/following permissions(.*): (.*?)\./); + // We're assuming that if there is no "one of" then all permissions are required + const isOneOfRequired = (description || "").includes("one of"); const permissions = match ? match[2].split(",") : []; - return permissions; + return { + isOneOfRequired, + permissions: permissions.map(permission => permission.trim()), + }; }; -export const getPermissions = (query: string, permissionMapping: PermissionMap) => { - const cursors = extractCursorsFromQuery(query); - - return cursors.map(findPermission(permissionMapping)).flat().filter(uniq); +export const getPermissions = ( + query: string, + introspectionQuery: IntrospectionQuery, +): SubscriptionQueryPermission => { + return extractPermissionsFromQuery(query, introspectionQuery); }; -export const buildPermissionMap = (elements: IntrospectionNode[]): PermissionMap => - elements - .filter(({ kind }) => kind === "OBJECT") - .filter(({ name }) => !/(Created|Create|Delete|Deleted|Update|Updated)$/.test(name)) - .reduce((saved, { name, description, fields }) => { - const permissions = extractPermissions(description); - const children = fields.reduce((prev, { name, description }) => { - const permissions = extractPermissions(description); - - return { - ...prev, - [name.toLowerCase()]: { permissions }, - }; - }, {}); - - return { - ...saved, - [name.toLowerCase()]: { - permissions, - children, - }, - }; - }, {}); - -const byKind = (name: string) => (element: SelectionNode) => element.kind === name; -const extractValue = (element: FieldNode) => element.name.value; -const isNotEvent = (value: string) => value !== "event"; - -export const extractCursorsFromQuery = (query: string) => { - const cursors: string[][] = []; +const extractSubscriptions = ( + subscriptions: GraphQLObjectType[], + tokens: string[], +): GraphQLObjectType[] => + tokens.reduce((acc, token) => { + const subscription = subscriptions.find(({ name }) => name === token); - try { - const ast = parse(query); + if (subscription) { + return [...acc, subscription]; + } + + return acc as GraphQLObjectType[]; + }, [] as GraphQLObjectType[]); - visit(ast, { - Field(node, _key, _parent, _path, ancestors) { - if (node.name.value !== "__typename") { - const cursor = ancestors.filter(byKind("Field")).map(extractValue).filter(isNotEvent); +const getSubscriptions = (typeMap: TypeMap): GraphQLObjectType[] => + Object.keys(typeMap).reduce((acc, key) => { + const type = typeMap[key] as GraphQLObjectType; - if (cursor.length > 0) { - cursors.push([...cursor, node.name.value]); + if (type instanceof GraphQLObjectType) { + const interfaces = type.getInterfaces(); + const hasEvent = interfaces.some(({ name }) => name === "Event"); + + if (hasEvent) { + return [...acc, type]; + } + } + + return acc; + }, [] as GraphQLObjectType[]); + +function getDescriptionsFromQuery(query: string, schema: GraphQLSchema): { [key: string]: string } { + const descriptions: { [key: string]: string } = {}; + const ast = parse(query); + const startToken = getStartToken(ast); + const tree = flattenedTokenArray(startToken, []); + const subscriptions = getSubscriptions(schema.getTypeMap()); + const subscriptionsFromQuery = extractSubscriptions(subscriptions, tree); + + visit(ast, { + Field(node, _key, _parent, _path, ancestors) { + for (const _type in subscriptionsFromQuery) { + const fieldPath = ancestors + .filter((ancestor: any) => ancestor.kind === "Field") + .map((ancestor: any) => ancestor.name.value) + .concat(node.name.value); + let type: GraphQLObjectType | undefined = subscriptionsFromQuery[_type]; + + for (const fieldName of fieldPath.slice(1, -1)) { + if (type) { + const field: GraphQLField = ( + unwrapType(type) as GraphQLObjectType + ).getFields()[fieldName]; + + // Some types can have multiple nested fields eg. lists + type = field && field.type ? (unwrapType(field.type) as GraphQLObjectType) : undefined; + } + } + + if (type) { + const field = type.getFields()[node.name.value]; + + if (field) { + descriptions[fieldPath.join(".")] = field.description || "No description available"; } } - }, - }); + } + }, + }); + + return descriptions; +} + +const extractSubscriptionQueryPermissions = (descriptions: { [key: string]: string }) => { + return Object.keys(descriptions).reduce((acc, key) => { + const { isOneOfRequired, permissions } = extractPermissions(descriptions[key]); + + if (permissions.length > 0) { + acc[key] = { isOneOfRequired, permissions }; + } + + return acc; + }, {} as SubscriptionQueryPermission); +}; + +const extractPermissionsFromQuery = ( + query: string, + introspectionQuery: IntrospectionQuery, +): SubscriptionQueryPermission => { + let permissions: SubscriptionQueryPermission = {}; + + try { + const schema = buildClientSchema(introspectionQuery); + const descriptions = getDescriptionsFromQuery(query, schema); + + permissions = extractSubscriptionQueryPermissions(descriptions); } catch (error) { // explicit silent } - return cursors; -}; - -const groupBy = (list: T[], groupSize = 2) => - list.slice(groupSize - 1).map((_, index) => list.slice(index, index + groupSize)); - -// Permission Map is a tree like nested structure. As we want to -// visit each level, we split the cursor provided by the user -// into chunks (groups) to check if there are permissions for -// each element between root and leafs -// e.g. -// ['query', 'order', 'invoices', 'name'] -// becomes -// [['query', 'order'], ['order', 'invoices'], ['invoices', 'name']] -// so we can check first ir `order` contains permission, then `invoices` -// and then `name` -export const findPermission = (permissions: PermissionMap) => (cursor: string[]) => { - const groups = groupBy(["query", ...cursor]); - - return groups.reduce( - (saved, [parent, child]) => [ - ...saved, - ...(permissions[parent] && permissions[parent].children[child] - ? permissions[parent].children[child].permissions - : []), - ], - [], - ); + return permissions; }; diff --git a/testUtils/mocks/introspection.ts b/testUtils/mocks/introspection.ts index 5c26986de7a..acdc966ba31 100644 --- a/testUtils/mocks/introspection.ts +++ b/testUtils/mocks/introspection.ts @@ -1,10 +1,11 @@ +import { gql } from "@apollo/client"; import { MockedResponse } from "@apollo/client/testing"; -import { IntrospectionQuery } from "@dashboard/custom-apps/components/PermissionAlert/utils"; +import { getIntrospectionQuery } from "graphql"; export const introspectionMocks: MockedResponse[] = [ { request: { - query: IntrospectionQuery, + query: gql(getIntrospectionQuery()), variables: {}, }, result: { @@ -50,8 +51,7 @@ export const introspectionMocks: MockedResponse[] = [ { kind: "OBJECT", name: "Product", - description: - "Represents an individual item for sale in the storefront.", + description: "Represents an individual item for sale in the storefront.", fields: [ { name: "id", @@ -152,8 +152,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "isAvailable", - description: - "Whether the product is in stock and visible or not.", + description: "Whether the product is in stock and visible or not.", }, { name: "attribute", @@ -189,8 +188,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "translation", - description: - "Returns translated product fields for the given language code.", + description: "Returns translated product fields for the given language code.", }, { name: "availableForPurchaseAt", @@ -207,8 +205,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "externalReference", - description: - "External ID of this product. \n\nAdded in Saleor 3.10.", + description: "External ID of this product. \n\nAdded in Saleor 3.10.", }, ], }, @@ -392,8 +389,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "translation", - description: - "Returns translated sale fields for the given language code.", + description: "Returns translated sale fields for the given language code.", }, { name: "channelListings", @@ -531,8 +527,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "original", - description: - "The ID of the order that was the base for this order.", + description: "The ID of the order that was the base for this order.", }, { name: "origin", @@ -658,8 +653,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "totalBalance", - description: - "The difference between the paid and the order total amount.", + description: "The difference between the paid and the order total amount.", }, { name: "userEmail", @@ -685,8 +679,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "errors", - description: - "List of errors that occurred during order validation.", + description: "List of errors that occurred during order validation.", }, { name: "displayGrossPrices", @@ -695,8 +688,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "externalReference", - description: - "External ID of this order. \n\nAdded in Saleor 3.10.", + description: "External ID of this order. \n\nAdded in Saleor 3.10.", }, { name: "checkoutId", @@ -780,8 +772,7 @@ export const introspectionMocks: MockedResponse[] = [ }, { name: "order", - description: - "Order related to the invoice.\n\nAdded in Saleor 3.10.", + description: "Order related to the invoice.\n\nAdded in Saleor 3.10.", }, ], },