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

The quest for a better User.login #161

Closed
bajtos opened this issue Jan 29, 2014 · 12 comments · Fixed by #163
Closed

The quest for a better User.login #161

bajtos opened this issue Jan 29, 2014 · 12 comments · Fixed by #163
Assignees

Comments

@bajtos
Copy link
Member

bajtos commented Jan 29, 2014

While implementing client SDKs consuming the new ACL/Auth features, we have found that client apps usually need to get details of the currently logged in user. At the moment, clients have to send two requests: User.login and User.get.

We should modify User.login so that it optionally adds the user data to the response.

Objectives

  1. Backwards compatibility. /users/login should return the same response as before this change.
  2. Opt-in scheme. Clients have to explicitly request the inclusion of user data.

/to @raymondfeng @ritch @Schoonology let's discuss what's the best solution. I'll describe few of them in a comment.

@ghost ghost assigned bajtos Jan 29, 2014
@ritch
Copy link
Member

ritch commented Jan 29, 2014

Using the existing include semantics for Model.find:

User.login({include: 'user'}, function(err, result) {
  console.log(result.user); // => current user
});

@bajtos
Copy link
Member Author

bajtos commented Jan 29, 2014

An alternate solution:

Assume that the decision whether User.login should include user data can be made on per-application level, thus all clients should be receiving the same kind of the response.

Implementation-wise, we need to make the remoting metadata of User.login configurable.

  1. AccessToken only

    returns: {arg: 'accessToken', type: 'object', root: true}
  2. AccessToken + User

    returns: [
      {arg: 'accessToken', type: 'object'},
      {arg: 'user', type: 'object'},
    ]

The implementation of User.login can stay the same, it can always send both accessToken and user to the callback.

@bajtos
Copy link
Member Author

bajtos commented Jan 29, 2014

@ritch That was the second option I wanted to mention. So the response will contain AccessToken properties + user, is that correct?

@ritch
Copy link
Member

ritch commented Jan 29, 2014

Yep

@bajtos
Copy link
Member Author

bajtos commented Jan 29, 2014

BTW the solution for this won't be trivial.

The naive implementation does not work, as Model.toJSON seems to ignore custom properties.

user.accessTokens.create({
  ttl: Math.min(credentials.ttl || User.settings.ttl, User.settings.maxTTL)
}, function(err, res) {
  res.user = user;
  fn(err, res);
});

I am also not sure if this will work out of the box with #160 (the nested User object should not include password and other sensitive data).

I was thinking about a different approach:

User.login({include: 'user'}, function(err, token, user) {
  // user is undefined when include:'user' was not specified
});

// remoting metadata
returns: [
  {arg: 'accessToken', type: 'object', root: true},
  {arg: 'user', type: 'object'},
]

This does not work in the current implementation, but the fix should be easy. (See SharedMethod.toResult).

@ritch
Copy link
Member

ritch commented Jan 29, 2014

Looks fine to me.

@bajtos
Copy link
Member Author

bajtos commented Jan 30, 2014

Oh well, my approach does not work either. At the end, the complex returns definition boils down to the same code being executed: result.user = user.

I have found a hacky workaround (see #162 for details), which I will probably use for now:

// inside User.login
if (include === 'user')
  token.__data.user = user;

There is one more thing we need consider: since AccessToken belongsTo User, it already has a user property of type function (see loopback-datasource-juggle/lib/relations.js#L244).

If User.login overrides this property value then the code calling User.login directly may stop working.

User.login(credentials, function(err, token) {
  token.user(function(err, user) {
    // do something
  });
});

I feel there should be a better way for exposing the belongsTo models in the JSON response, possibly similar to the solution in loopbackio/loopback-datasource-juggler#64. @raymondfeng could you advise?

@bajtos
Copy link
Member Author

bajtos commented Jan 30, 2014

Let's continue the discussion about AccessToken belongsTo User in #165.

@lukewendling
Copy link

So, if I want to get the user from the access token, I should use accessToken.__data.user ?

@HarisHashim
Copy link

HarisHashim commented Sep 22, 2017

I have the same question. Using accessToken.__data.user as in

console.log("User: %j", accessToken.__data.user);

Did give me the json for user. But why not accessToken.user ?

I suspect the answer is:

There is one more thing we need consider: since AccessToken belongsTo User, it already has a user property of type function (see loopback-datasource-juggle/lib/relations.js#L244).

But is there better or less hackish way to do this instead of accessToken.__data.user ?

@rahulrsingh09
Copy link

@bajtos @ritch Loopback says include.toLowerCase is not a function\

@bajtos
Copy link
Member Author

bajtos commented Oct 6, 2017

@lukewendling @HarisHashim

But is there better or less hackish way to do this instead of accessToken.__data.user ?

I think if the related user data has been already stored in accessToken.__data.user, then you may be able to retrieve it by calling accessToken.user(). I am not sure though. Let's move this discussion to #165 please.

@rahulrsingh09

Loopback says include.toLowerCase is not a function\

I am afraid I have no idea what you are writing about. Please open a new issue and provide us with enough details to reproduce the problem you are encountering, see http://loopback.io/doc/en/contrib/Reporting-issues.html#bug-report

@strongloop strongloop locked and limited conversation to collaborators Oct 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants