strongloop / loopback-example-access-control

An example demonstrating LoopBack access control mechanisms.
Other
370 stars 168 forks source link

Fix for missing username in project view. #36

Closed mikehedman closed 9 years ago

mikehedman commented 9 years ago

login() returns a token, not the logged in user, so retrieving the user to get the username.

mikehedman commented 9 years ago

I've submitted a better fix for this in #37.

superkhau commented 9 years ago

Thanks for you contributions @mikehedman. I've spoke to @raymondfeng about this and there is one more feature in the User.login that isn't really mentioned anywhere yet. There is a second parameter you can pass in to include the user model. Meaning you can do something like

User.login({}, 'user', function(err, token) {
  var user = token.user;
  ...
});

I haven't had the time to update this yet, but if you want to give it a shot. Please submit an update to your PR and use this style instead. This way you don't need to do two separate requests to get user info. See https://github.com/strongloop/loopback/blob/master/common/models/user.js#L128. You can see include as the second param. Cheers.

superkhau commented 9 years ago

@crandmck we need to document the second include param in the apidocs

mikehedman commented 9 years ago

@superkhau - that "kind of" works :) There is a recorded issue with what you are suggesting. In the code for login() is this note:

            if (Array.isArray(include) ? include.indexOf('user') !== -1 : include === 'user') {
              // NOTE(bajtos) We can't set token.user here:
              //  1. token.user already exists, it's a function injected by
              //     "AccessToken belongsTo User" relation
              //  2. ModelBaseClass.toJSON() ignores own properties, thus
              //     the value won't be included in the HTTP response
              // See also loopback#161 and loopback#162
              token.__data.user = user;

That being said, when I changed my code to use token.__data.user it did work:

    app.models.User.login({
      email: email,
      password: password
    }, 'user', function(err, token) {
      if (err) {
        res.render('index', {
          email: email,
          password: password,
          loginFailed: true
        });
      } else {
        res.render('projects', {
          username: token.__data.user.username,
          accessToken: token.id
        });
      }
    });

But I'm disinclined to add that to the pull request, since it's directly accessing a "protected" property (__data).

superkhau commented 9 years ago

@mikehedman i think you got it right. that's how i would've done it (taking that note into consideration).

@raymondfeng is there any other way? i think token.user doesn't work according the inline comments

mikehedman commented 9 years ago

Hi, I did some poking around, and there doesn't seem to be a real clear way to fix this. As I said, I'm not crazy about having client code do direct access to the __data property. One thing I thought of was adding an "includes" property to the AccessToken object, and then in login() do something like: token.includes.user = user; This would also be the first step in accommodating different includes - the docs say that it can also be a function or an array. So it would be good to build out that structure...and then use it for user for now. Sorry, but I just don't know enough about Loopback and the properties structures to make that change myself.

bajtos commented 9 years ago

Ignore my comment above, there is another PR with a cleaner solution - #37.

@mikehedman please don't open a new PR when you need to rework an existing one. Use git history rewriting instead (git commit --amend && git push -f or git rebase -i master && git push -f).

mikehedman commented 9 years ago

@bajtos Sorry for the confusion. I did not know about the possibility of doing the git history rewrite. All I knew how to do was close this PR and reference the new one (#37).
I think the confusion came after I had closed this PR, but conversation on the larger issues continued in this thread.