Skip to content

Commit 3bcb5a0

Browse files
flovilmartflovilmart
authored
Ensure User ACL's are more flexible and secure #3588 (#4860)
* Fixes an issue that would let the beforeDelete be called when user has no access to the object * Ensure we properly lock user - Improves find method so we can attempt to read for a write poking the right ACL instead of using masterKey - This ensure we do not run beforeDelete/beforeFind/beforeSave in the wrong scenarios * nits * Caps insufficient
1 parent 9e5d26e commit 3bcb5a0

File tree

9 files changed

+158
-39
lines changed

9 files changed

+158
-39
lines changed

‎spec/ParseAPI.spec.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -975,6 +975,25 @@ describe('miscellaneous', function() {
975975
});
976976
});
977977

978+
it('test beforeDelete with locked down ACL', async () => {
979+
let called = false;
980+
Parse.Cloud.beforeDelete('GameScore', (req, res) => {
981+
called = true;
982+
res.success();
983+
});
984+
const object = new Parse.Object('GameScore');
985+
object.setACL(new Parse.ACL());
986+
await object.save();
987+
const objects = await new Parse.Query('GameScore').find();
988+
expect(objects.length).toBe(0);
989+
try {
990+
await object.destroy();
991+
} catch(e) {
992+
expect(e.code).toBe(Parse.Error.OBJECT_NOT_FOUND);
993+
}
994+
expect(called).toBe(false);
995+
});
996+
978997
it('test cloud function query parameters', (done) => {
979998
Parse.Cloud.define('echoParams', (req, res) => {
980999
res.success(req.params);

‎spec/ParseUser.spec.js

Lines changed: 71 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,71 @@ describe('Parse.User testing', () => {
525525
});
526526
});
527527

528+
it('never locks himself up', async () => {
529+
const user = new Parse.User();
530+
await user.signUp({
531+
username: 'username',
532+
password: 'password'
533+
});
534+
user.setACL(new Parse.ACL());
535+
await user.save();
536+
await user.fetch();
537+
expect(user.getACL().getReadAccess(user)).toBe(true);
538+
expect(user.getACL().getWriteAccess(user)).toBe(true);
539+
const publicReadACL = new Parse.ACL();
540+
publicReadACL.setPublicReadAccess(true);
541+
542+
// Create an administrator role with a single admin user
543+
const role = new Parse.Role('admin', publicReadACL);
544+
const admin = new Parse.User();
545+
await admin.signUp({
546+
username: 'admin',
547+
password: 'admin',
548+
});
549+
role.getUsers().add(admin);
550+
await role.save(null, { useMasterKey: true });
551+
552+
// Grant the admins write rights on the user
553+
const acl = user.getACL();
554+
acl.setRoleWriteAccess(role, true);
555+
acl.setRoleReadAccess(role, true);
556+
557+
// Update with the masterKey just to be sure
558+
await user.save({ ACL: acl }, { useMasterKey: true });
559+
560+
// Try to update from admin... should all work fine
561+
await user.save({ key: 'fromAdmin'}, { sessionToken: admin.getSessionToken() });
562+
await user.fetch();
563+
expect(user.toJSON().key).toEqual('fromAdmin');
564+
565+
// Try to save when logged out (public)
566+
let failed = false;
567+
try {
568+
// Ensure no session token is sent
569+
await Parse.User.logOut();
570+
await user.save({ key: 'fromPublic'});
571+
} catch(e) {
572+
failed = true;
573+
expect(e.code).toBe(Parse.Error.SESSION_MISSING);
574+
}
575+
expect({ failed }).toEqual({ failed: true });
576+
577+
// Try to save with a random user, should fail
578+
failed = false;
579+
const anyUser = new Parse.User();
580+
await anyUser.signUp({
581+
username: 'randomUser',
582+
password: 'password'
583+
});
584+
try {
585+
await user.save({ key: 'fromAnyUser'});
586+
} catch(e) {
587+
failed = true;
588+
expect(e.code).toBe(Parse.Error.SESSION_MISSING);
589+
}
590+
expect({ failed }).toEqual({ failed: true });
591+
});
592+
528593
it("current user", (done) => {
529594
const user = new Parse.User();
530595
user.set("password", "asdf");
@@ -2379,7 +2444,7 @@ describe('Parse.User testing', () => {
23792444
}, (error, response, body) => {
23802445
expect(error).toBe(null);
23812446
const b = JSON.parse(body);
2382-
expect(b.error).toBe('invalid session token');
2447+
expect(b.error).toBe('Invalid session token');
23832448
request.put({
23842449
headers: {
23852450
'X-Parse-Application-Id': 'test',
@@ -2471,7 +2536,7 @@ describe('Parse.User testing', () => {
24712536
expect(error).toBe(null);
24722537
const b = JSON.parse(body);
24732538
expect(b.code).toEqual(209);
2474-
expect(b.error).toBe('invalid session token');
2539+
expect(b.error).toBe('Invalid session token');
24752540
done();
24762541
});
24772542
});
@@ -2513,7 +2578,7 @@ describe('Parse.User testing', () => {
25132578
}, (error,response,body) => {
25142579
const b = JSON.parse(body);
25152580
expect(b.code).toEqual(209);
2516-
expect(b.error).toBe('invalid session token');
2581+
expect(b.error).toBe('Invalid session token');
25172582
done();
25182583
});
25192584
});
@@ -2550,7 +2615,7 @@ describe('Parse.User testing', () => {
25502615
done();
25512616
}, function(err) {
25522617
expect(err.code).toBe(Parse.Error.INVALID_SESSION_TOKEN);
2553-
expect(err.message).toBe('invalid session token');
2618+
expect(err.message).toBe('Invalid session token');
25542619
done();
25552620
});
25562621
});
@@ -2626,7 +2691,7 @@ describe('Parse.User testing', () => {
26262691
});
26272692
});
26282693

2629-
it("invalid session tokens are rejected", (done) => {
2694+
it("Invalid session tokens are rejected", (done) => {
26302695
Parse.User.signUp("asdf", "zxcv", null, {
26312696
success: function() {
26322697
request.get({
@@ -2639,7 +2704,7 @@ describe('Parse.User testing', () => {
26392704
},
26402705
}, (error, response, body) => {
26412706
expect(body.code).toBe(209);
2642-
expect(body.error).toBe('invalid session token');
2707+
expect(body.error).toBe('Invalid session token');
26432708
done();
26442709
})
26452710
}

‎spec/helper.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,6 @@ if (process.env.PARSE_SERVER_TEST_CACHE === 'redis') {
114114
}
115115

116116
const openConnections = {};
117-
118117
// Set up a default API server for testing with default configuration.
119118
let server;
120119

‎src/Auth.js

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,14 +21,14 @@ function Auth({ config, isMaster = false, isReadOnly = false, user, installation
2121

2222
// Whether this auth could possibly modify the given user id.
2323
// It still could be forbidden via ACLs even if this returns true.
24-
Auth.prototype.couldUpdateUserId = function(userId) {
24+
Auth.prototype.isUnauthenticated = function() {
2525
if (this.isMaster) {
26-
return true;
26+
return false;
2727
}
28-
if (this.user && this.user.id === userId) {
29-
return true;
28+
if (this.user) {
29+
return false;
3030
}
31-
return false;
31+
return true;
3232
};
3333

3434
// A helper to get a master-level Auth object
@@ -64,7 +64,7 @@ var getAuthForSessionToken = function({ config, sessionToken, installationId } =
6464
return query.execute().then((response) => {
6565
var results = response.results;
6666
if (results.length !== 1 || !results[0]['user']) {
67-
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
67+
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
6868
}
6969

7070
var now = new Date(),

‎src/Controllers/DatabaseController.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -869,7 +869,8 @@ class DatabaseController {
869869
op,
870870
distinct,
871871
pipeline,
872-
readPreference
872+
readPreference,
873+
isWrite,
873874
}: any = {}): Promise<any> {
874875
const isMaster = acl === undefined;
875876
const aclGroup = acl || [];
@@ -930,7 +931,11 @@ class DatabaseController {
930931
}
931932
}
932933
if (!isMaster) {
933-
query = addReadACL(query, aclGroup);
934+
if (isWrite) {
935+
query = addWriteACL(query, aclGroup);
936+
} else {
937+
query = addReadACL(query, aclGroup);
938+
}
934939
}
935940
validateQuery(query);
936941
if (count) {

‎src/RestQuery.js

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,13 @@ function RestQuery(config, auth, className, restWhere = {}, restOptions = {}, cl
2424
this.clientSDK = clientSDK;
2525
this.response = null;
2626
this.findOptions = {};
27+
this.isWrite = false;
28+
2729
if (!this.auth.isMaster) {
28-
this.findOptions.acl = this.auth.user ? [this.auth.user.id] : null;
2930
if (this.className == '_Session') {
30-
if (!this.findOptions.acl) {
31+
if (!this.auth.user) {
3132
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN,
32-
'This session token is invalid.');
33+
'Invalid session token');
3334
}
3435
this.restWhere = {
3536
'$and': [this.restWhere, {
@@ -188,17 +189,28 @@ RestQuery.prototype.buildRestWhere = function() {
188189
});
189190
}
190191

192+
// Marks the query for a write attempt, so we read the proper ACL (write instead of read)
193+
RestQuery.prototype.forWrite = function() {
194+
this.isWrite = true;
195+
return this;
196+
}
197+
191198
// Uses the Auth object to get the list of roles, adds the user id
192199
RestQuery.prototype.getUserAndRoleACL = function() {
193-
if (this.auth.isMaster || !this.auth.user) {
200+
if (this.auth.isMaster) {
194201
return Promise.resolve();
195202
}
196-
return this.auth.getUserRoles().then((roles) => {
197-
// Concat with the roles to prevent duplications on multiple calls
198-
const aclSet = new Set([].concat(this.findOptions.acl, roles));
199-
this.findOptions.acl = Array.from(aclSet);
203+
204+
this.findOptions.acl = ['*'];
205+
206+
if (this.auth.user) {
207+
return this.auth.getUserRoles().then((roles) => {
208+
this.findOptions.acl = this.findOptions.acl.concat(roles, [this.auth.user.id]);
209+
return;
210+
});
211+
} else {
200212
return Promise.resolve();
201-
});
213+
}
202214
};
203215

204216
// Changes the className if redirectClassNameForKey is set.
@@ -523,6 +535,9 @@ RestQuery.prototype.runFind = function(options = {}) {
523535
if (options.op) {
524536
findOptions.op = options.op;
525537
}
538+
if (this.isWrite) {
539+
findOptions.isWrite = true;
540+
}
526541
return this.config.database.find(this.className, this.restWhere, findOptions)
527542
.then((results) => {
528543
if (this.className === '_User') {

‎src/RestWrite.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,7 @@ RestWrite.prototype.runDatabaseOperation = function() {
965965

966966
if (this.className === '_User' &&
967967
this.query &&
968-
!this.auth.couldUpdateUserId(this.query.objectId)) {
968+
this.auth.isUnauthenticated()) {
969969
throw new Parse.Error(Parse.Error.SESSION_MISSING, `Cannot modify user ${this.query.objectId}.`);
970970
}
971971

‎src/Routers/UsersRouter.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ export class UsersRouter extends ClassesRouter {
130130

131131
handleMe(req) {
132132
if (!req.info || !req.info.sessionToken) {
133-
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
133+
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
134134
}
135135
const sessionToken = req.info.sessionToken;
136136
return rest.find(req.config, Auth.master(req.config), '_Session',
@@ -140,7 +140,7 @@ export class UsersRouter extends ClassesRouter {
140140
if (!response.results ||
141141
response.results.length == 0 ||
142142
!response.results[0].user) {
143-
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
143+
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
144144
} else {
145145
const user = response.results[0].user;
146146
// Send token back on the login, because SDKs expect that.

‎src/rest.js

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
// things.
99

1010
var Parse = require('parse/node').Parse;
11-
import Auth from './Auth';
1211

1312
var RestQuery = require('./RestQuery');
1413
var RestWrite = require('./RestWrite');
@@ -54,9 +53,9 @@ function del(config, auth, className, objectId) {
5453
'bad objectId');
5554
}
5655

57-
if (className === '_User' && !auth.couldUpdateUserId(objectId)) {
56+
if (className === '_User' && auth.isUnauthenticated()) {
5857
throw new Parse.Error(Parse.Error.SESSION_MISSING,
59-
'insufficient auth to delete user');
58+
'Insufficient auth to delete user');
6059
}
6160

6261
enforceRoleSecurity('delete', className, auth);
@@ -67,14 +66,16 @@ function del(config, auth, className, objectId) {
6766
const hasTriggers = checkTriggers(className, config, ['beforeDelete', 'afterDelete']);
6867
const hasLiveQuery = checkLiveQuery(className, config);
6968
if (hasTriggers || hasLiveQuery || className == '_Session') {
70-
return find(config, Auth.master(config), className, {objectId: objectId})
69+
return new RestQuery(config, auth, className, { objectId })
70+
.forWrite()
71+
.execute()
7172
.then((response) => {
7273
if (response && response.results && response.results.length) {
7374
const firstResult = response.results[0];
7475
firstResult.className = className;
7576
if (className === '_Session' && !auth.isMaster) {
7677
if (!auth.user || firstResult.user.objectId !== auth.user.id) {
77-
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'invalid session token');
78+
throw new Parse.Error(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token');
7879
}
7980
}
8081
var cacheAdapter = config.cacheController;
@@ -110,6 +111,8 @@ function del(config, auth, className, objectId) {
110111
}, options);
111112
}).then(() => {
112113
return triggers.maybeRunTrigger(triggers.Types.afterDelete, auth, inflatedObject, null, config);
114+
}).catch((error) => {
115+
handleSessionMissingError(error, className, auth);
113116
});
114117
}
115118

@@ -130,20 +133,33 @@ function update(config, auth, className, restWhere, restObject, clientSDK) {
130133
const hasTriggers = checkTriggers(className, config, ['beforeSave', 'afterSave']);
131134
const hasLiveQuery = checkLiveQuery(className, config);
132135
if (hasTriggers || hasLiveQuery) {
133-
return find(config, Auth.master(config), className, restWhere);
136+
// Do not use find, as it runs the before finds
137+
return new RestQuery(config, auth, className, restWhere)
138+
.forWrite()
139+
.execute();
134140
}
135141
return Promise.resolve({});
136-
}).then((response) => {
142+
}).then(({ results }) => {
137143
var originalRestObject;
138-
if (response && response.results && response.results.length) {
139-
originalRestObject = response.results[0];
144+
if (results && results.length) {
145+
originalRestObject = results[0];
140146
}
141-
142-
var write = new RestWrite(config, auth, className, restWhere, restObject, originalRestObject, clientSDK);
143-
return write.execute();
147+
return new RestWrite(config, auth, className, restWhere, restObject, originalRestObject, clientSDK)
148+
.execute();
149+
}).catch((error) => {
150+
handleSessionMissingError(error, className, auth);
144151
});
145152
}
146153

154+
function handleSessionMissingError(error, className) {
155+
// If we're trying to update a user without / with bad session token
156+
if (className === '_User'
157+
&& error.code === Parse.Error.OBJECT_NOT_FOUND) {
158+
throw new Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth.');
159+
}
160+
throw error;
161+
}
162+
147163
const classesWithMasterOnlyAccess = ['_JobStatus', '_PushStatus', '_Hooks', '_GlobalConfig', '_JobSchedule'];
148164
// Disallowing access to the _Role collection except by master key
149165
function enforceRoleSecurity(method, className, auth) {

0 commit comments

Comments
 (0)