Allow read-access to protectedFields based on user for custom classes

[Dobbias]

Fixes the inconsistency regarding ACLs, protectedFields and custom classes mentioned in #5884
The fix however was different to the one suggested in the issue.

Users given read access in an ACL now can read the protected fields of the object aswell.
Similar functionality was available in the _User class by allowing the user access to the protectedFields based on the object id and the currently logged-in user's id.

This pull request adds similar functionality to all custom classes. The users added to an object's ACL with read access are allowed to read the protectedFields.

[davimacedo]

@dplewis @acinader the idea here is to allow users with explicit ACL access to an object to see the protected fields. It makes sense to me. And I've also seen that it was the original idea, but never implemented. Thoughts?

[davimacedo]

Why don't you just go through the object ACLs and check if there is a rule with the user id and read true?

[Dobbias]

As far as I understand the aclGroup contains all roles associated with the current user and the user id. aclGroup is not the object's ACL. So I first use the aclGroup to retrieve the current user's id. Then I use the actual object to check if the user id has read permission using getReadAccess.

If there is a better way to retrieve the current user's id (instead of using the acl group) I'll gladly use it.

The aclGroup approach is used in the addPointerPermissions method too:

parse-server/src/Controllers/DatabaseController.js

Lines 1467 to 1482 in ef14ca5

	const userACL = aclGroup.filter(acl => {
	return acl.indexOf('role:') != 0 && acl != '*';
	});
	// the ACL should have exactly 1 user
	if (perms && perms[field] && perms[field].length > 0) {
	// No user set return undefined
	// If the length is > 1, that means we didn't de-dupe users correctly
	if (userACL.length != 1) {
	return;
	}
	const userId = userACL[0];
	const userPointer = {
	__type: 'Pointer',
	className: '_User',
	objectId: userId,
	};

Line 202 avoids the whole retrieval of the user id if there are no protected fields to remove.

[acinader]

Just now really thinking this through for the first time.

If we go this proposed route, would we still be able to do something like: give read access to the roles: 'admin' and 'agent' via an ACL and still hide some fields from 'agent' that we return for 'admin'?

That seems like the kind of granularity we want.

If we wanted to use existing functionality and features to solve the problem, I'd think we'd try and use roles and/or a 'clp like pointer permission' in the protected fields declaration to achieve a solution for #5884?

[Dobbias]

Just now really thinking this through for the first time.

If we go this proposed route, would we still be able to do something like: give read access to the roles: 'admin' and 'agent' via an ACL and still hide some fields from 'agent' that we return for 'admin'?

That seems like the kind of granularity we want.

If we wanted to use existing functionality and features to solve the problem, I'd think we'd try and use roles and/or a 'clp like pointer permission' in the protected fields declaration to achieve a solution for #5884?

@acinader Pull request 5301 also added the mentioned role functionality for protectedFields. It is still valid after the changes of this pull request. But if the roles from the object ACL don't allow an user to read the protectedFields and the user's id is explicitly added to the ACL he nevertheless will be able to retrieve the protectedFields.

While roles can be used to access protectedFields, 'clp like pointer permission' cannot.

Let's imagine a setup where the current role based access to the protected data wouldn't work and the changes in this PR would allow the imagined setup:

1. A book class that is publicly readable
2. We want the author user to read hidden fields on the book object
3. We don't want to read the public or any other author to read the hidden fields

With this setup we could allow access to the fields using an "Author" role but this would allow other authors to read the data too.
With the proposed changes we could add the author's user id to the object's ACL to grant read access on the hidden fields.

[acinader]

@Dobbias, your example with a book and authors is a good one to focus on.

In your case, I'd expect we'd want an 'author' column (actually, we'd want an 'authors' column, but more on that below.)

So if we extend the 'pointer permissions' from CLPs to protected fields, our protectedFields json would then look something like:

protectedFields: {
    Book: { '*': ['profit', 'contract'], 'readUserFields:author': [] },
}

I think this makes it explicit and contained to a single location.

As for 'author' vs. 'authors'...

I made a unit test just now to see if CLP's would support an array of Pointer<Parse.User> but it is not supported. I think it should be and your 'Book' example makes clear why.

Wadda ya think?

[Dobbias]

@acinader I do like your approach a lot more than the one I added in this PR. I think it is more explicit than just adding user ids to the ACL. And it is located next to the protectedFields' definition in code (from an users perspective).

[Dobbias]

I'll close this PR since a better fix has been proposed and should be implemented.