disableRemoteMethod does not recognize method aliases

[Overdrivr]

Could you please clarify why in ACL definition, deleteById is used versus in the API it is destroyById ?

It is rather confusing when defining acls for this specific method, people (understand: me, probably others) will try to use destroyById because of this documentation while the correct keyword is deleteById. Documentation even mentions removeById.

Is it planned to have more consistency on the topic ?

[faridnsh]

They are alias, you can use any. Though it's not mentioned anywhere...

[Overdrivr]

I disagree, when writing ACL I believe they're not, 'destroyById' does not resolve anything.

Also what is the motivation for having an alias ? it's rather confusing.

[gunjpan]

@Overdrivr : Hi, as @alFReD-NSH mentioned, destroyById and deleteById are aliases for removeById. Check this code: dao.js

For the ACL, I'm checking if and where consistency is required! Thank you.

[mrbatista]

@gunjpan the problem is when you define remote method. See this definition as an example. Some time ago I had the same problem and I solved by referring to the code. Why don't modify remoteMethod to accept String or Array<String>?

[mrbatista]

@gunjpan this for default CRUD model. For some model alias is defined, like this, but function remoteMethod not use this option.
It seems that the aliases method is used for generate $resource for loopback-sdk-angular. See this.

[gunjpan]

@bajtos : Could you PTAL!

[bajtos]

I am confused, what is the problem you are trying to address here? Can somebody please provide a code snippet showing what you are trying to achieve, explain what was the (unexpected) actual result and what you would like to see instead?

[bajtos]

Can somebody please provide a code snippet showing what you are trying to achieve, explain what was the (unexpected) actual result and what you would like to see instead?

A failing unit-test would be even better.

[Overdrivr]

@bajtos ok, I will try to make the failing unit-test. keep you posted

[Overdrivr]

@bajtos I have worked on this issue. I made a repo https://github.com/Overdrivr/loopback-consistency-test for performing e2e tests.

During testing, I found something that I believe does not agree with documentation, that prevents me to test the actual issue in this ticket.

If I create two models foo_deleteById and bar.

• foo_deleteById : ACL deny all to everyone, grant create to $authenticated, grant deleteById to $authenticated
• bar: no ACL

Then a relation foo_deleteById hasMany bar.

At this point, using an authenticated user, I can create a related model using a related method POST foos/1/bars WITHOUT having explicitely granted remote methods.

See the test : https://github.com/Overdrivr/loopback-consistency-test/blob/master/test/test-delete.js#L49
The foo_deleteById model json : https://github.com/Overdrivr/loopback-consistency-test/blob/master/common/models/foo-delete-by-id.json

I will check again all the tests and setup, but I cannot find why this test is passing. Or maybe documentation is wrong on this point ?

Hope this helps and makes sense

[bajtos]

@raymondfeng @ritch you are much more familiar with ACLs than I am. Could you please help here?

[Overdrivr]

Any updates ?

I can confirm that I am seeing inconsistencies for hiding methods.

For instance, in a model script

MyModel.disableRemoteMethod('destroyById', false);
MyModel.disableRemoteMethod('removeById', false);

Do not hide the method. Only deleteById does work

MyModel.disableRemoteMethod('deleteById', true);

[gunjpan]

@Overdrivr : I believe here that the method aliases are not identified from the list of methods to disable it.
I am escalating as a bug and we will get it prioritized.

[bajtos]

This should be a relatively easy fix in strong-remoting, we need to fix SharedClass.prototype.isMethodEnabled to check all aliases, not only the primary method name:

https://github.com/strongloop/strong-remoting/blob/d2037311b379b36f781a091379f1a4da927b629d/lib/shared-class.js#L144-L154

Are there any volunteers willing to contribute this bug fix?

[Overdrivr]

Sounds easy enough, I can tackle this issue. I'll make a PR then

[Overdrivr]

Fixed via strongloop/strong-remoting#385

[Overdrivr]

I am still facing this issue with latest version of loopback and strong-remoting (2.33.0) despite the fix being in this version. I'm guessing that fixing behavior in strong-remoting was not sufficient.

+-- loopback@2.38.3
| +-- bcryptjs@2.4.3
| +-- body-parser@1.17.2
| +-- canonical-json@0.0.4
| +-- cookie-parser@1.4.3
| +-- debug@2.6.8
| +-- depd@1.1.0
| +-- ejs@2.5.6
| +-- errorhandler@1.5.0
| +-- express@4.15.3
| +-- inflection@1.12.0
| +-- isemail@1.2.0
| +-- loopback-connector-remote@1.3.3
| +-- loopback-context@1.0.0
| +-- loopback-phase@1.4.1
| +-- nodemailer@2.7.2
| +-- nodemailer-stub-transport@1.1.0
| +-- stable@0.1.6
| +-- strong-globalize@2.8.4
| +-- strong-remoting@2.33.0
| +-- uid2@0.0.3
| `-- underscore.string@3.3.4

I will try to make a failing test here in this repository.

[Overdrivr]

The issue remains on related models,

this works:

MyModel.disableRemoteMethod('__destroyById__relatedModel', false);

while this doesnt:

MyModel.disableRemoteMethod('__deleteById__relatedModel', false);
MyModel.disableRemoteMethod('__removeById__relatedModel', false);

[bajtos]

I think this is the same issue that was fixed in 3.x versions by #3565:

However, if a related model is attached after the initial model is configured, the globs do not work. I have updated to reconfigure shared method properties after each new model is attached to remoting.

Unfortunately the backport of #3565 is not trivial because the codebases have diverged and we don't have enough bandwidth to spend more time on this.

@Overdrivr could you perhaps take a look at #3565, extract the bits that are relevant to 2.x and open a new pull request to fix the problem you are experiencing?

[Overdrivr]

@bajtos I'm facing the issue with all my models and relations defined in .json files.
If I understand #3565 correctly, glob issues appear for relations configured manually in a .js file after server has booted. But not if the relations are defined in the config and initialized during boot.

So I'm not sure this is the problem here. Does this makes sense to you and is it correct ?

[bajtos]

@Overdrivr Your description makes sense to me, but I cannot tell whether your issue is or is not different from what #3565 fixed. I am afraid I don't bandwidth to investigate this problem myself. You can try to upgrade your app to LoopBack 3.x to see if the problem goes away, or you can debug through the code to build a better understanding yourself.

[stale]

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.

[stale]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.