If I configure an alternative auth token, I should be able to log out and not be able to use that token.

[jazeee]

In the following code:
https://github.com/kahmali/meteor-restivus/blob/devel/lib/restivus.coffee#L312

I believe that it should not remove the token variable from Mongo, but rather, should use the configured auth token option: https://github.com/kahmali/meteor-restivus#configuration-options

From my tests, login and logout won't work in general, because Meteor now stores "hashedToken" in the users table, which means that Restivus requires using:

Restivus.configure
    useAuth: true
    auth:
        token: "services.resume.loginTokens.hashedToken"
        user: ->
            userId: @request.headers['x-user-id']
            token: Accounts._hashLoginToken(@request.headers['x-auth-token'])

Currently, with the above configuration, I can log out without my token being removed from the database. I can log out but still access protected resources.

[jazeee]

I believe that both login and logout should use the following in the find functions.

@config.auth.token

[kahmali]

Good catch! Surprised that went undiscovered this long. I'll try to fix that up tonight. I'll keep you posted.

[jazeee]

No problem! Thanks.
Also, big plus for coding in CoffeeScript!

[kahmali]

After attempting a fix last night, I realize this issue is a little less trivial than I thought. If the fix were to simply just use the configured auth token location (in @config.auth.token) when removing the key, I certainly would have swapped that out last night. The token location that I'm using when querying for the token in the Meteor.users collection is in an array, 'services.resume.loginTokens', where each element is an object that contains a token. Because the elements are objects, the selection when removing the token must look like:

Meteor.users.update @user._id, {$pull: 'services.resume.loginTokens': token: authToken}

As you can see, the last property in the key must be split to accommodate the Mongo API. If I knew that every user were going to store their tokens in an object I could easily write something to do the split, but that's not a safe assumption to make.

To compensate for this uncertainty, we need the user to declare whether their custom token is stored as an object or string, which is bringing unnecessary complexity in the API. I need to give this some more thought to figure out the cleanest way to support this. I will gladly accept any suggestions, or even better, a pull request.

Are your custom tokens stored as objects? Also, can I ask why you're OK using the default login with username and password, but not with storing those tokens in the default location? I guess my assumption was that folks wouldn't want to use the default auth endpoints if they weren't using the default token location.

On a side note, after further researching authentication more, I really hate the default auth endpoints in Restivus. I just used what existed in RestStop2 when I converted it into Restivus, without giving it much thought. I would much rather support a bearer token auth. I'm going to focus more on getting Restivus to work with middleware, so we can use the middleware from simple:rest to provide much more flexible and standard auth. At the least I'll add some bearer token auth support after the 0.7.0 release.

This is going to have to come after the 0.7.0 release as well, unless I get a PR. This is an enhancement really, not a bug fix. The default auth endpoints are currently intended to be used with the default auth settings. I'm not against improving that, it just needs to be done cleanly.

[jazeee]

My main goal is to provide a REST interface to do a custom log in using the same way that I do using the standard Accounts packages. In particular, I call Accounts.registerLoginHandler for DDP style authentication.
When I log in using that mechanism, or actually, even a standard Accounts login using the default Meteor code, the Accounts package creates a hashedToken only.

Ideally, regardless of how I log in, I should be able to go to my browser, pull the Local Storage -> Meteor.loginToken, and use it in an authorized REST call. This would demonstrate that the REST package does the same thing as the DDP Accounts package.

In my tests, the Restivus package login mechanism only works if using the default mechanism for log in. It creates a token, not a hashedToken, which does not match what the Accounts package does. This means that the users logging in via REST vs DDP will end up with different tokens in the Users collection.
It also means that any custom login mechanism, will not work as expected, since they typically will let the Accounts package do much of the database stuff, validation, etc.

I suspect that Meteor used to store the plain token in the Users collection at one time. This code probably was based on that. Meteor code then changed to store a hashed version of the token, perhaps for increased security.
I suspect that is a good thing, which then suggests that perhaps this package should also do the same.
I think that it could be done in a non-breaking way, by changing the token tag everywhere to hashedToken, and then ensuring that the code hashes the user's token when storing, and when verifying.
I can look at that option, and send it to you if I think it will work...

[kahmali]

Okay, that's definitely something we should fix. That gets to the root of your problem instead of providing a band-aid fix. We still need to support the old loginTokens.token, deprecating it for now in favor of loginTokens.hashedToken. I came across this while working on simple:rest, and I should have thought to look back at the implementation in Restivus. The Accounts package provides a set of functions for generating hashed tokens. I would like to make hashedToken the default, but that would be API-breaking and will need to be a part of 0.7.0. I think the plan of action should be:

