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

Allow for token revocation with token-based authentication #73

Closed
shea256 opened this issue Aug 2, 2013 · 6 comments
Closed

Allow for token revocation with token-based authentication #73

shea256 opened this issue Aug 2, 2013 · 6 comments

Comments

@shea256
Copy link
Contributor

shea256 commented Aug 2, 2013

Hi everyone.

On the account management page (http://python-eve.org/tutorials/account_management.html), under section 6 of "Accounts with Token Authentication", it says to enable user-restricted resource access as follows:

AUTH_USERNAME_FIELD: 'token'

However, this practice means that tokens can never be revoked, or else users will no longer be able to have access to documents associated with their old tokens.

When logging a user out, I'd like to be able to revoke a token and then generate a new token when they log in. I'd also like to update a token when the user changes his/her password.

How about enabling user-restricted resource access with the id field (an immutable field) and then having an incoming token map to a certain id.

AUTH_USERNAME_FIELD: '_id'

Please correct me if there is something I'm missing.

@nicolaiarocci
Copy link
Member

Hello,

you can actually set AUTH_USERNAME_FIELD to _id and yes, you're probably right: if you're concerned about revoking/updating tokens, using the user id is probably a good idea. Also, since you're the third person to ask about it, I should probably update the documentation! Let me know if you have any trouble with using the user id. Thanks!

@shea256
Copy link
Contributor Author

shea256 commented Aug 3, 2013

Ok great thanks, I'll try this out and see how this works.

So... shouldn't everyone be concerned about revoking/updating tokens? If you have a user log out or change passwords, you need to revoke the token. Also, if a token is somehow exposed, you need to have it revoked. I would say that any model that doesn't allow for this is quite insecure, wouldn't you agree?

Side question - do you think its ok to use the mongodb _id field as the public unique identifier of an object? Most people on stackoverflow said it should be fine, but I'd just take a look at this page for reference - there are a lot of things people can discover if they know a bunch of the _id's for a given site.
http://stackoverflow.com/questions/13729151/am-i-exposing-sensitive-data-if-i-put-a-bson-id-in-a-url/13740114

@nicolaiarocci
Copy link
Member

As it is said in that thread there isn't really any relevant information being disclosed. Also ObjectIds are included with every GET response stream anyway, even if you're using a different unique identifier, so ID scanning to guess service success-rate, time-zone usage ranges etc. would still be possible.

@nicolaiarocci
Copy link
Member

Actually I've been reviewing the code and I think that simply setting auth_username_field to _id won't cut it. This also affects #71 and #46. Sorry for the mess, I'll ponder a little about this, and see what I can come up with.

@nicolaiarocci
Copy link
Member

I've come up with a possible solution. It is simple enough and puts more control in the hands of the API maintainer. It would break backward compatibility though (something I wouldn't worry much about since we're still in alpha):

  1. the eve.auth.BasicAuth base class exposes a new userid propriety which defaults to None;
  2. on implementing the check_auth method the API maintainer can now set userid to whatever value s/he wants;
  3. AUTH_USERNAME_FIELD and the corresponding endpoint setting auth_username_field get renamed to AUTH_FIELD and auth_field;
  4. if userid has been set on authentication its value is stored in auth_field on serving POST requests;
  5. if userid has been set on authentication, only documents with a matching auth_field will be processed on serving GET/PATCH/DELETE requests,

This leaves the API maintainer with complete freedom on determining the userid value and, since it's being done at the auth level (where the user record is already being looked at), doesn't force multiple database lookups on the user records. In most implementations the check_auth method will just retrieve the user record and validate it against requet.authorization as usual and, if this is the case, set userid to the user ObjectId.

By contrast the current implementation only stores request.authorization.username in AUTH_USERNAME_FIELD, making username/token editing quite a maintenance mess.

Thoughts?

cc @rxl @bcattle @asdine

@bcattle
Copy link
Contributor

bcattle commented Aug 4, 2013

I think this is a great solution.

bcattle pushed a commit to bcattle/eve that referenced this issue Sep 12, 2013
… the

results of the refactoring performed in pyeve#73. Added new methods to
`eve.io.mongo`: `_convert_query_objectids`, `combine_queries`,
`get_value_from_query`, and `query_contains_field`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants