Access Control does not work with update many mutation. Properties "itemId" and "itemIds" are not returning with correct data on update many mutation.

[chtonal]

Bug report

Access Control properties "itemId" and "itemIds" are not returning with correct data on update many mutation.

Describe the bug

When using update many on list view in Admin UI, mutation "updateUsers" is called with the following data:

[{
  authentication: {
    item: {
      name: 'Operator',
      surname: 'RRR',
      email: 'operator@keystonejs.com',
      password: '****************************',
      isAdmin: true,
      updatedAt_utc: undefined,
      updatedAt_offset: undefined,
      createdAt_utc: undefined,
      createdAt_offset: undefined,
      updatedBy: 5ea6a4c6c091e737c44b85d3,
      createdBy: 5ea6a4c6c091e737c44b85d3,
      __v: 0,
      id: '5eaacb89cdeef94438cb74ec',
      updatedAt: '2020-05-04T17:02:29.569Z',
      createdAt: '2020-04-30T12:58:49.853Z'
    },
    listKey: 'User'
  },
  listKey: 'User',
  operation: 'update',
  originalInput: [
    { id: '5ea7211ae6aabe1a0c13bc09', data: [Object] },
    { id: '5eb015509d857c04e47b2756', data: [Object] },
    { id: '5eaacb89cdeef94438cb74ec', data: [Object] },
    { id: '5eaffd2d3a9e872a703410d2', data: [Object] }
  ],
  gqlName: 'updateUsers',
  itemId: [
    '5ea7211ae6aabe1a0c13bc09',
    '5eb015509d857c04e47b2756',
    '5eaacb89cdeef94438cb74ec',
    '5eaffd2d3a9e872a703410d2'
  ],
  itemIds: undefined
}]

Here, "itemId" must be the current updated item's id in order to check it in access control methods and "itemIds" must be the id's of the list that requested for update. But current implementation of “itemIds” returns with “undefined”.

In my access control methods, updating item on its view (mutation updateUser) give me correct result, but updating whole items (mutation updateUsers) in list view in Admin UI gives wrong result so that access control mechanism does not work correctly.

In order to avoid this issue “itemId” must return current item's id in order to check which item is in progress for access control and “itemIds” must be replaced with the current result of itemId which is:

  itemId: [
    '5ea7211ae6aabe1a0c13bc09',
    '5eb015509d857c04e47b2756',
    '5eaacb89cdeef94438cb74ec',
    '5eaffd2d3a9e872a703410d2'
  ],

Thanks,

[Vultraz]

To clarify, you're using the Update popout to update several items at once, right? Not one single item in details view?

[chtonal]

@Vultraz Yes right. I am using update popout to update several items at once. Single item works fine.

With this issue, unauthorized user can make itself authorized or can update unauthorized fields.

[Vultraz]

Ok, I think I see the issue. About your access control function, though. Does it rely on itemId being a singular ID? By the spec, itemIds (plural) is supposed to be set in update-many and delete-many ops. It's correctly set in delete-many, but not update-many (this issue). I can fix that, but itemId (singular) would be undefined in that case. Would your setup work with that?

EDIT: this is what a fixed version would look like:

  itemId: undefined,
  itemIds: [ '5eb18978706f667c88b678c6', '5eb18981706f667c88b678c7' ]

[chtonal]

Ok, I think I see the issue. About your access control function, though. Does it rely on itemId being a singular ID? By the spec, itemIds (plural) is supposed to be set in update-many and delete-many ops. It's correctly set in delete-many, but not update-many (this issue). I can fix that, but itemId (singular) would be undefined in that case. Would your setup work with that?

EDIT: this is what a fixed version would look like:

  itemId: undefined,
  itemIds: [ '5eb18978706f667c88b678c6', '5eb18981706f667c88b678c7' ]

Actually, itemId is needed for which item’s access control check is in progress. If we define itemId as undefined, we can just check access control in mutation level. I mean, if access control returns false, updateUsers mutation is not going to update any item in that list, even several items access control check returns true. To avoid, we have to know which item update is in progress.

I dont know if i expressed the issue well? I will provide a detailed code as soon as possible.

[Vultraz]

Were you using field or list access control?

EDIT: dug deeper, found even more issues and updated the PR. I think it should fix your issue now 😃 itemIds (plural) will be set in both list and field-level access controls. itemId (singular) will be undefined in list-level controls (since that's only called once), but will be set to the item currently being checked in field-level controls (since those are called multiple times.

[chtonal]

@Vultraz Suppose i have 4 different user groups: [Superadmin, Admin, Operator, Enduser]. Operator can not change the userType to Superadmin or to Admin. Besides, Operator can only modify Enduser or its own item. Following ImperativeAccess control code is for the clarification;

access: {
...
update: auth => {
...
if(auth.authentication.item.userType === 'Operator') {
	if(!includes(['Superadmin', 'Admin'], auth.originalInput['userType'])) {
		if(includes([undefined, 'Operator'], auth.originalInput['userType'])) {
			// Big problem starts here
			// we need to check if operator is updating its own item
			// however there is no 'id' to check here
			// we have originalInput itemId and itemIds to compare if this item belongs to operator
			let tmpResult = false;
			if( isArray(auth.originalInput) ) { // means that its updateUsers mutation
				// In the following equation because we dont know which item is currently updated, returning value is the value for
				// whole the items in the list of updateUsers mutation. If this is false, false for all the items in the list
				// if it is true, true for every items in the list. It does not give us correct result
				tmpResult = find(auth.originalInput, {'id': auth.authentication.item.id}).data['id'] === auth.authentication.item.id;
				// OR
				tmpResult = itemId === auth.authentication.item.id; // This is the clearest way to implement but ***we dont know it***
				// it gives us correct result because itemId is the currently updated items id.
				// However, itemId is not known in updateUsers mutation.
			} else {
				// it is updateUser mutation that has single item
				if(auth.originalInput['id']) {
					tmpResult = isEqual(auth.originalInput['id'], auth.authentication.item.id);
				} else {
					// Here itemId works fine because it refers to updateUser mutation
					// updateUser mutation returns with the itemId which is the item that is in update progress.
					tmpResult = isEqual(itemId, auth.authentication.item.id);
				}
			}
			return tmpResult;
		} else if ( includes([undefined, 'Enduser'], auth.originalInput['userType']) ) {
			return true;
		} else {
			return false;
		}
	} else {
		return false;
	}
}
}
}

Field-level control is fine but in some cases list-level control is mandatory. Somehow, if we are not able to find the id of currently updated item in updateUsers mutation we have to move some control statements to field-level.

[Vultraz]

Well, no, you can't get the ID of the currently-checked item at list level because list checks are only called once. It's checking access for the full list. If you want to check with specific IDs, you'll want field-level. My PR ensures the field-level hooks will have the necessary info.

[chtonal]

Thank you for finding the solution quickly. I am closing this issue.

Well, no, you can't get the ID of the currently-checked item at list level because list checks are only called once. It's checking access for the full list. If you want to check with specific IDs, you'll want field-level. My PR ensures the field-level hooks will have the necessary info.

[Vultraz]

My PR was merged. The changes will be in the next release.