parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.9k stars 4.78k forks source link

LiveQuery WebSocket server ignores enableAnonymousUsers and CLP #2851

Closed ruimgoncalves closed 6 years ago

ruimgoncalves commented 8 years ago

LiveQuery WebSocket server ignoresenableAnonymousUsers = false setting and allows an anonymous user to connect and subscribe to queries even if sessionTokenis not passed.

The LiveQuery server should prevent clients from connecting if this setting is turned off, and make both servers work consistently. This also prevents the need of setting ACL on each document if the intent is only check for current logged-in users and thus reducing the servers overhead.

I also unchecked the GameScore class public read and write operations in CLP via the Parse Dashboard, so I expected the server to block read operations from unknown users. Then I read the LiveQuery docs and it does not take into account CLP. I think it could be interesting to have less granular security management in some scenarios, like this one.

Steps to reproduce

  1. Install latest version, git clone https://github.com/ParsePlatform/parse-server-example.git and npm install
  2. Create a GameScore class with some fields, like [playerName, score, cheatMode], goto security (CLP) and uncheck Public Read and Write.
  3. Change the server index.js file
var api = new ParseServer({
  databaseURI: databaseUri || 'mongodb://localhost:27017/dev',
  cloud: process.env.CLOUD_CODE_MAIN || __dirname + '/cloud/main.js',
  appId: process.env.APP_ID || 'myAppId',
  masterKey: process.env.MASTER_KEY || 'masterKey', //Add your master key here. Keep it secret!
  //clientKey: process.env.CLIENT_KEY || 'clientKey',
  serverURL: process.env.SERVER_URL || 'http://localhost:1337/parse',  
  enableAnonymousUsers : process.env.ENABLE_ANON_USERS || false,
  allowClientClassCreation: process.env.CLIENT_CLASS_CREATION || false,
  verbose : process.env.VERBOSE || 1,
  liveQuery: {
    classNames: ["GameScore"] // List of classes to support for query subscriptions
  }
});
  1. Create an empty html file add this javascript script
Parse.initialize("myAppId");
Parse.serverURL = 'http://localhost:1337/parse';
console.log("Current user", Parse.User.current());

let query = new Parse.Query('GameScore');
query.find().then((data, err)=>{
   console.log("Find = ",data, err);
});

let subscription = query.subscribe();
subscription.on('open', () => {
   console.log("Connection Open");
});

Parse.LiveQuery.on('error', (error) => {
   console.error("Connection Error", error);
});

Expected Results

The logs messages should be

Current User, null
Connection Error ...

The connection should be rejected and a console message should be Connection Error. The client should not receive additional messages from the ws server.

Actual Outcome

Instead you get in the browsers console the log message

Current User, null
Connection Open

As proof, open chrome dev tools -> network, filter by WS (websocket) connections and open the row with the parse name, then select the frames tab, you get data frames every time the subscribed data is changed. So to test this open Parse Dashboard and create, delete or edit documents in the GameScore collection.

Environment Setup

verbose: Current clients 0
verbose: Current subscriptions 0
verbose: REQUEST for [GET] /parse/classes/GameScore: {
  "where": {
  },
} method=GET, url=/parse/classes/GameScore, host=localhost:1337, connection=keep-alive, content-length=177, pragma=no-cache, cache-control=no-cache, origin=http://localhost:1337, user-agent=Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36, content-type=text/plain, accept=*/*, referer=http://localhost:1337/public/test.html, accept-encoding=gzip, deflate, ,pt;q=0.8,en-US;q=0.6,en;q=0.4, 
error: Error generating response. ParseError {
  code: 119,
  message: 'Permission denied for action find on class GameScore.' } code=119, message=Permission denied for action find on class GameScore.
verbose: Request: %j op=connect, applicationId=myAppId
info: Create new client: 1
verbose: Push Response : "{\"op\":\"connected\",\"clientId\":1}"
[object Object]
verbose: Request: %j op=subscribe, requestId=1, className=GameScore, 
verbose: Push Response : "{\"op\":\"subscribed\",\"clientId\":1,\"requestId\":1}"
verbose: Create client 1 new subscription: 1
verbose: Current client number: 1
flovilmart commented 8 years ago

