Skip to content

Add push parameter checking and query installation #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ function ParseServer(args) {
router.merge(require('./sessions'));
router.merge(require('./roles'));
router.merge(require('./analytics'));
router.merge(require('./push'));
router.merge(require('./push').router);
router.merge(require('./installations'));
router.merge(require('./functions'));
router.merge(require('./schemas'));
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"mongodb": "~2.1.0",
"multer": "^1.1.0",
"parse": "^1.7.0",
"moment": "^2.11.1",
"request": "^2.65.0"
},
"devDependencies": {
Expand All @@ -30,7 +31,7 @@
},
"scripts": {
"pretest": "MONGODB_VERSION=${MONGODB_VERSION:=3.0.8} mongodb-runner start",
"test": "TESTING=1 ./node_modules/.bin/istanbul cover --include-all-sources -x **/spec/** ./node_modules/.bin/jasmine",
"test": "NODE_ENV=test TESTING=1 ./node_modules/.bin/istanbul cover --include-all-sources -x **/spec/** ./node_modules/.bin/jasmine",
"posttest": "mongodb-runner stop",
"start": "./bin/parse-server"
},
Expand Down
121 changes: 114 additions & 7 deletions push.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,124 @@

var Parse = require('parse/node').Parse,
PromiseRouter = require('./PromiseRouter'),
rest = require('./rest');
rest = require('./rest'),
moment = require('moment');

var router = new PromiseRouter();
var validPushTypes = ['ios', 'android'];

function handlePushWithoutQueue(req) {
validateMasterKey(req);
var where = getQueryCondition(req);
validateDeviceType(where);
// Replace the expiration_time with a valid Unix epoch milliseconds time
req.body['expiration_time'] = getExpirationTime(req);
return rest.find(req.config, req.auth, '_Installation', where).then(function(response) {
throw new Parse.Error(Parse.Error.COMMAND_UNAVAILABLE,
'This path is not implemented yet.');
});
}

/**
* Check whether the deviceType parameter in qury condition is valid or not.
* @param {Object} where A query condition
*/
function validateDeviceType(where) {
var where = where || {};
var deviceTypeField = where.deviceType || {};
var deviceTypes = [];
if (typeof deviceTypeField === 'string') {
deviceTypes.push(deviceTypeField);
} else if (typeof deviceTypeField['$in'] === 'array') {
deviceTypes.concat(deviceTypeField['$in']);
}
for (var i = 0; i < deviceTypes.length; i++) {
var deviceType = deviceTypes[i];
if (validPushTypes.indexOf(deviceType) < 0) {
throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED,
deviceType + ' is not supported push type.');
}
}
}

/**
* Get expiration time from the request body.
* @param {Object} request A request object
* @returns {Number|undefined} The expiration time if it exists in the request
*/
function getExpirationTime(req) {
var body = req.body || {};
var hasExpirationTime = !!body['expiration_time'];
if (!hasExpirationTime) {
return;
}
var expirationTimeParam = body['expiration_time'];
var expirationTime;
if (typeof expirationTimeParam === 'number') {
expirationTime = new Date(expirationTimeParam * 1000);
} else if (typeof expirationTimeParam === 'string') {
expirationTime = new Date(expirationTimeParam);
} else {
throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED,
body['expiration_time'] + ' is not valid time.');
}
// Check expirationTime is valid or not, if it is not valid, expirationTime is NaN
if (!isFinite(expirationTime)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it might be some left over code from before? expirationTime is a Date, doesn't seem like it makes sense to call isFinite on it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, isFinte can be used to check Date object directly, since it will auto transfer Date to number. If Date is invalid, it will return false, otherwise it will return true.

throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED,
body['expiration_time'] + ' is not valid time.');
}
return expirationTime.valueOf();
}

/**
* Get query condition from the request body.
* @param {Object} request A request object
* @returns {Object} The query condition, the where field in a query api call
*/
function getQueryCondition(req) {
var body = req.body || {};
var hasWhere = typeof body.where !== 'undefined';
var hasChannels = typeof body.channels !== 'undefined';

var where;
if (hasWhere && hasChannels) {
throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED,
'Channels and query can not be set at the same time.');
} else if (hasWhere) {
where = body.where;
} else if (hasChannels) {
where = {
"channels": {
"$in": body.channels
}
}
} else {
throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED,
'Channels and query should be set at least one.');
}
return where;
}

function notImplementedYet(req) {
throw new Parse.Error(Parse.Error.COMMAND_UNAVAILABLE,
'This path is not implemented yet.');
/**
* Check whether the api call has master key or not.
* @param {Object} request A request object
*/
function validateMasterKey(req) {
if (req.info.masterKey !== req.config.masterKey) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks pushes with client keys, but I guess you can add that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for the V1 version, we do not plan to support client push. We can add it later.

throw new Parse.Error(Parse.Error.PUSH_MISCONFIGURED,
'Master key is invalid, you should only use master key to send push');
}
}

