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

Allow runtime modification of accounts-oauth services set #4014

Closed
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@kentonv
Contributor

kentonv commented Mar 25, 2015

Hi all,

For Sandstorm.io, we would like the server administrator to be able to dynamically configure which login services are available. We ship an already-built server bundle to users who deploy on their own hardware, so it's inconvenient to tell people to rebuild with different Meteor packages. In fact, we'd like to have an admin UI that allows the administrator to toggle services on and off without even restarting the server. It seems like this is probably a common need for Meteor apps that are designed to have many instances managed by different people.

This PR is a proposal which seems to accomplish what we want, but please let us know if there is a better way.

In this PR, we add a method to remove login services from the registered set, which is most of what we need. We also add a check at login time to reject logins using a deregistered service. Otherwise, because packages like google and github are always resident in our server even if not enabled, people would presumably be able to log in with them by manually sending the right requests.

It looks like the code in accounts-base will already reject calls to configureLoginService for a service name not in the serviceNames() set, which is great because we wouldn't want arbitrary users to be able to configure services that the admin left disabled (and presumably never configured).

Separately from this PR, it looks like we'll need to forgo the accounts-{google,github,...} packages and instead depend directly on {google,github,...} and implement the accounts-oauth registration part directly. Fortunately this appears to be only a couple lines of code for each service. It would be nice to see this become more controllable in the future, though.

Thoughts?

-Kenton

PS. We'd love to add a test, but this package doesn't appear to have any tests, and it's not really clear how tests should be written. Please let us know if there is a good place to put them.

Add Accounts.oauth.deregisterService method
Also change the Accounts.registerLoginHandler callback for
accounts-oauth to error out on non-registered services.
@glasser

This comment has been minimized.

Member

glasser commented Mar 26, 2015

Hmm. If you're not going to be able to achieve your goals using accounts-{google,github,...}, then how does this change help you? Can't you just only call Accounts.oauth.registerService conditionally in that case?

I mean, this is a pretty small change and I don't see much of a downside... but I don't see how it helps you if the point is to work around the automatic top-level Accounts.oauth.registerService lines anyway.

Or am I misinterpreting "Separately from this PR" and you mean "if we can't use this PR, then"?

@glasser

This comment has been minimized.

Member

glasser commented Mar 27, 2015

Also, one tricky bit is that you need to synchronize your runtime deregister between server and client. We synchronize the register basically by just making it always happen if the package is loaded. Presumably you've solved this issue in your app, but it will be important to be clear that consumers of this api have to do this themselves.

@kentonv

This comment has been minimized.

Contributor

kentonv commented Mar 27, 2015

Two answers:

  1. We'd like for our admin settings UI to be part of the Sandstorm UI, and it would be nicer if the server did not have to restart after every change. Admittedly this is a fairly minor convenience, since admins probably won't be changing these settings often.
  2. The change to the login handler seems important regardless, though I'm not completely sure. Since we'll have e.g. the google package loaded even when Google login is disabled, it seems like a client could manually-craft requests to log in using Google unless we explicitly check in the login handler. I assume this would require that the Google login has been configured, but it's possible the admin configured it previously and then later disabled it. I suppose we could purge the oauth configuration for a service when it is disabled, though that seems like a fragile way to prevent login compared to an explicit check, and would be inconvenient for the admin if they only intended to temporarily disable it.

@glasser glasser closed this in b439bca Mar 27, 2015

@glasser

This comment has been minimized.

Member

glasser commented Mar 27, 2015

That's convincing!

@kentonv

This comment has been minimized.

Contributor

kentonv commented Mar 28, 2015

Thanks!

Looks like someone commented that maybe the name should have been "unregister" rather than "deregister". In retrospect I sort of agree that blue is a better color for the bike shed, but we're happy either way. :)

@glasser

This comment has been minimized.

Member

glasser commented Mar 28, 2015

Good call, and in fact there is already an unregisterService for the corresponding non-accounts- call.

glasser added a commit that referenced this pull request Mar 28, 2015

@rclai

This comment has been minimized.

rclai commented Mar 29, 2015

Cool thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment