Ensure _acl is updated when _rperm and _wperm updated #2701
Conversation
ghost
added
the
| @@ -330,18 +330,11 @@ const parseObjectToMongoObjectForCreate = (className, restCreate, schema) => { | |||
| // Main exposed method to help update old objects. | |||
| const transformUpdate = (className, restUpdate, parseFormatSchema) => { | |||
| let mongoUpdate = {}; | |||
| let acl = addLegacyACL(restUpdate)._acl; | |||
There was a problem hiding this comment.
._acl is not the full acl but just the legacy part, so none of the properties below exist as we are trying to reference acl._acl._acl for example.
$set._rperm and _wperm are provided via transformKeyValueForUpdate on line 340 so all that needs to be added here is the legacy property _acl
There was a problem hiding this comment.
that will render the following lines incorrect...
This:
mongoUpdate.$set._acl = acl._acl;
Should be:
mongoUpdate.$set._acl = acl;
Without a change to: https://github.com/ParsePlatform/parse-server/pull/2701/files#diff-82f0219ca721372e8f2bdbc45c83bbdcR333
There was a problem hiding this comment.
If you are suggesting not changing line 333 then none of the if clauses will execute on lines 336, 339, 342
The object returned by addLegacyACL has the format:
{
_acl: {
id: w,
id: r},
_rperm: [],
_wperm: []
}
There was a problem hiding this comment.
So the whole thing as a no-op in the end and now it's breaking all the tests...
There was a problem hiding this comment.
Sorry I thought it was an intermittent test failure.
The tests were failing when restUpdate contained non-acl updates, e.g. { _failed_login_count: { __op: 'Increment', amount: 1 } }.
I've restored the original code for line 334 which fixes the issue.
|
Fixes #2628 |
| mongoUpdate.$set._wperm = acl._wperm; | ||
| } | ||
| if (acl._acl) { | ||
| mongoUpdate.$set._acl = acl._acl; |
There was a problem hiding this comment.
Actually, the bug should be fixed by just setting acl Instead of acl._acl
There was a problem hiding this comment.
An alternative fix would be to remove the ._acl on line 333 and keep the original code, i.e.
let acl = addLegacyACL(restUpdate);
if (acl) {
mongoUpdate.$set = {};
if (acl._rperm) {
mongoUpdate.$set._rperm = acl._rperm;
}
if (acl._wperm) {
mongoUpdate.$set._wperm = acl._wperm;
}
if (acl._acl) {
mongoUpdate.$set._acl = acl._acl;
}
}
I felt that was pointless though as _rperm and _wperm are set elsewhere and thought there must have been a reason line 333 was changed in the first place to reference _acl
There was a problem hiding this comment.
If it was written like that, there must be a reason, that is not obvious. The acl._acl is clearly an error from the original commiter and not his intention. I'd like to keep the code as it is, as it's working for the other use cases. You don't really know what you maybe breaking by removing it
Current coverage is 92.25% (diff: 100%)@@ master #2701 diff @@
==========================================
Files 102 102
Lines 12446 12446
Methods 1559 1559
Messages 0 0
Branches 2039 2039
==========================================
+ Hits 11478 11482 +4
+ Misses 968 964 -4
Partials 0 0
|
|
@steven-supersolid updated the pull request - view changes |
|
@steven-supersolid updated the pull request - view changes |
This should fix the bug introduced here: https://github.com/ParsePlatform/parse-server/pull/2021/files#diff-82f0219ca721372e8f2bdbc45c83bbdcR303
Updated existing test and added new test.
@drew-gross can you take a look please?