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

Per-user API tokens #24

Open
brenns10 opened this issue Feb 23, 2017 · 3 comments
Open

Per-user API tokens #24

brenns10 opened this issue Feb 23, 2017 · 3 comments

Comments

@brenns10
Copy link
Contributor

I've recently been working with the API (I built a client library in Go if it interests anyone). I noticed that API tokens allow sending as any user, which kind of limits how we could deploy this in our school---I wouldn't give an API token unless it can be limited to a single user. It's fine for chat bots which send love on behalf of people, but not good if people wanted to install their own client and needed their own API key.

It seems to me like this would not be too difficult to implement user-specific API tokens, without disturbing the existing API token system.

  1. Add a owner_id attribute to the AccessKey model which refers to an Employee. This attribute would be null for normal AccessKeys created by admins. AccessKeys which are created by non-admins would have their owner_id set to their own Employee Id, and those keys would only be allowed to send love on behalf of that Employee.
  2. Make the API Key form / table accessible to any logged in user (but not linked to by anything except README/documentation). Administrators would have an additional form checkbox which indicates whether the API key is global or user-specific. Administrators would be able to view API tokens which have owner_id = null, while ordinary users would only see those which belong to them.
  3. Update the create API key view to create user-specific API tokens for non administrators. For administrators, allow them to decide what kind of API token is created.
  4. Finally, update the send love API view to check the type of API token before sending.

This way organizations with less structure / accountability than Yelp could allow users to use the API without opening it up to impersonation. Is this something that you would accept a pull request for?

@rockdog
Copy link
Member

rockdog commented Feb 23, 2017

+1 for this idea.

Regarding 2:
What about renaming the Admin dropdown to Manage. Normal users would only the see the API Keys item while admins continue seeing what‘s there right now.

Administrators would be able to view API tokens which have owner_id = null, while ordinary users would only see those which belong to them.

If Admins can create global and user-specific keys why should they not be able to see them? I would suggest that admins have access to all keys (so they are able to revoke them if needed) while users only see theirs.

Regarding 3:
When admins create a new key instead of the checkbox there could be another optional input that autocompletes usernames so when submitted it‘s either a user key if present or a global key if omitted.

Regarding 4:
I think right now the sender parameter for POST /love is mandatory. With user specific API keys this wouldn‘t be necessary any more. One could leave it out since the sender can be identified by the API key. Only if a global API key is used the sender must still be present in the request. I would be fine with modifying the existing endpoint - changing something from mandatory to optional shouldn‘t be a breaking change.

One other question: should the number of API keys for users be limited?

@brenns10
Copy link
Contributor Author

What about renaming the Admin dropdown to Manage. Normal users would only the see the API Keys item while admins continue seeing what‘s there right now.

Sounds like a good idea.

If Admins can create global and user-specific keys why should they not be able to see them? I would suggest that admins have access to all keys (so they are able to revoke them if needed) while users only see theirs.

Yeah, you're right. I think I was a bit tired when I wrote this :)

When admins create a new key instead of the checkbox there could be another optional input that autocompletes usernames so when submitted it‘s either a user key if present or a global key if omitted.

Yep, this is better.

I think right now the sender parameter for POST /love is mandatory. With user specific API keys this wouldn‘t be necessary any more. One could leave it out since the sender can be identified by the API key. Only if a global API key is used the sender must still be present in the request. I would be fine with modifying the existing endpoint - changing something from mandatory to optional shouldn‘t be a breaking change.

I was thinking we could do that. Glad it's acceptable!

One other question: should the number of API keys for users be limited?

This sounds like a sensible thing to do, but I'm struggling to come up with a justification. In order to limit this, we would almost certainly need to create an additional data store model to store API key counts (since we can't aggregate in GQL queries...), and we would need to take care to keep them updated. There's not currently any user-facing way to delete API keys, so we would need to add that (although that should probably exist independent of this proposal).

But I can't think of any tangible benefits beyond sounding like a sensible thing to do.

@rockdog
Copy link
Member

rockdog commented Feb 23, 2017

I would also go for not limiting and rather start dealing with it when (if ever) it becomes a problem. It was just an idea that jumped to my mind that I wanted to bring up.

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