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

A way to disable configureLoginService method altogether #7745

Closed
mitar opened this issue Sep 1, 2016 · 6 comments
Closed

A way to disable configureLoginService method altogether #7745

mitar opened this issue Sep 1, 2016 · 6 comments
Labels
confirmed We want to fix or implement it Type:Feature

Comments

@mitar
Copy link
Contributor

mitar commented Sep 1, 2016

I do not like how there is a configureLoginService method available which allows configuring service the first time without any permission checks. This means that anyone who installs my app and does not configure all services my app otherwise integrates (and uses their packages) can get their services configured by somebody, no? And this could then open others to login into the app using a different authentication mechanism then expected.

@hwillson
Copy link
Contributor

hwillson commented May 30, 2017

Hi all - just reviewing this issue a bit. Currently, there are safeguards in place to prevent the following (extracted from 3720106):

  • Alice launches site with Facebook login
  • Mallory sends configureLoginService method to configure the Twitter service
  • Alice runs "meteor add accounts-twitter" and is impressed that Twitter integration
    Just Works with no configuration
  • Now the app is using Mallory's credentials

The problem that remains (as I understand it) is covered under the following scenario:

  • Alice ships an app (that others can install themselves) with the accounts-twitter package installed, but no initial service configuration has been completed (since the idea is that people who install the app will take care of this themselves).
  • Bob installs a copy of Alice's app without taking care of the initial Twitter service configuration.
  • Mallory notices this and fires in a configureLoginService method to configure the Twitter service using his credentials
  • Pwnage?

Given the above scenario, how would we go about locking this down? The configureLoginService Method exists to help facilitate wizard-style UI's for the initial service configuration, so locking it down further could hamper this feature. I know this issue has been marked as pull-requests-encouraged, but I don't think we have enough of a plan broken out here to help contributors.

@mitar
Copy link
Contributor Author

mitar commented May 30, 2017

(I took the liberty of replacing Mallory and Bob names in above scenario, because in many such scenarios Mallory stands for a malicious one.)

@mitar
Copy link
Contributor Author

mitar commented May 30, 2017

Yes, the above scenario explains it well. The existing code does not seem to protect it.

What I would do is simple allow one to ship an app without configureLoginService enabled. This is what me as an app developer would want. That I can ship an app without it, and provide my own admin panel with my own configuration for admins (which I am already doing) and then I can check permissions in my custom way to allow only people in admin group or something to do it.

Personally, I would do it so that if no -ui package (with Blaze ;-) ) for accounts is present, this method is disabled. If you add any such package, which provides easy configuration, then you get this fancy method. Otherwise, no method, no ui (no Blaze). I think now that we moved ui out of base accounts packages because of removing Blaze, we could also move out this method. So developers have to opt-in to use it.

For myself, I would also be OK with opting out. But that is less safe in general if you do not know about it.

@hwillson
Copy link
Contributor

Thanks @mitar - I prefer the opt-in approach as well. This sounds great!

@mitar
Copy link
Contributor Author

mitar commented May 31, 2017

So, just to be clear, opt-in has slight backwards incompatibility if you have been relying on it being present in the base package, but not using ui package. But I think it is worth it.

@hwillson
Copy link
Contributor

hwillson commented Jun 6, 2017

To help provide a more clear separation between feature requests and bugs, and to help clean up the feature request backlog, Meteor feature requests are now being managed under the https://github.com/meteor/meteor-feature-requests repository.

Migrated to meteor/meteor-feature-requests#13.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
confirmed We want to fix or implement it Type:Feature
Projects
None yet
Development

No branches or pull requests

3 participants