enableAnonymousUsers=false actually don't prevent anyone to fetch data from parse-server. This feature actually let users to have a session without being explicitly signed in through email and password, setting it to off, just prevents sessions to be created.

ruimgoncalves commented 8 years ago

Hi @flovilmart, thanks for your reply, so how does one prevent anonymous users from fetching data? Should I change the title?

flovilmart commented 8 years ago

so how does one prevent anonymous users from fetching data

At the moment, there is no straightforward way of achieving that, you can use a mix of Roles, ACL's and CLP.

There is this PR https://github.com/ParsePlatform/parse-server/pull/893 that would give a 'false sense of security' unless you restrict who can signup / disable signup altogether.

TimNZ commented 6 years ago

I had a quick look through LiveQuery src, and it seems class level permissions are not checked, only object ACL.

Is CLP checks going to be implemented?

I have classes with CLP of 'requiresAuthentication' for everything, so subscribed clients who are not yet signed in are getting subscription events.

flovilmart commented 6 years ago

Yes, you are right, they are not implemented yet, i’ll Have a look to implement them.

TimNZ commented 6 years ago

Thanks. I'd give it a go and do a pull , but I'm time challenged, and would want to maintain the code structure and style which is a learning curve.

For example, would you put CLP check in matchesACL(), or a new function matchesCLP(), and then have a parent function hasAccess() that calls matchesACL() and matchesCLP(), requiring a refactor of all calls to matchesACL()

flovilmart commented 6 years ago

@TimNZ I'm having a look right now and the biggest challenge would be to refactor the CLP logic so it can be shared with the liveQuery. Contrary to ACL's where we can match the ACL against any user / session token, the CLP's are stored in the schema, and we may not want to get it from the schema all the times.

I'd add a new method, matchesCLP, before matchesACL. In parse-server, CLP's must pass before we even consider looking into the ACL's. See SchemaController.validatePermissions

The easiest way to get a schemaContoller is through the DatabaseController

Because we have the AppId, if ParseLiveQueryServer is sharing the parse-server instance, that helps, otherwise we need to provide a way to load a databaseController on it, or an access to the shared SchemaCache :)

For now we don't have 'readOnly' SchemaController that would be driven only by the cache, but that could be an idea, for example if you use a distributed cache like Redis.

As you see it's not all trivial :)

Last resort solution, it the regular REST API to load the schema (which I would not recommend)

Another thing we could do, it provide the CLP object, when we emit the object in ParseCloudCodePublisher

This way, the LiveQueryServer may take the decision it want. (that may be actually the best solution).

A small refactor on the SChemaController to make the validatePermissions static, so you can pass a schema / list of classLevelPermissions to test against would probably be needed, but that's minor.

TimNZ commented 6 years ago

Ok. Definitely leaving it to the expert.

But I'm launching next week, so of course I expect you to make this your #1 priority :) [jokes]

flovilmart commented 6 years ago

Haha, yeah it's kinda a rabbit hole, but I'll try solution 2 as it's been obviously badly overlooked.

flovilmart commented 6 years ago

@TimNZ, what’s your particular use case for classLevel permissions? I believe pointer permissions right? Because liveQuery is only about reads. Do you expect with find disabled to not propagate the objects?

TimNZ commented 6 years ago

Yes. If a client doesn't have permissions to a class/object, they shouldn't be getting any notifications (and a copy of the object) of them.

I'm using JS client in browser extensions and Electron apps.

It's not a big deal, I'm going to create a 'broadcast' class that is the only class enabled for live queries that has basic details for doing subsequent query on other classes for via queries.

But even then I don't want non authenticated clients getting subscription notifications until they have authenticated - e.g. requiresAuthentication = true.

I can not subscribe the client till they are authenticated, but if someone is bored they could simply copy my JS code and get subscription events using visible App ID and JS keys. Would they, very unlikely, nothing interesting in what I'm going to push without access to other classes.

But to me it is obvious that the server should be enforcing the security model consistently.

TimNZ commented 6 years ago

Whatever implementation/options enables the above works for me. Having a 'requiresAuthentication = true' or 'enableAnonymousClient = false' option on the LiveQuery Server would do the job as well.

flovilmart commented 6 years ago

Ok, i’ll Have a look then. Thanks ;)

