Skip to content
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

Move /models /routes to explorer middleware #1134

Closed
ritch opened this issue Feb 25, 2015 · 13 comments
Closed

Move /models /routes to explorer middleware #1134

ritch opened this issue Feb 25, 2015 · 13 comments
Assignees
Labels

Comments

@ritch
Copy link
Member

ritch commented Feb 25, 2015

This issue raises the concern for these routes always being available. They should not be available from a vanilla use of loopback.rest(). Instead whatever is using them (I'm assuming explorer) should add them or turn them on.

@bajtos
Copy link
Member

bajtos commented Feb 25, 2015

Note: loopback-explorer does not use this routes, it has it's own routes for ages. If there was not a backwards-compatibility concern, I would say the routes can be immediately deleted.

I am proposing to add a flag that will control whether these routes are added or not. This new flag should be:

  • Enabled by default to preserve backwards compatibility.
  • Deprecated from the beginning, so that users are warned about this problem. Use depd for deprecation, that's what express uses too.
  • Disabled in the configuration scaffolded by slc loopback via loopback-workspace. That way new applications come without these routes.

@greaterweb
Copy link
Contributor

Thanks guys for looking into this.

I can take a stab at the first two items @bajtos has suggested. Depending on time, I might be able to look into the loopback-workspace one as well.

I'll be sure to include a few tests as good measure.

@greaterweb
Copy link
Contributor

Enabled by default to preserve backwards compatibility.

Trying to think through a config option that is meaningful and concise for this one, two options

  • disableLegacyExplorer - (boolean, false by default) seems appropriate but is a bit lengthy - true disables the routes, false or omitting the option keeps them enabled
  • legacyExplorer - (boolean, true by default) this shorter option seems better - false disables the routes, true or omitting the option keeps them enabled

Thoughts?

@greaterweb
Copy link
Contributor

Work completed to resolve this in PR. Though I wonder if we should improve the middleware to limit only GET requests.

@bajtos
Copy link
Member

bajtos commented Feb 26, 2015

+1 for legacyExplorer, disableLegacyExplorer=false is a double negative which is better to avoid.

Depending on time, I might be able to look into the loopback-workspace one as well.

This part is easy, just add a new entry to templates/components/api-server/component.js and a new unit-test to test/end-to-end.js.

@greaterweb
Copy link
Contributor

@bajtos, no problem. I can make the update to the loopback-workspace project

greaterweb added a commit to greaterweb/loopback-workspace that referenced this issue Feb 26, 2015
Add option to generated config file for the new legacyExplorer
option introduced as a fix to strongloop/loopback#1134. This
will ensure that the /routes and /models routes are disabled
by default.
@crandmck
Copy link
Contributor

crandmck commented Mar 5, 2015

Does this require any doc updates or additions?

Currently, to disable API Explorer, docs just say to delete explorer.js boot script: http://docs.strongloop.com/display/LB/Security+considerations#Securityconsiderations-DisablingAPIExplorer Will that change with this?

How is the legacyExplorer option exposed?

@greaterweb
Copy link
Contributor

The instructions for disabling the API Explorer remain valid. This issue was to address two routes remaining exposed which have not been used by the API Explorer for some time.

The legacyExplorer option is an addition to the the config.json file. When making the upgrade to v2.14.0, it's is a good idea to add "legacyExplorer": false to your config. I'm sure we'll see an update soon to the config.json documentation.

@crandmck
Copy link
Contributor

crandmck commented Mar 5, 2015

I'm sure we'll see an update soon to the config.json documentation.

Heh, that would be my job ;-)
Please review to see if I explained it correctly:
http://docs.strongloop.com/display/LB/config.json#config.json-Top-levelproperties
Thanks!

@greaterweb
Copy link
Contributor

Ha! Sorry, I did not know :-)

Thanks for all of the great work documenting, I imagine it's a decent challenge keeping up with all StrongLoop has going on!

Your addition to the docs looks good, two small changes though:

Set to false to disable old routes /models and /routes routes that are exposed, but no longer used by API Explorer; use true or omit the option to keep them enabled.

Thanks!

@crandmck
Copy link
Contributor

crandmck commented Mar 6, 2015

Thanks for all of the great work documenting, I imagine it's a decent challenge keeping up with all StrongLoop has going on!

You're welcome... and, yes, it does keep me quite busy :-)

Thanks for catching the typo! Fixed...

greaterweb added a commit to greaterweb/loopback-workspace that referenced this issue Mar 26, 2015
Add option to generated config file for the new legacyExplorer
option introduced as a fix to strongloop/loopback#1134. This
will ensure that the /routes and /models routes are disabled
by default.
bajtos pushed a commit to strongloop/loopback-workspace that referenced this issue Mar 27, 2015
 * Remove deprecation warnings (Miroslav Bajtoš)

 * Add unit-test verifying top-level CORS setup (Miroslav Bajtoš)

 * Setup a single top-level CORS middleware (claylo)

 * Disable legacy explorer routes by default. Add option to generated config file for the new legacyExplorer option introduced as a fix to strongloop/loopback#1134. This will ensure that the /routes and /models routes are disabled by default. (Ron Edgecomb)
dotlouis added a commit to dotlouis/StudLoop that referenced this issue Apr 25, 2015
- Removed afterSave() model hook because:
1. it was depreciated in favor of Operation hooks
(http://docs.strongloop.com/display/public/LB/Operation+hooks)
2. it was creating cyclic calls since calling identity() would trigger
the afterSave()
==> so it's the job of the client to call the remote identity() method

- Removed dummy-data creation in bootscript.
Insertion of such data can be done via the API explorer

- legacyExplorer : false (see
strongloop/loopback#1134)
@bajtos
Copy link
Member

bajtos commented Apr 30, 2015

I am closing this issue as fixed by #1135.

@ritch feell free to reopen if you have any objections.

@bajtos bajtos closed this as completed Apr 30, 2015
@gausie
Copy link
Contributor

gausie commented Jul 29, 2015

It would be good to be able to hit a model endpoint with the OPTIONS verb and get the equivalent of its entry in /models.

reactDev037 added a commit to reactDev037/Workspace-backend that referenced this issue Sep 5, 2018
Add option to generated config file for the new legacyExplorer
option introduced as a fix to strongloop/loopback#1134. This
will ensure that the /routes and /models routes are disabled
by default.
reactDev037 added a commit to reactDev037/Workspace-backend that referenced this issue Sep 5, 2018
 * Remove deprecation warnings (Miroslav Bajtoš)

 * Add unit-test verifying top-level CORS setup (Miroslav Bajtoš)

 * Setup a single top-level CORS middleware (claylo)

 * Disable legacy explorer routes by default. Add option to generated config file for the new legacyExplorer option introduced as a fix to strongloop/loopback#1134. This will ensure that the /routes and /models routes are disabled by default. (Ron Edgecomb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants