Disable all remote methods and enable only selected ones (white-list)
voitau opened this issue · 75 comments
There is a bunch of existing open/closed issues with questions how to hide the method of the model. My question is about how can all the methods be disabled first and then enabled only those which are necessary? (basically, duplicates this comment: #465 (comment))
Here is the simple function which hides all the methods of the model:
/**
* Hides all the methods besides those in 'methods'.
*
* @param Model model to be updated.
* @param methods array of methods to expose, e.g.: ['find', 'updateAttributes'].
*/
var setMethodsVisibility = function(Model, methods) {
methods = methods || [];
Model.sharedClass.methods().forEach(function(method) {
method.shared = methods.indexOf(method.name) > -1;
});
};However, it doesn't hide two groups:
- remote methods, which have to be hidden like this: Model.disableRemoteMethod(name, isStatic)
- custom methods, like User.login or User.resetPassword (didn't find any way to hide them at all);
Could you please advise what would be the correct way to hide everything (without specific call for each method by its name explicitly) and allow only the methods which are necessary?
@bajtos Can you please clarify which part of this is docs and which is an enhancement request.
@voitau Could you please advise what would be the correct way to hide everything (without specific call for each method by its name explicitly) and allow only the methods which are necessary?
AFAIK, this is not possible at the moment. Let's keep this issue open to track your request for that. Note that the implementation will require changes in the strong-remoting module too.
However, it doesn't hide custom methods, like User.login or User.resetPassword (didn't find any way to hide them at all);
You have to call User.disableRemoteMethod before creating the explorer middleware. I have filled a new GH issue to fix that - see #686.
@crandmck Can you please clarify which part of this is docs and which is an enhancement request.
After a closer look, this is a pure enhancement. I am going to remove the "doc" label.
I'm able to hide my remote methods from the explorer when using @voitau's function in 2.17.2.
+1
great code @voitau, thanks!
I'm new to loopback, what is the best place to put that code so I can use for all my models?
To hide ALL methods, I ended up doing this
function disableAllMethodsBut(model, methodsToExpose)
{
if(model && model.sharedClass)
{
methodsToExpose = methodsToExpose || [];
var modelName = model.sharedClass.name;
var methods = model.sharedClass.methods();
var relationMethods = [];
var hiddenMethods = [];
try
{
Object.keys(model.definition.settings.relations).forEach(function(relation)
{
relationMethods.push({ name: '__findById__' + relation, isStatic: false });
relationMethods.push({ name: '__destroyById__' + relation, isStatic: false });
relationMethods.push({ name: '__updateById__' + relation, isStatic: false });
relationMethods.push({ name: '__exists__' + relation, isStatic: false });
relationMethods.push({ name: '__link__' + relation, isStatic: false });
relationMethods.push({ name: '__get__' + relation, isStatic: false });
relationMethods.push({ name: '__create__' + relation, isStatic: false });
relationMethods.push({ name: '__update__' + relation, isStatic: false });
relationMethods.push({ name: '__destroy__' + relation, isStatic: false });
relationMethods.push({ name: '__unlink__' + relation, isStatic: false });
relationMethods.push({ name: '__count__' + relation, isStatic: false });
relationMethods.push({ name: '__delete__' + relation, isStatic: false });
});
} catch(err) {}
methods.concat(relationMethods).forEach(function(method)
{
var methodName = method.name;
if(methodsToExpose.indexOf(methodName) < 0)
{
hiddenMethods.push(methodName);
model.disableRemoteMethod(methodName, method.isStatic);
}
});
if(hiddenMethods.length > 0)
{
console.log('\nRemote mehtods hidden for', modelName, ':', hiddenMethods.join(', '), '\n');
}
}
};FYI, I am working on a feature that enables/disables sharedMethods using settings in config.json/model-config.json. It's both a whitelister and blacklister. See #1667
@superkhau Is there any sprint planned for these features?
@mrbatista Anything marked with a label is up for sprint planning. There is no guarantee when the issue will be pulled into the current sprint. See https://waffle.io/strongloop-internal/scrum-loopback.
@EricPrieto In what file did you run that method?
@devonatdomandtom in any file... Just pass the model that you want the methods to be hidden. I usually run that on the model implementation.
@EricPrieto thanks! did you include the function definition there as well?
This is what I'm doing to disable current methods:
YourModel.sharedClass.methods().forEach(function(method) {
YourModel.disableRemoteMethod(method.name, method.isStatic);
});
@devonatdomandtom no, I store this function in some helpers library
helpers.js
module.exports.disableAllMethods = function disableAllMethods(model, methodsToExpose)
{
if(model && model.sharedClass)
{
methodsToExpose = methodsToExpose || [];
var modelName = model.sharedClass.name;
var methods = model.sharedClass.methods();
var relationMethods = [];
var hiddenMethods = [];
try
{
Object.keys(model.definition.settings.relations).forEach(function(relation)
{
relationMethods.push({ name: '__findById__' + relation, isStatic: false });
relationMethods.push({ name: '__destroyById__' + relation, isStatic: false });
relationMethods.push({ name: '__updateById__' + relation, isStatic: false });
relationMethods.push({ name: '__exists__' + relation, isStatic: false });
relationMethods.push({ name: '__link__' + relation, isStatic: false });
relationMethods.push({ name: '__get__' + relation, isStatic: false });
relationMethods.push({ name: '__create__' + relation, isStatic: false });
relationMethods.push({ name: '__update__' + relation, isStatic: false });
relationMethods.push({ name: '__destroy__' + relation, isStatic: false });
relationMethods.push({ name: '__unlink__' + relation, isStatic: false });
relationMethods.push({ name: '__count__' + relation, isStatic: false });
relationMethods.push({ name: '__delete__' + relation, isStatic: false });
});
} catch(err) {}
methods.concat(relationMethods).forEach(function(method)
{
var methodName = method.name;
if(methodsToExpose.indexOf(methodName) < 0)
{
hiddenMethods.push(methodName);
model.disableRemoteMethod(methodName, method.isStatic);
}
});
if(hiddenMethods.length > 0)
{
console.log('\nRemote mehtods hidden for', modelName, ':', hiddenMethods.join(', '), '\n');
}
}
};models/SomeModel.js
var disableAllMethods = require('../helpers.js').disableAllMethods;
module.exports = function(SomeModel)
{
disableAllMethods(SomeModel, [... methodsIDontWannaHide]);
...
};@EricPrieto Amazing sir. That was exceedingly helpful
@EricPrieto thanks!
Anytime, guys :)
@alFReD-NSH thanks, ur code is simple.
Here is gist for model mixin to disable all remote methods and enable only selected in json model config. https://gist.github.com/ratik/5252e4c168a8c29329c0
Please @ratik ,give the proper credits, most of that code was posted here #651 (comment)
@EricPrieto sure, thank you for your code.
@ratik thanks mate :)
@EricPrieto thanks!
@EricPrieto hey thanks for the code man!!, works great and saved me a lot of time!! ;P
@EricPrieto works great. I used the @ratik mixin variant though. That way I could define what methods are expose in the model.json files.
try this loopback-remote-routing
Rather than separately defining the defined methods, I wanted to only include methods supported by my model's ACLs. So, building on @EricPrieto's solution, I added:
function disableUnauthorizedMethods (model) {
const acls = model.definition.settings.acls || [];
let authorizedMethods = [];
acls.forEach((acl) => {
if (acl.permission === 'ALLOW' && acl.property) {
if (Array.isArray(acl.property)) {
authorizedMethods = authorizedMethods.concat(acl.property);
}
else if (acl.property !== '*') {
authorizedMethods.push(acl.property);
}
}
});
disableAllMethods(model, authorizedMethods);
}
Hope that helps someone. If anyone has a scenario where you wouldn't always want the available methods to map to the ACL definitions, I'd be interested to hear it (I'm fairly new to Loopback and may have overlooked something!).
Edited
Rather than call disableUnauthorizedMethods from JS, you can add it as a mixin:
const {disableUnauthorizedMethods} = require('../services/model');
module.exports = (Model) => { // eslint-disable-line
Model.on('attached', () => disableUnauthorizedMethods(Model));
};
Or, you can add it your boot script and run it for all models:
const {disableUnauthorizedMethods} = require('../services/model');
module.exports = (app) => {
Object.keys(app.models).forEach((modelName) => {
const model = app.models[modelName];
disableUnauthorizedMethods(model);
});
};
The latter example does disable the methods, but they will still appear in the Swagger output as the boot script is run after the Swagger stuff is generated. Not sure how to get around this so instead I've created a base model SPersistedModel, activated the mixin for the base class, and extended this for my own custom classes.
I had the same problem.
My first solution was to manually update the "public":true items in server/model-configuration.json but it was overridden anytime I used the Swagger tool to refresh the LoopBack API (with slc loopback:swagger myswaggerfilename command from the project root).
I finally wrote a Grunt task as a reliable workaround.
- Run it just after a
slc loopback:swaggergeneration or just before running the API live. - you just have to specify the names of the paths I want to expose in the javascript array
list_of_REST_path_to_EXPOSE - and make sure you are happy with the backup folder for the original
/server/model-config.jsonfile.
I wanted to share it with you in case you would be interested :
https://github.com/FranckVE/grunt-task-unexpose-rest-path-loopback-swagger
I have an idea: how about adding a model-level setting to control the default behaviour (all methods are public, all methods are private) and then allow models to provide overrides in the "methods" section. These overrides can change not only method visibility, but also other metadata like the description used by swagger generator - IIRC there is an issue asking for this feature, but I cannot find it right now :(
Example of a model definition that exposed only "find" method:
{
name: "Car",
defaultMethodVisibility: "private", // or "public"
methods: {
find: {
public: true,
description: "custom description"
}
}
}I'm keen to see a better standard here, but I feel there's a confusing overlap between whether not you can see a route exists (visibility) and whether or not you can use that route:
- We already had ACLs to define whether or not a route can be accessed. It's really weird that a route I've defined as being completely inaccessibly in my ACLs would require a separate configuration to say that I don't want it to appear in Swagger (of course I don't!). And the same applies for routes are accessible - if I've set up an ACL saying it's accessible, I don't want to have to also configure that it should be visible.
- There's also a real risk that beginners will mark a route as private, see that it's not in Swigger, and then assume that it's secure.
I think the default behaviour should be that routes that are completely inaccessibly in the ACLs are hidden in Swagger, and all other routes are displayed. This encourages good design and makes it much easier to review your ACLs are working when quickly prototyping.
I understand there's a use-case for making a route 'private' (or, to borrow a better term from CSS, 'hidden'), but suggest a solution where this is still configured using ACLs.
Suggested solution
In my model (using existing behaviour):
"name": "Project",
"plural": "projects",
"acls": [
{
"principalType": "ROLE",
"principalId": "siteAdmin",
"permission": "ALLOW",
"property": [
"updateById"
]
},
{
"principalType": "ROLE",
"principalId": "$authenticated",
"permission": "ALLOW",
"property": [
"find",
"findById"
]
},
{
"accessType": "*",
"principalType": "ROLE",
"principalId": "$everyone",
"permission": "DENY",
"property": "*"
}
],
In config.json (this is new - feel free to adapt the terms and where this config sits to better fit into the loopback ecosystem):
{
"swagger": {
"hiddenAclPrincipleIds": [
"siteAdmin"
]
}
}
Resulting behaviour:
- Swagger shows
GET /projectsandGET /projects/:id- these are accessible to all logged in users - Swagger does not show
PUT /projects/:id. It's accessible to site admins (thanks to our role resolver), but hidden by the config setting - Swagger does not show any other routes. There's no way to access them thanks to our
DENY $everyone
Thoughts?
+1
@bajtos Maybe server/model-config.json is a better place to allow such customizations.
IMO, there are multiple aspects:
- What methods are defined to be
remoteable? (With remoting metadata) - What remote methods should be exposed to REST? (By default, all remoteable methods are)
- What remote methods should be exposed to Swagger? (By default. all remote methods from 2)
- How does ACL play a role in the visibility?
I believe we shouldn't use ACL to do this, as it can be dynamic and it will make it harder to reason about.
Perhaps... though:
- Surely the security of whether or not the routes can be called is the most important bit? (so if it's difficult to reason about this because the ACLs can be dynamic, then perhaps that's an issue in itself and is more important to resolve)
- If the ACLs and the visibility are totally disconnected, then I'm betting that's going even be an even bigger headache for reasoning about
- Normally we wouldn't have Swagger to generate the result of the dynamic rules. The beauty here is that we do, and so it would become very easy to review the result of these (and reason with the result)
That said, I'm not suggesting that my way is best. It'd be great if someone would review how the other frameworks do it (Rails, Yii2, Django, Laravel, etc.) and work out what the consensus is / what works best.
+1
FWIW, as of #1667, one can white/black-list methods in the model-config by setting
"options": {
"remoting": {
"sharedMethods": {
"*": false, // disable all methods
"create": true // enable (white-list) create only
}
}
}@voitau: as described by @bajtos in above solution, you can white/black list methods for a particular model in server/model-config.json file; it will look like, for a model named MyModel:
server/model-config.json
"MyModel": {
"dataSource": "db",
"public": true,
"options":{
"remoting": {
"sharedMethods": {
"*": false,
"create": true
}
}
}
},Closing now. Should you need any further assistance, please comment here and I'll be happy to help. Thank you.
"MyModel": {
"dataSource": "db",
"public": true,
"options":{
"remoting": {
"sharedMethods": {
"*": false,
"create": true
}
}
}
}
I can hide the related models end points like for example __get__tags because I have tried this:
"MyModel": {
"dataSource": "db",
"public": true,
"options":{
"remoting": {
"sharedMethods": {
"*": false,
"create": true,
"__get__tags": false
}
}
}
}
And it is now working?!
@ahmed-abdulmoniem : Hi, I tried to reproduce it and found that this functionality is limited to the model's methods only ATM. I'll create a new issue to expand the coverage of this feature to methods attached to model by applying relations.
Thank you for reporting. And, please feel free to submit a patch for this and we will help along the way to get it landed.
@gunjpan Thank you.
@gunjpan Another issue ..
Can we hide PATCH end points?
I see for example PATCH and PUT has the same method names but with index?! like upsert_0 and upsert?
When I hide upsert it hides both PATCH and PUT .. can we keep one of them and hide the others?
@ahmed-abdulmoniem : Added to #2860 .
@gunjpan Thanks
I'm not sure if this is the best place for this comment, but I found @EricPrieto's disableAllMethodsWithExceptions incredibly helpful. Thank you for documenting it! I received a deprecation warning for disableRemoteMethod when using it, so I rewrote the function to use the recommended disableRemoteMethodByName.
const disableAllMethodsWithExceptions = function disableAllMethods(model, methodsToExpose) {
if(model && model.sharedClass)
{
methodsToExpose = methodsToExpose || [];
const modelName = model.sharedClass.name;
const methods = model.sharedClass.methods();
const relationMethods = [];
const hiddenMethods = [];
try
{
relationMethods.push({ name: 'prototype.patchAttributes' });
Object.keys(model.definition.settings.relations).forEach(function(relation)
{
relationMethods.push({ name: 'prototype.__findById__' + relation });
relationMethods.push({ name: 'prototype.__destroyById__' + relation });
relationMethods.push({ name: 'prototype.__updateById__' + relation });
relationMethods.push({ name: 'prototype.__exists__' + relation });
relationMethods.push({ name: 'prototype.__link__' + relation });
relationMethods.push({ name: 'prototype.__get__' + relation });
relationMethods.push({ name: 'prototype.__create__' + relation });
relationMethods.push({ name: 'prototype.__update__' + relation });
relationMethods.push({ name: 'prototype.__destroy__' + relation });
relationMethods.push({ name: 'prototype.__unlink__' + relation });
relationMethods.push({ name: 'prototype.__count__' + relation });
relationMethods.push({ name: 'prototype.__delete__' + relation });
});
} catch(err) {}
methods.concat(relationMethods).forEach(function(method)
{
if(methodsToExpose.indexOf(method.name) < 0)
{
hiddenMethods.push(method.name);
model.disableRemoteMethodByName(method.name);
}
});
if(hiddenMethods.length > 0)
{
console.log('\nRemote methods hidden for', modelName, ':', hiddenMethods.join(', '), '\n');
}
}
};Should we add this (explanation and code for disableAllMethodsWithExceptions) to the documentation? If so, what would be the best place?
Why is prototype.patchAttributes treated separately? I noticed that disabling patchAttributes alone doesn't have any effect (anymore, was fine until a few weeks ago), but wonder if this the intended behaviour?
Should we add this (explanation and code for disableAllMethodsWithExceptions) to the documentation? If so, what would be the best place?
Not at the moment, This feature is partially complete, once implemented, should eliminate a need to write above code. We can document the feature at the time.
@gunjpan Cool--please keep me posted. I added the needs-doc label to remind us when we get to that point. Thanks.
I have refactored the code to be a little more DRY and also include a method for passing in an array of endpoints to disable.
'use strict';
const
relationMethodPrefixes = [
'prototype.__findById__',
'prototype.__destroyById__',
'prototype.__updateById__',
'prototype.__exists__',
'prototype.__link__',
'prototype.__get__',
'prototype.__create__',
'prototype.__update__',
'prototype.__destroy__',
'prototype.__unlink__',
'prototype.__count__',
'prototype.__delete__'
];
function reportDisabledMethod( model, methods ) {
const joinedMethods = methods.join( ', ' );
if ( methods.length ) {
console.log( '\nRemote methods hidden for', model.sharedClass.name, ':', joinedMethods, '\n' );
}
}
module.exports = {
disableAllExcept( model, methodsToExpose ) {
const
excludedMethods = methodsToExpose || [];
var hiddenMethods = [];
if ( model && model.sharedClass ) {
model.sharedClass.methods().forEach( disableMethod );
Object.keys( model.definition.settings.relations ).forEach( disableRelatedMethods );
reportDisabledMethod( model, hiddenMethods );
}
function disableRelatedMethods( relation ) {
relationMethodPrefixes.forEach( function( prefix ) {
var methodName = prefix + relation;
disableMethod({ name: methodName });
});
}
function disableMethod( method ) {
var methodName = method.name;
if ( excludedMethods.indexOf( methodName ) < 0 ) {
model.disableRemoteMethodByName( methodName );
hiddenMethods.push( methodName );
}
}
},
/**
* Options for methodsToDisable:
* create, upsert, replaceOrCreate, upsertWithWhere, exists, findById, replaceById,
* find, findOne, updateAll, deleteById, count, updateAttributes, createChangeStream
* -- can also specify related method using prefixes listed above
* and the related model name ex for Account: (prototype.__updateById__followers, prototype.__create__tags)
* @param model
* @param methodsToDisable array
*/
disableOnlyTheseMethods( model, methodsToDisable ) {
methodsToDisable.forEach( function( method ) {
model.disableRemoteMethodByName( method );
});
reportDisabledMethod( model, methodsToDisable );
}
};@dancingshell could you turn this into a module, maybe even a mixin?
Also, this does not hide Model.prototype.updateAttributes
temporary solution is model.disableRemoteMethod('updateAttributes', false)
@Discountrobot and @dancingshell --- converted to mixin:
You can expose methods in your model.json file:
{
"mixins":{
"DisableAllMethods":{
"expose":[
"find",
"findById"
]
}
}
}or hide specific methods:
{
"mixins":{
"DisableAllMethods":{
"hide":[
"create"
]
}
}
}https://gist.github.com/drmikecrowe/7ec75265fda2788e1c08249ece505a44
hi,
thanks for the gist :)
As proposed in #2953
What do you think of adding an option per method to decide whether the remote method should be made totally unavailable (in code and over rest), or just over rest?
What I chose to do (I just updated the Gist) is to not hide any methods that had explicit ACL's. So, if you granted an allow permission, then expose that method or remote method (based on these comments #651 (comment))
@drmikecrowe , all, please see this gist
I refactored bits and pieces to make the code more straightforward and compatible with any version of loopback including v3 where a number of function names change.
Especially, i simplified the whole <is this a static or is this a related instance method?> logic. The reason why you only get static model methods from Model.sharedClass.methods() in your code is that you don't wait for all models to be attached, this is easily fixed by waiting for app.on('started') event. The prototype. prefix appending is then decided against the isStatic property of the remote method. This way there's no need to establish beforehand a list of possible methods prefix, making it time proof.
I also added an option for the mixin to work even if not specifying any method to explicitly hide or expose
Also included is an index.js file to allow for mixin declaration
Note: the code now uses the disableMethodByName method from loopback v3, please adapt if your loopback version does not support it yet.
After all I bootstrapped a whole new mixin which is more compact (leveraging on lodash sugar):
the option to enable methods from ACLs is now configurable with enableFromACLs param (which defaults to true)
@ebarault Where did you spot enableFromACLs ? Not seen anything mentioned of it and searching Google and the codebase returns nothing...
@PaddyMann : it's from my own mixin code : here
you configure it in the mixin setup section of the Model definition : here
(although it's intended to bet set when expected false, as it defaults to true)
ah great stuff (sorry I misread your previous comment!)
I'll probably give your mixin a try in the next week or two :)
@ebarault: what would be the differences between using your gist and this module https://github.com/Neil-UWA/loopback-remote-routing ? (Are there any differences?) If there are differences, I would recommend creating your code as a npm module.
@c3s4r thanks for bringing this mixin through
essentially the difference is that the solution proposed in this discussion can automatically enable only the methods explicitly covered by static ACLs in the model configuration, so you don't have to enumerate them manually once more
yes, the idea is to port it as an npm module ultimately
I just finished creating a module for this. I changed the names of the options a little bit, and added unit tests. I manually tested on loopback 2 and 3, and it seems to be working fine.
https://www.npmjs.com/package/loopback-setup-remote-methods-mixin
I'm also thinking on adding an option for defining new remote methods as well. (Which will be deprecated when the Methods section is implemented for Loopback 3, but can be useful for Loopback 2).
Any comments or suggestions are welcome :)
As I said: I will be pushing the mixin as an npm module soon.
@c3s4r : although you most probably did this with all the best intentions :
- mention the contributors when publishing on npm, don't just publish as you own creation. I'm referring to the people who brought something from this discussion
- the way you refactored does not work for nonStatic methods named with
prototype.that are enabled from ACLs definition
@ebarault: Sorry, I did this on my best intentions, didn't want to steal the credit. I couldn't wait because I was needing it for a project, so I putted some effort on doing the unit tests, refactoring the code and publishing the module. Anyways,... let me know how you would like to proceed. I can mention you on the credits, or I can transfer you the project.
-> no pb @c3s4r : not my full credit at all either. We can add a list of contributors from this thread.
It remains that the implementation is broken for now. I'll reach out on your github repo to discuss these, we can co-author it if you will. Just give me a moment to move on with other pending stuff.
I just pushed the package to npm with the outstanding bug fixed. Also, I added @ebarault and @drmikecrowe as contributors and a credits section in the README file.
https://www.npmjs.com/package/loopback-setup-remote-methods-mixin
@ebarault I think it makes sense to create an issue for that at https://github.com/devsu/loopback-setup-remote-methods-mixin/issues
@ebarault: I'm aware of the limitation, I didn't see it like a big issue, anyways, if you think it should be changed, please open an issue on the other repo and let's continue the conversation there.
thanks all for the module, it is cool, but it is slow when you have many models and each of them has many relations. anything we can do about it?
@go2smartphone Something to deal with server.on('started', () => { ... }) ≈5 sec to process each model. Actually it works the same without looking if app is started (Loopback v2.36.0).
@go2smartphone @HyperC0der Please open a new issue describing the problem you are facing in detail, ideally with a small app reproducing it (see http://loopback.io/doc/en/contrib/Reporting-issues.html#bug-report).