TimNZ commented 6 years ago

Thanks a lot Florent, great service!

I wrote my own BaaS in 2012->2014, after discovering Kinvey.

http://cloudglue.xemware.com/help/platform/endpoints

It's ironic that I abandoned it, and now using Parse for a totally different project. I'm going round in circles :)

Though my was aimed as full front and backend solution, and accessible to business type folk. Might use Parse to reattempt this one day.

TimNZ commented 6 years ago

Heads up I'm going to do a pull in a few days.

These changes support what I want which is granular control to deny live query subscriptions, and modify subscription queries. Current system is all or nothing, and taxing if clients can't be prevented from connecting or control what queries they can subscribe to, both of which I think have performance considerations at scale.

Negates need for CLP checks.

Changes so far:

ParseLiveQueryServer.js



Planning in future to:
* Have LiveQuery event handler support promise completion like Cloud Functions, to allow async processing.  For example I want to retrieve the current User object in 'beforesubscribe' to do query modification.

Please provide any feedback/thoughts.
flovilmart commented 6 years ago

Enable anonymous users is not a way to prevent non logged in users to connect to parse-server, but it allows creating users with the anonymous auth data, generated by the client SDK’s.

There is no way, at the moment to prevent a client to fetch data from parse-server, and it’s unlikely a change we’ll allow.

flovilmart commented 6 years ago

I would be ok with adding onConnect, onSubscribe etc.. like options that would be functions that return a promise or throw an error to prevent further processing / mutation. Not exposing it as cloud code triggers

flovilmart commented 6 years ago

The CLP checks are an open PR. Role support will come later.

TimNZ commented 6 years ago

Enable anonymous users is not a way to prevent non logged in users to connect to parse-server, but it allows creating users with the anonymous auth data, generated by the client SDK’s.

There is no way, at the moment to prevent a client to fetch data from parse-server, and it’s unlikely a change we’ll allow.

My naming is off then.

Regardless of the authentication mechanism, an authenticated/logged in user always has a sessionToken?

If so, then I have implemented a config option (which needs to be named appropriately) that acts as a basic control mechanism to control allowing live query connects and subscriptions. Without this, you are relying on the ACL mechanism which comes at a performance cost for each connected client, which you have been unable to prevent from connecting/subscribing.

Is that sort of control not good practice?

TimNZ commented 6 years ago

The CLP checks are an open PR. Role support will come later.

Thanks, did not see this.

TimNZ commented 6 years ago

Final thing for a few days as I better focus on launching product!

I would be ok with adding onConnect, onSubscribe etc.. like options that would be functions that return a promise or throw an error to prevent further processing / mutation. Not exposing it as cloud code triggers

Right now LQ Server events are hooked via Parse.Cloud.onLiveQueryEvent(), what does what you are suggesting above look like instead?

flovilmart commented 6 years ago

Yeah, probably was not the better design of all, as the liveQuery may be disconnected from parse-server IIRC. Would functions passed part of the initialization options work for you?

TimNZ commented 6 years ago

Thanks.

Whatever supports the changes which I will use to implement block and override subscriptions as per earlier comment, presumption being I can test for sessionToken as a basic control over whether to deny connect/subscribe, which is present regardless of auth mechanism.

https://github.com/TimNZ/parse-server/commit/a8ab11d35b990103508b26ff4ac14e0158b6730f#diff-7ff5112d0c45b691746af5e56e5e3c92

stale[bot] commented 6 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

rsmets commented 2 years ago

I have found myself in a situation where I need to support Live Query subscriptions to a client without a session. Thus the ACL way of limiting LQ subscriptions rights is not an option. I was under the impression that CLPs would work if I only enabled Read on Get (with objects created with custom IDs generated on the client so it only was able to subscribe to its corresponding entity). However not the case, CLPs are not respected when it comes to LQ.

If I am understanding this old thread correctly LQ CLP limitations was never added due to the complexity of sharing the schema info with the LQ before hooks? And the request / work to support was dropped because a workaround, using session tokens, was conceived?

According to this issue it was being worked on and the issue was closed however does not seem to be the case it was ever actually done?

Thanks for rehashing this old topic. This seems to be the most relevant issue to what I am aiming todo.