router.route('POST','/push', notImplementedYet);
var router = new PromiseRouter();
router.route('POST','/push', handlePushWithoutQueue);

module.exports = {
router: router
}

module.exports = router;
if (typeof process !== 'undefined' && process.env.NODE_ENV === 'test') {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a good way to expose internal methods for testing purpose? I follow the same way we do in JS and React repo, but I am not so confident about this. @peterdotjs @hallucinogen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have used TESTING=1 which loads testing-routes, you could re-use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gfosco , haha, then I know why we have TESTING=1. But it seems we have a bug here. I remember all parameters in process.env is string so process.env.TESTING == 1 will always return false. Could you double check that?
If TESTING = 1 is just for test purpose, probably using process.env.NODE_ENV is a better idea. It is more common use than setting random variable and we can get rid of the integer/string stuff. I will send a PR if you do not mind.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds fine to replace TESTING with NODE_ENV. Pretty sure JS coerces "1" == 1 just fine, it's "1" === 1 that would fail.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commenting the original question: yes the check looks fine. Just make sure it's consistent across the code. In JS SDK we achieve this by caching the value in some JSON config https://github.com/ParsePlatform/Parse-SDK-JS/blob/master/src/CoreManager.js#L111

module.exports.getQueryCondition = getQueryCondition;
module.exports.validateMasterKey = validateMasterKey;
module.exports.getExpirationTime = getExpirationTime;
module.exports.validateDeviceType = validateDeviceType;
}
206 changes: 206 additions & 0 deletions spec/push.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
var push = require('../push');

describe('push', () => {
it('can check valid master key of request', (done) => {
// Make mock request
var request = {
info: {
masterKey: 'masterKey'
},
config: {
masterKey: 'masterKey'
}
}

expect(() => {
push.validateMasterKey(request);
}).not.toThrow();
done();
});

it('can check invalid master key of request', (done) => {
// Make mock request
var request = {
info: {
masterKey: 'masterKey'
},
config: {
masterKey: 'masterKeyAgain'
}
}

expect(() => {
push.validateMasterKey(request);
}).toThrow();
done();
});

it('can get query condition when channels is set', (done) => {
// Make mock request
var request = {
body: {
channels: ['Giants', 'Mets']
}
}

var where = push.getQueryCondition(request);
expect(where).toEqual({
'channels': {
'$in': ['Giants', 'Mets']
}
});
done();
});

it('can get query condition when where is set', (done) => {
// Make mock request
var request = {
body: {
'where': {
'injuryReports': true
}
}
}

var where = push.getQueryCondition(request);
expect(where).toEqual({
'injuryReports': true
});
done();
});

it('can get query condition when nothing is set', (done) => {
// Make mock request
var request = {
body: {
}
}

expect(function() {
push.getQueryCondition(request);
}).toThrow();
done();
});

it('can throw on getQueryCondition when channels and where are set', (done) => {
// Make mock request
var request = {
body: {
'channels': {
'$in': ['Giants', 'Mets']
},
'where': {
'injuryReports': true
}
}
}

expect(function() {
push.getQueryCondition(request);
}).toThrow();
done();
});

it('can validate device type when no device type is set', (done) => {
// Make query condition
var where = {
}

expect(function(){
push.validateDeviceType(where);
}).not.toThrow();
done();
});

it('can validate device type when single valid device type is set', (done) => {
// Make query condition
var where = {
'deviceType': 'ios'
}

expect(function(){
push.validateDeviceType(where);
}).not.toThrow();
done();
});

it('can validate device type when multiple valid device types are set', (done) => {
// Make query condition
var where = {
'deviceType': {
'$in': ['android', 'ios']
}
}

expect(function(){
push.validateDeviceType(where);
}).not.toThrow();
done();
});

it('can throw on validateDeviceType when single invalid device type is set', (done) => {
// Make query condition
var where = {
'deviceType': 'osx'
}

expect(function(){
push.validateDeviceType(where);
}).toThrow();
done();
});

it('can throw on validateDeviceType when single invalid device type is set', (done) => {
// Make query condition
var where = {
'deviceType': 'osx'
}

expect(function(){
push.validateDeviceType(where)
}).toThrow();
done();
});

it('can get expiration time in string format', (done) => {
// Make mock request
var timeStr = '2015-03-19T22:05:08Z';
var request = {
body: {
'expiration_time': timeStr
}
}

var time = push.getExpirationTime(request);
expect(time).toEqual(new Date(timeStr).valueOf());
done();
});

it('can get expiration time in number format', (done) => {
// Make mock request
var timeNumber = 1426802708;
var request = {
body: {
'expiration_time': timeNumber
}
}

var time = push.getExpirationTime(request);
expect(time).toEqual(timeNumber * 1000);
done();
});

it('can throw on getExpirationTime in invalid format', (done) => {
// Make mock request
var request = {
body: {
'expiration_time': 'abcd'
}
}

expect(function(){
push.getExpirationTime(request);
}).toThrow();
done();
});
});