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

Prevent the creation of admins/superusers with Token Authentication #74

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

Comments

@shea256
Copy link
Contributor

shea256 commented Aug 2, 2013

On the account management page (http://python-eve.org/tutorials/account_management.html), under "Accounts with Token Authentication", it seems like a malicious user can just create an admin or superuser through the "/accounts/" endpoint, which would be troublesome.

Shouldn't the "/accounts/" endpoint be secured so that accounts that are created only have the "user" role? Then, an existing admin or superuser can just manually go into the database and update a newly created user so that he/she has the admin or superuser role.

I might be missing something, so if that is the case please let me know.

@nicolaiarocci
Copy link
Member

I believe in that tutorial the /accounts/ endpoint is actually secured: only admins and superuser can update it.

@shea256
Copy link
Contributor Author

shea256 commented Aug 3, 2013

Ah ok, you're right. So if you have a frontend web app, how would you enable account signup?

What I ended up doing: I exposed the POST method on accounts as a public method and then changed the allowed roles for accounts to ['user']. That way, anyone can create an account, but the only role allowed is user. And then maybe an admin or superuser can go into the db and manually change someone's role to include admin or superuser status? Any suggestions for how to make this better?

Some side questions that are related:

  • Is there a way to specify default fields? I think there was a reference to it somewhere but I didn't see it in the documentation.
  • Is there a way to restrict the fields that are returned? I just created an on_getting_ filter, but I thought there might be a better way to do it that was built in.
  • Is there a way to not require certain fields to be posted but still require them to exist in the db? For example, I don't want to require the token field on posting, but I do want to auto-generate it upon account creation and ensure that every record in the db has a token.

@nicolaiarocci
Copy link
Member

What I ended up doing: I exposed the POST method on accounts as a public method and then changed the allowed roles for accounts to ['user']. That way, anyone can create an account, but the only role allowed is user. And then maybe an admin or superuser can go into the db and manually change someone's role to include admin or superuser status? Any suggestions for how to make this better?

That tutorial assumes that:

  • The /accounts/ endpoint is restricted to superuser and admins.
  • The web/mobile app is an authorized superuser itself.

Then:

  1. The user fills the registration form and submits it to the app
  2. The app validates the form fields
  3. The app performs a POST, authenticating itself as a superuser.
  4. Since the app is a known superuser the webservice stores the user document (the user itself is of course assigned a user role).
  5. The user can now consume the webservice if so s/he wishes, with just user privileges.

Of course you can have several apps as superusers (a mobile app, a web app, etc.). Account creation is firmly filtered/controlled by your applications and users can still use the service as needed. If you want your webapp users to consume the API by means of the webapp and you still want to track their activity that's easily done as since their next login to the webapp you can use their account credential to interact with the webservice (you use the superuser role only on account creation/edition).

@nicolaiarocci
Copy link
Member

Is there a way to restrict the fields that are returned? I just created an on_getting_ filter, but I thought there might be a better way to do it that was built in.

http://python-eve.org/features.html#projections

Is there a way to not require certain fields to be posted but still require them to exist in the db? For example, I don't want to require the token field on posting, but I do want to auto-generate it upon account creation and ensure that every record in the db has a token

In the tutorial you have exactly that. The token field is not included with the request payload but only with the auth headers. It is then injected in the stored document by the auth code.

Is there a way to specify default fields? I think there was a reference to it somewhere but I didn't see it in the documentation.

There is no default_value field setting, if that's what you mean. I guess this is something that should be handled at the application level, or you could use the event_hooks to add default values for missing fields. Adding a default value option would probably be a good idea though.

@shea256
Copy link
Contributor Author

shea256 commented Aug 5, 2013

Ah ok. So I just realized that the problem is that a front-end app can't be a superuser, as that would require storing a secret key on the client (which wouldn't be secure). It seems that the only way to allow user creation is to expose a public endpoint for user creation.

I think the reason I was confused is that the tutorial doesn't have you create an endpoint that a front-end app can post to, which led me to believe that the app would post to "/accounts/".

I'm thinking of creating an endpoint called "/signup/" or "/create_account/" and then allowing my angular app to post to it. Then, the appropriate flask controller posts to the "/accounts/" endpoint. What do you think? Should a little section on this be included in the tutorial, perhaps?

UPDATE: Just ran into a problem with the approach I just described - I can't submit a post request to the "/accounts/" endpoint and I can't call the post('accounts') function from within my Flask app, as currently, "post(resource)" grabs the payload using the "payload()" function.

