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

[7397] onLogout should provide connection information #7433

Merged
merged 4 commits into from Aug 2, 2016

Conversation

Projects
None yet
5 participants
@Ragsboss

Ragsboss commented Jul 18, 2016

Fix for #7397. See commit description for more details. Added a new test and ran all existing tests.
@abernix could you please review this?

Raghavendra Rachamadugu Raghavendra Rachamadugu
[7397] onLogout should provide connection information
Adding an argument to onLogoutCallback. The callback is an object with two properties - userId and connection. I thought of loading up the user and instead have the user property, but felt like this maybe redundant. Logout hooks can always do this if they need. However one advantage of having user instead of userId is it makes it symmetric/consistent with onLogin callback. Let me know if you strongly feel we should have user instead userId..

Also extended an existing test to validate the onLogout callback gets this argument with expected user id
this._onLogoutHook.each(function (callback) {
callback();
callback(logoutContext);

This comment has been minimized.

@benjamn

benjamn Jul 19, 2016

Member

I would just call callback({ userId, connection }) here, to provide a defensive copy of the context information to each callback.

This comment has been minimized.

@Ragsboss
@abernix

This comment has been minimized.

Member

abernix commented Jul 19, 2016

@Ragsboss In addition to the comments above, how do you feel about having the onLogin receiving the same parameter (logoutContext) for consistency between the two going forward?

@Ragsboss

This comment has been minimized.

Ragsboss commented Jul 20, 2016

@abernix changing onLogin's current argument would be a breaking change. I'm not sure if this is ok? In fact (as I mentioned in my commit message for first commit), I was wondering if I should instead pass logoutContext as { user, connection } instead of { userId, connection } to make it consistent with existing context (aka attempt in the code) that's passed to onLogin callbacks. As I think more this makes more sense (saving on one user load is too much of a micro optimization and not worth the inconsistency imo). So I changed the code to pass user instead of userId. Take a look and let me know what you think. For reference the onLogin callback receives an object like this based on my tests.
{"type":"password","allowed":true,"methodName":"login","methodArguments":[{"user":{"username":"admin@abc.com"},"password":{"digest":"d6b5d69d3c396de3ffb35077d0bbee33b4605eb9cabf251af305716e61","algorithm":"sha-256"}}],"user":{"_id":"QoxkdfReN8RpnfBKD","createdAt":"2016-07-16T00:34:47.179Z","services":{"password":{"bcrypt":"$2a$10$tRPe0c40CI7c/1MfK9a9fePXQYvHGmr8sh88q/ZGYMAAl3ACLBWve"},"resume":{"loginTokens":[]}},"username":"admin@abc.com","profile":{"firstname":"Admin"}},"connection":{"id":"mr2SQXqQyWeoHMuyi","clientAddress":"127.0.0.1","httpHeaders":{"referer":"https://192.168.56.103/","x-forwarded-for":"127.0.0.1","x-real-ip":"192.168.56.1","x-forwarded-proto":"http","host":"192.168.56.103","user-agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36","accept-language":"en-US,en;q=0.8"}}}

Raghavendra Rachamadugu
[7397] Change argument to onLogout callback to be consistent with onL…
…ogin argument

onLogin argument is an object that has user and connection fields among other fields. So I'm making onLogout also have user and connection fields to make things consistent.

Other fields that onLogin callbacks receive include type, allowed, methodName, methodArguments - which are not available or applicable for onLogout.

abernix added a commit to abernix/meteor-docs that referenced this pull request Jul 20, 2016

Document parameters for the Accounts onLogout
This depends on meteor/meteor#7433 getting merged and shows the object which will be passed to onLogout on the server when a user logs out.
@abernix

This comment has been minimized.

Member

abernix commented Jul 20, 2016

You're right, we wouldn't want that to be a breaking change. I was mainly rooting for getting the connection and user parameters to be the same – which you've done. I think the resolution of userId to user to achieve this consistency is certainly worth it. Once this is merged, meteor/docs#66 should cover the documentation for this (if you want to take a look).

LGTM.

@Ragsboss

This comment has been minimized.

Ragsboss commented Jul 20, 2016

@benjamn and @abernix thank you for your review and comments. I'm not sure what the process is now. Who can merge this pull request to meteor/devel?

@abernix

This comment has been minimized.

Member

abernix commented Jul 21, 2016

Magical pull-request gnomes will come through and make sure we're all thinking clearly. Give it some time, but they will come. 🍪

(In seriousness, it will be reviewed by the core team in due time – generally within a week)

@@ -176,9 +176,10 @@ Ap._failedLogin = function (connection, attempt) {
});
};
Ap._successfulLogout = function () {
Ap._successfulLogout = function (connection, userId) {
const user = userId && this.users.findOne(userId);

This comment has been minimized.

@mitar

mitar Aug 1, 2016

Collaborator

Are we sure we want to always make a database query here? Why wouldn't we just pass userId and leave to the developer to fetch the user document if they want? I think this is not necessary to do automatically.

This comment has been minimized.

@mitar

mitar Aug 1, 2016

Collaborator

(Moreover, you are fetching the user object with all fields, which can potentially be large.)

This comment has been minimized.

@Ragsboss

Ragsboss Aug 1, 2016

This is to be consistent with other handlers like onLogin.

This comment has been minimized.

@mitar

mitar Aug 1, 2016

Collaborator

I do not see user to be passed to onLogin? connection is cheap, because it is already available. But user object is not.

This comment has been minimized.

@Ragsboss

Ragsboss Aug 1, 2016

onLogin does get an object with user property. See following code. This is also mentioned in the documentation and we indeed use this property in our project.

var user;
if (result.userId)
user = this.users.findOne(result.userId);

var attempt = {
type: result.type || "unknown",
allowed: !! (result.userId && !result.error),
methodName: methodName,
methodArguments: _.toArray(methodArgs)
};
if (result.error)
attempt.error = result.error;
if (user)
attempt.user = user;

@tmeasday tmeasday self-assigned this Aug 2, 2016

@tmeasday

This comment has been minimized.

Contributor

tmeasday commented Aug 2, 2016

Great work @Ragsboss!

@tmeasday tmeasday merged commit f89dc15 into meteor:devel Aug 2, 2016

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tmeasday added a commit to meteor/docs that referenced this pull request Aug 3, 2016

Document parameters for the Accounts onLogout
This depends on meteor/meteor#7433 getting merged and shows the object which will be passed to onLogout on the server when a user logs out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment