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.

Sign up for GitHub

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

Jump to bottom

Use _attemptLogin to try to login if accounts-base is available #24

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use _attemptLogin to try to login if accounts-base is available #24

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0bfd16
Use _attemptLogin to try to login if accounts-base is available.
mitar Apr 21, 2016
95cc968
Merge branch 'master' into use-attempt-login
mitar Aug 20, 2016
8b61fdd
Merge branch 'master' into use-attempt-login
mitar May 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ if (__meteor_runtime_config__.SANDSTORM) {
return true;
});
Package["accounts-base"].Accounts.validateNewUser(function (user) {
if (!user.services.sandstorm) {
if (!(user.services && user.services.sandstorm)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also fixed this.

throw new Meteor.Error(403, "Non-Sandstorm login mechanisms disabled on Sandstorm.");
}
return true;
Expand All @@ -54,6 +54,19 @@ if (__meteor_runtime_config__.SANDSTORM) {

if (Package["accounts-base"]) {
Meteor.users._ensureIndex("services.sandstorm.id", {unique: 1, sparse: 1});

// Sandstorm does not use login tokens so we override login method to not use them.
// Because when this package is enabled no other login method is allowed
// (see validateLoginAttempt callback above) this should not break anything.
Package["accounts-base"].Accounts._loginUser = function (methodInvocation, userId, stampedLoginToken) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm uncomfortable with this level of monkey-patching. Calling private methods is bad but largely unavoidable due to lack of public APIs... but replacing private methods is going too far IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I explain in the comment, I do not think this interferes with anything.

This whole package hijacks the whole normal login process anyway. So hijacking one method more...

I think this makes much better integration for apps which use accounts-base. But it is your call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easy to imagine Meteor renaming _loginUser -- it would be their right to do so and expect no breakage. I'm not sure what will happen to accounts-sandstorm then -- will break loudly, or will it silently have different behavior?

accounts-sandstorm isn't really "hijacking" the login process, it's implementing its own login process -- something Meteor is actually designed to allow. The lower levels of Meteor are designed to be independent of accounts-base.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make a test that if Package["accounts-base"].Accounts._loginUser does not exist before assignment it throws an error.

BTW, Meteor is pretty sensitive to changes even to internals. For example, when they by accident released one change to internals recently which broke some expectations of internals user by 3rd party package they quickly reverted their change and released it without, to have a next time full cycle of people being able to test this. Otherwise we could just have tests to check for all this behavior and we would be warned in time.

Anyway, maybe then we can introduce an options here or two packages with different level of integration, so that users can decide which one to use. I personally would prefer full integration with Meteor and based on my experience with Meteor I would not worry so much of using internal methods.

But, maybe the real issue here is that this package does not have any tests.

// We misused stampedLoginToken argument to pass the "sandstorm" object to this method.
methodInvocation.connection._sandstormUser = stampedLoginToken;
methodInvocation.setUserId(userId);

// We do not return anything because we do not have anything to add to the
// Sandstorm user info object which is being returned through options.
return {};
};
}

Meteor.onConnection(function (connection) {
Expand Down Expand Up @@ -86,19 +99,31 @@ if (__meteor_runtime_config__.SANDSTORM) {
delete logins[token];
}

// Set connection info. The call to setUserId() resets all publishes. We update the
// connection's sandstorm info first so that when the publishes are re-run they'll see the
// new info. In theory we really want to update it exactly when this.userId is updated, but
// we'd have to dig into Meteor internals to pull that off. Probably updating it a little
// early is fine?
//
// Note that calling setUserId() with the same ID a second time still goes through the motions
// of restarting all subscriptions, which is important if the permissions changed. Hopefully
// Meteor won't decide to "optimize" this by returning early if the user ID hasn't changed.
this.connection._sandstormUser = info.sandstorm;
this.setUserId(info.userId);

return info;
if (Package["accounts-base"]) {
return Package["accounts-base"].Accounts._attemptLogin(this, "loginWithSandstorm", arguments, {
userId: info.userId,
type: "sandstorm",
// Misusing stampedLoginToken to pass "sandstorm" object to _loginUser method.
stampedLoginToken: info.sandstorm,
// Options are returned if login is successful.
options: info
});
}
else {
// Set connection info. The call to setUserId() resets all publishes. We update the
// connection's sandstorm info first so that when the publishes are re-run they'll see the
// new info. In theory we really want to update it exactly when this.userId is updated, but
// we'd have to dig into Meteor internals to pull that off. Probably updating it a little
// early is fine?
//
// Note that calling setUserId() with the same ID a second time still goes through the motions
// of restarting all subscriptions, which is important if the permissions changed. Hopefully
// Meteor won't decide to "optimize" this by returning early if the user ID hasn't changed.
this.connection._sandstormUser = info.sandstorm;
this.setUserId(info.userId);

return info;
}
}
});

Expand Down