Potential solution: do what this guy suggests http://stackoverflow.com/questions/10313001/is-it-possible-to-make-post-request-in-flask and create a function that we could call from within the app: "post(resource, payl)". In the function collections_endpoint(), the line "response = post(resource)" would be updated to "response = post(resource, payload())".

@shea256
Copy link
Contributor Author

shea256 commented Aug 5, 2013

In my local version of Eve, I changed the beginning of post.py to the following:

@ratelimit()
@requires_auth('resource')
def post(resource):
    payl = payload()
    return _post(resource, payl)

def _post(resource, payl):
    ...

I then changed methods/init.py so that it exports _post() and now, I can call _post() and simulate a POST request from one of my functions (namely "/signup/").

Let me know if you think this change should be incorporated in, or if there is a better way to do what I'm doing.

@shea256
Copy link
Contributor Author

shea256 commented Aug 7, 2013

@nicolaiarocci what are your thoughts on this? how would you implement a public /signup/ endpoint?

I ended up doing what I said in my previous comment (calling the _post() function I separated out), and it works well, but it requires that I pip install from a specific branch on my forked version of eve.

@nicolaiarocci
Copy link
Member

One completely different approach would be to hook a callback function to the /signup/ endpoint. This hook receives the payload, and then you could use app.data.driver.db['accounts'] to update the db directly.

Following up on your approach, the post function could probably be refactored like this:

@ratelimit()
@requires_auth('resource')
def post(resource, payl=None)
    ...
    if payl is None:
        payl=payload()

Didn't really test this, but it should be doable.

@shea256
Copy link
Contributor Author

shea256 commented Aug 8, 2013

Yeah I want to do all the other things in the post function, so I can't just use app.data.driver.db['accounts] and I don't want to copy and paste those lines.

So yes, that refactoring would be very helpful.

nicolaiarocci added a commit that referenced this issue Aug 9, 2013
When calling post() from your own code, you can provide an alternative
payload. This can be useful, for example, when you have a callback
function hooked to a certain endpoint, and want to perform additional
post() calls from there.

Please be advised that in order to successfully use this option,
a request context must be available.

See https://github.com/nicolaiarocci/eve/issues/74 for a discussion and
a typical use case.
@nicolaiarocci
Copy link
Member

Please give it a shot and let me know if it works as expected.

@shea256
Copy link
Contributor Author

shea256 commented Aug 9, 2013

Ok sounds good, thanks.

And the reason I separated them out into two functions originally was to encapsulate the _post() function so that it doesn't have that dependency on the global payload. There is a lot of use of request globals and others. Do you think this is a concern / could be refactored or do you think it is best as is? If so, why?

@nicolaiarocci
Copy link
Member

flask.request is, by Flask design, the way you access current request data; hardly a global from my perspective. Also, can you point me to some of the 'others' you're referencing to?

@shea256
Copy link
Contributor Author

shea256 commented Aug 12, 2013

You're right, its technically not a global. However, when a request comes in, the request object has effectively global scope.

What I generally like to do is have the functions that handle incoming requests access flask.request, and then pass values from flask.request to any functions that need to be called from there on out. That way, all the subsequent functions that are called don't depend on any outside values. The function works as is.

A huge benefit of this is function reuseability. You can create a script that just calls post 10 times, without having to send a request to the server, for example.

@shea256
Copy link
Contributor Author

shea256 commented Aug 12, 2013

Now I remember why I split the post() function and created a separate _post(payload) function...

The current post function looks like this:

@ratelimit()
@requires_auth('resource')
def post(resource, payl=None):
    ...

I want my code to be able to call the function without authorization, but the @requires_auth line means that I get the following response:

Please provide proper credentials

@nicolaiarocci
Copy link
Member