1. Add a hotfix (v0.6.7) to use the custom auth token location, but provide a warning in the docs that it will only work if the token is stored in an array of objects
2. In v0.6.8 or 0.7.0, change the default auth login endpoint to use loginTokens.hashedToken exclusively (so all future logins are made in an identical way to the existing Accounts DDP spec)
   1. Note: Must hash the token when checking auth in login endpoint
   2. Change the logout endpoint to attempt to remove the token from both the loginTokens.token (for legacy reasons) and the loginTokens.hashedToken
   3. Update the docs with a deprecation warning for token (consider logging a deprecation warning to the console if that isn't too extreme)
3. In 0.7.0 or later, change default token location to loginTokens.hashedToken, and eventually remove any support (in logout endpoint) and deprecation warnings for loginTokens.token

Does that sound like a good course of action? Anything you'd like to add or change?

[jazeee]

I believe that it will not actually break the system, at least not badly. The reason is:

• Upgrade to this fix
• All new HTTP request with old token will fail. (The userId has no matching hashed token).
• The user will simply have to re-authenticate. They get a new valid token.
• Once reauthenticated, the user will work as normal.

I have some code I am testing to see how the hashedToken will work. I am adding some test code as well. This part is a bit tricky since the api unit tests configure Restivus, and I cannot reconfigure it in the separate test. I am not sure how to separate the unit tests appropriately...

I am also running into some other unit test issues, so perhaps I will commit without it. I can test login and logout on the command line.

[kahmali]

Here's what I started for tests in a local branch:

  describe 'The default authentication endpoints', ->
    token = null
    username = 'test'
    password = 'password'

    # Delete the test account if it's still present
    Meteor.users.remove username: username

    userId = Accounts.createUser
      username: username
      password: password

    it 'should allow a user to login', (test, next) ->
      HTTP.post Meteor.absoluteUrl('/api/v1/login'), {
        data:
          user: username
          password: password
      }, (error, result) ->
        response = JSON.parse result.content
        test.equal result.statusCode, 200
        test.equal response.status, 'success'
        test.equal response.data.userId, userId
        test.isTrue response.data.authToken

        # Store the token for later use
        token = response.data.authToken

        next()

    it 'should allow a user to logout', (test, next) ->
      HTTP.get Meteor.absoluteUrl('/api/v1/logout'), {
        headers:
          'X-User-Id': userId
          'X-Auth-Token': token
      }, (error, result) ->
        response = JSON.parse result.content
        test.equal result.statusCode, 200
        test.equal response.status, 'success'
        next()

    it 'should remove the logout token after logging out', (test, next) ->
      Restivus.addRoute 'prevent-access-after-logout', {authRequired: true},
        get: -> true

      HTTP.get Meteor.absoluteUrl('/api/v1/prevent-access-after-logout'), {
        headers:
          'X-User-Id': userId
          'X-Auth-Token': token
      }, (error, result) ->
        response = JSON.parse result.content
        test.isTrue error
        next()

Allowing multiple API configurations is coming in 0.7.0 (see #64), which is why I just need to push that update out asap. I'm going to put a hold on all updates (after this) until 0.7.0 is released. It's just going to cause me a bunch more work trying to merge back to devel. It will also allow me to clean up the tests significantly. Sorry about the poor test structure!

[jazeee]

Hey, it is quite good!
My code passes your tests, (with the last few lines updated to:

    test.equal result.statusCode, 401
    test.equal response.status, 'error'
    next()

Sweet

[kahmali]

You beat me to those edits! :D Good work!

I agree, it may not be the worst thing in the world to just force all clients to reauthenticate. Kind of evil, since we could potentially make it a little less obtrusive by checking for the loginTokens.token during auth (at least through one more release), but definitely not an API-breaking change. I could be convinced to go either way, but I would lean towards providing the legacy support, just to make the changes as unnoticeable to clients as possible.

[jazeee]

I added a few other tests as well:
A user should be able to log in with an email address. (even if logged in elsewhere)
A user should fail to log in with wrong password.
See pull request.

[jazeee]

No big deal with regads to the test code structure. It isn't that big of an issue... I am happy that we have test code at all!

I was not excited about parsing the hashToken query, but I did put that in as well.
I am unsure how to test that, without adding custom authentication features, which may be more work than we want...

[jazeee]

Also, I changed the log in failure messages to "Unauthorized".
This leaks the minimum amount of data to hackers. When the system outputs "Unknown user", this allows someone to scan common usernames until they hit successful ones.

I will put in a different ticket to "slow down failure requests", which effectively blocks hackers from scanning the server at a high rate.