refactor(query): deprecate explain without master key #7521

mstniy · 2021-08-24T17:58:28Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

Currently, any user is able to run a query with the explain parameter and obtain the raw result returned by MongoDB. This discloses too much information to the clients, nor is it of great utility to them.

Related issue: #7519

Approach

rest.js now prints a deprecation warning if a non-master user tries to run an explain query. Client-facing behaviour is intact.

TODOs before merging

Add test cases
Add entry to changelog
Add changes to documentation (guides, repository pages, in-code descriptions)
Add security check
Add new Parse Error codes to Parse JS SDK

…on warning Added pending tests that ensure non-master users cannot run explain queries Added the change to the CHANGELOG

codecov · 2021-08-24T18:04:33Z

Codecov Report

Merging #7521 (3577c96) into master (1e0d408) will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           Coverage Diff           @@
##           master    #7521   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files         181      181           
  Lines       13273    13278    +5     
=======================================
+ Hits        12467    12472    +5     
  Misses        806      806

Impacted Files	Coverage Δ
src/rest.js	`97.84% <80.00%> (-1.02%)`	⬇️
src/batch.js	`91.37% <0.00%> (-1.73%)`	⬇️
src/RestWrite.js	`93.78% <0.00%> (-0.16%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`93.02% <0.00%> (+0.65%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e0d408...3577c96. Read the comment docs.

DEPRECATIONS.md

mtrezza · 2021-08-26T17:39:36Z

@mstniy Please feel free to request a review once this is ready.

mtrezza

Apologies for the long wait time, it has slipped through my notifications...

If you rebase this on master, you'll find a new deprecation ID in the deprecation table. you can just increment the last one for this deprecation.

mtrezza · 2021-09-09T10:51:22Z

DEPRECATIONS.md

@@ -6,6 +6,7 @@ The following is a list of deprecations, according to the [Deprecation Policy](h
 |-------------------------------------------------|----------------------------------------------------------------------|---------------------------------|---------------------------------|-----------------------|-------|
 | Native MongoDB syntax in aggregation pipeline   | [#7338](https://github.com/parse-community/parse-server/issues/7338) | 5.0.0 (2022)                    | 6.0.0 (2023)                    | deprecated            | -     |
 | Config option `directAccess` defaults to `true` | [#6636](https://github.com/parse-community/parse-server/pull/6636)   | 5.0.0 (2022)                    | 6.0.0 (2023)                    | deprecated            | -     |
+| `explain` queries used by non-master users | [#7519](https://github.com/parse-community/parse-server/issues/7519)   | 5.0.0 (2022)                    | 6.0.0 (2023) | deprecated            | -     |


Can you rephrase that as

Execute explain queries without masterkey

And can you add an ID for this deprecation, after you merge master into this PR

mtrezza · 2021-09-09T10:52:03Z

spec/ParseQuery.spec.js

@@ -5218,4 +5218,36 @@ describe('Parse.Query testing', () => {
    // Validate
    expect(result.executionStats).not.toBeUndefined();
  });
+
+  xit('users cannot use explain queries', async () => {


I assume this is in preparation for after the deprecation?

Could you add a short comment above?

Deprecation DEPPS3: enable this test after deprecation

Assuming that DEPPS3 is the ID of this deprecation

mtrezza · 2021-09-09T10:52:14Z

spec/ParseQuery.spec.js

+      equal(e.message, 'Cannot explain');
+    }
+  }).pend('Disabled until non-master explains are disabled');
+  it('the master key can use explain queries', async () => {


newline between tests

mtrezza · 2021-09-09T11:11:34Z

CHANGELOG.md

@@ -108,6 +108,7 @@ ___
 - Added Deprecation Policy to govern the introduction of breaking changes in a phased pattern that is more predictable for developers (Manuel Trezza) [#7199](https://github.com/parse-community/parse-server/pull/7199)
 - Add REST API endpoint `/loginAs` to create session of any user with master key; allows to impersonate another user. (GormanFletcher) [#7406](https://github.com/parse-community/parse-server/pull/7406)
 - Add official support for MongoDB 5.0 (Manuel Trezza) [#7469](https://github.com/parse-community/parse-server/pull/7469)
+- Deprecated ``explain` queries run by non-master users (Kartal Kaan Bozdogan) [#7519](https://github.com/parse-community/parse-server/issues/7519)


Can you rephrase this as:

Deprecated executing explain queries without masterkey

mtrezza · 2021-09-09T11:13:49Z

src/rest.js

@@ -26,6 +27,12 @@ function checkLiveQuery(className, config) {
 // Returns a promise for an object with optional keys 'results' and 'count'.
 function find(config, auth, className, restWhere, restOptions, clientSDK, context) {
  enforceRoleSecurity('find', className, auth);
+  if (restOptions && restOptions.explain && !auth.isMaster) {
+    //throw new Parse.Error(Parse.Error.OPERATION_FORBIDDEN, 'Cannot explain');


Can you also add a comment here please, similar to the one above Deprecation DEPPS3: ...

mtrezza · 2021-09-09T11:14:36Z

src/rest.js

@@ -57,6 +64,12 @@ function find(config, auth, className, restWhere, restOptions, clientSDK, contex
 const get = (config, auth, className, objectId, restOptions, clientSDK, context) => {
  var restWhere = { objectId };
  enforceRoleSecurity('get', className, auth);
+  if (restOptions && restOptions.explain && !auth.isMaster) {
+    //throw new Parse.Error(Parse.Error.OPERATION_FORBIDDEN, 'Cannot explain');
+    Deprecator.logRuntimeDeprecation({


Can you also add a comment here please, similar to the one above Deprecation DEPPS3: ...

mtrezza · 2021-09-30T23:35:31Z

@mstniy Do you think you could resolve the conflicts so we can prepare this PR for merge?

Explain queries run by non-master users now print a runtime deprecati…

2359f6e

…on warning Added pending tests that ensure non-master users cannot run explain queries Added the change to the CHANGELOG

mstniy mentioned this pull request Aug 24, 2021

refactor(query): deprecate explain without master key #7520

Closed

7 tasks

mtrezza reviewed Aug 24, 2021

View reviewed changes

DEPRECATIONS.md Outdated Show resolved Hide resolved

mstniy added 2 commits August 25, 2021 11:56

Add deprecation version number

b9e2706

Merge branch 'master' into explain_7519_runtime

882861b

mtrezza changed the title ~~security(query): deprecate explain without master key~~ refactor(query): deprecate explain without master key Aug 25, 2021

Merge remote-tracking branch 'official/master' into explain_7519_runtime

d27d3f6

mstniy requested a review from mtrezza August 26, 2021 18:18

Merge branch 'master' into explain_7519_runtime

3577c96

mtrezza requested changes Sep 9, 2021

View reviewed changes

mtrezza force-pushed the alpha branch from 59215e6 to e6d7d8f Compare May 1, 2022 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(query): deprecate explain without master key #7521

refactor(query): deprecate explain without master key #7521

mstniy commented Aug 24, 2021

codecov bot commented Aug 24, 2021 •

edited

Loading

mtrezza commented Aug 26, 2021

mtrezza left a comment •

edited

Loading

mtrezza Sep 9, 2021

mtrezza Sep 9, 2021

mtrezza Sep 9, 2021

mtrezza Sep 9, 2021

mtrezza Sep 9, 2021

mtrezza Sep 9, 2021

mtrezza commented Sep 30, 2021

refactor(query): deprecate explain without master key #7521

Are you sure you want to change the base?

refactor(query): deprecate explain without master key #7521

Conversation

mstniy commented Aug 24, 2021

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

codecov bot commented Aug 24, 2021 • edited Loading

Codecov Report

mtrezza commented Aug 26, 2021

mtrezza left a comment • edited Loading

Choose a reason for hiding this comment

mtrezza Sep 9, 2021

Choose a reason for hiding this comment

mtrezza Sep 9, 2021

Choose a reason for hiding this comment

mtrezza Sep 9, 2021

Choose a reason for hiding this comment

mtrezza Sep 9, 2021

Choose a reason for hiding this comment

mtrezza Sep 9, 2021

Choose a reason for hiding this comment

mtrezza Sep 9, 2021

Choose a reason for hiding this comment

mtrezza commented Sep 30, 2021

codecov bot commented Aug 24, 2021 •

edited

Loading

mtrezza left a comment •

edited

Loading