I see your point but really there are too many areas where request methods and properties are used (an API is all about processing a client's request after all). We'd end up passing a lot of stuff around, often not being used by the middleware and just being passed downstream. I don't think polluting functions/methods signatures is a good approach in this case, as the flask.request oject is just an import away.

@shea256
Copy link
Contributor Author

shea256 commented Aug 13, 2013

Wait, I still can't call the post() function without authorization. We should address this, wouldn't you say? What do you think of my original _post() function?

@nicolaiarocci
Copy link
Member

Isolating the business logic from the request is tricky, request is also used further down in the post code. And then there's the app object which of course will return an error if post is called directly from a python script. Maybe you can provide me with a complete example/explanation of your workflow (which seems quite peculiar by the way, and I am always hesitant to refactor the code for just one-shot use cases - working on your own fork would probably be a good idea).

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

Post to /signup/ with just email and password... function creates token and sets the role and then submits the formed object to the _post() function. I want to use this function because it adds the date modified and all that other stuff.

How would you approach this?

@nicolaiarocci
Copy link
Member

Why not hook a callback function to on_insert then? You get the whole payload (with date modified etc.) and you can update it before it gets inserted.

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

I tried that before. The thing is, I want a public endpoint for creating an account. If I make the account endpoint public, then anyone can do whatever they want and make themselves a superuser. If I restrict input to just a normal user, now I can't make superusers. If I don't make the endpoint public, there's no way for a Frontend app to create a user.

Here's why I want to do /signup/ instead of POST /accounts/:

I can do a normal post to /accounts/ with superuser creds to create an admin. Or my Frontend app can post to /signup/ to create a regular user.
-Ryan

On Wed, Aug 14, 2013 at 2:53 AM, Nicola Iarocci notifications@github.com
wrote:

Why not hook a callback function to on_insert then? You get the whole payload (with date modified etc.) and you can update it before it gets inserted.

Reply to this email directly or view it on GitHub:
https://github.com/nicolaiarocci/eve/issues/74#issuecomment-22618028

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

Also, let's say I want another endpoint to enable the signing up of multiple people of the same company all at once. Then I could have someone sign up for 5 seats and enforce that the company is the same for all the items coming in.

I can think of so many potential uses for allowing different endpoints with different behaviors before insertion that all access the same resource. That's why you should be able to call the post() method.
-Ryan

On Wed, Aug 14, 2013 at 9:28 AM, Ryan Shea ryan@ryanshea.org wrote:

I tried that before. The thing is, I want a public endpoint for creating an account. If I make the account endpoint public, then anyone can do whatever they want and make themselves a superuser. If I restrict input to just a normal user, now I can't make superusers. If I don't make the endpoint public, there's no way for a Frontend app to create a user.
Here's why I want to do /signup/ instead of POST /accounts/:
I can do a normal post to /accounts/ with superuser creds to create an admin. Or my Frontend app can post to /signup/ to create a regular user.
-Ryan
On Wed, Aug 14, 2013 at 2:53 AM, Nicola Iarocci notifications@github.com
wrote:

Why not hook a callback function to on_insert then? You get the whole payload (with date modified etc.) and you can update it before it gets inserted.

Reply to this email directly or view it on GitHub:
https://github.com/nicolaiarocci/eve/issues/74#issuecomment-22618028

@nicolaiarocci
Copy link
Member

I see. You achieve something similar by using the datasource feature, which allows you to create multiple endpoints (each with a different security setting) based on the same db collection. You could set a /signup/ endpoint for your front-end (which I assume is pure javascript on the client at this point, otherwise you wouldn't be having this issue in the first place). This endpoint would target the 'accounts' collection on the db, would only expose the email and username fields, and would allow user/public posting. Then you would have the /accounts/ end-point which would target the same db collection, would expose more fields, and would be accessible only by your admin interfaces. Both endpoints can have their own callback functions of course. Would that do the trick?

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

Ok yes, I actually tried messing around with the datasource feature but it was giving me some trouble. I can take another stab at it.

And what would it matter that my front-end is pure javascript? If it wasn't, I'd be able to do the signup on the server, but then that would entail either registering an admin API key to be used by the front-end or replicating code from the post() function or calling the post function itself.

@nicolaiarocci
Copy link
Member

What i mean is, you would probably have authentication credentials on your server and thus you could post as a superuser, creating user accounts on behalf of the user (which basically what the tutorial is all about)

Sent from Mailbox for iPhone

On Wed, Aug 14, 2013 at 4:26 PM, Ryan Shea notifications@github.com
wrote:

Ok yes, I actually tried messing around with the datasource feature but it was giving me some trouble. I can take another stab at it.

And what would it matter that my front-end is pure javascript? If it wasn't, I'd be able to do the signup on the server, but then that would entail either replicating code from the post() function or calling the post function itself.

Reply to this email directly or view it on GitHub:
https://github.com/nicolaiarocci/eve/issues/74#issuecomment-22639264

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

So that can work for a web front-end application, but it can't work for a mobile app.

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

Based on the solutions we discussed, I still think there is a strong case for separating the post() function into a post() and _post() function. It's also a two line fix.

@shea256
Copy link
Contributor Author

shea256 commented Aug 14, 2013

But yes, I'm going to go back to the datasource feature and see how that works out.

@nicolaiarocci
Copy link
Member

Let me know if the datasource feat allows you to get around it. If not, please submit a PR with the proposed changes, I'm more than willing to give it a spin :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants