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

Add permissions for API keys #156

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from
Draft

Add permissions for API keys #156

wants to merge 23 commits into from

Conversation

justinclift
Copy link
Member

@justinclift justinclift commented Jun 7, 2021

This is still very much a work in progress. The goal is for people to be able to limit what can be done with their API keys. eg, set permissions

So far, the UI pieces are at the bottom of the user Settings page, and look like this:

2021-06-08-api-keys-concept_layout_v2

While ok for an initial go, it seems like it could become very unwieldy with even just a few API keys. So, the field of on/off boxes and similar might be better moved into a separate page with a button to go there. eg Change permissions or something

List of things:

  • - On/off toggles to select which functions are allowed for a given API key
    • Probably 60-70% done
  • - Database selector, to limit which database(s) an API key is allowed to work on
    • So far, it's "All databases" or "Pick a specific one". That should be good enough to start with
    • Probably 60-70% done
  • - IP address range(s), to limit which IP addresses the API key is allowed to be requested from
    • Pretty sure this will be a good idea for places who know they'll only be requesting from specific addresses. eg a static ip or similar
    • Not yet started
  • - Hook up the API code, to validate the incoming function calls with the chosen permissions
    • Not yet started

This is just an initial HTML design to see how it looks visually,
with no working code doing anything yet.

Pretty sure these function call toggles will need to be moved to
a separate permissions page, otherwise it'll be extremely
unwieldy for anyone with multiple API keys.
This is just the initial browser webUI to backend data sending.
No validation, nor storage of the values, in the backend yet.
Looks like my dev database is out of date and needs updating too ;)
…G database

Still a bunch of stuff to do, but it's taking shape
This UX interaction approach seems to work pretty
smoothly.  Doesn't yet set the initial values on
the settings page to match what is in the database
though.  That'll need writing (maybe next), as well
as some further cleanup and rounding out missing
backend bits.
@chrisjlocke
Copy link
Member

I looked at how MySql does it, as it has to do something similar for database users. Here, it lists each user (or API key in your world) and you modify it independently on its own page.

image

I'm wondering if a friendly name (which would be a user name?) would be handy - if you had a key garblegarblegarble which did something, would you really know what that was in a month's time? If you could set a friendly name to it, it might help admin-wise ... but then you're maybe going down the route of calling them 'users' and not 'api keys' ... calling them users is more friendly though ... but guess you don't want to rewrite a bunch of code now for no benefit...
(sorry - mumbling again...)

@chrisjlocke
Copy link
Member

chrisjlocke commented Jun 7, 2021

In the MySQL world, you create users then assign them to databases (with the above permissions being configurable for each database). The config screen for this in phpMyAdmin is a bit unwieldy though - initially confusing with 'create user', 'assign to database', 'set permissions' forms everywhere. I like your current approach though - either all or 'just this one'.

@justinclift
Copy link
Member Author

Interesting thought about the friendly name thing. We could definitely add some kind of "friendly name" and/or "notes and comments" field that's free form text for people to use. They'd only be extra data (metadata?) for making things easier to remember though, and wouldn't be used in actual authentication or wherever.

Oh, there's no real need for changing "api keys" to "users". That'd just confuse things. The ability to lock down what an api key can do is the important thing. 😄

@justinclift
Copy link
Member Author

Oh, this is what the database selection drop down (all databases, or just a specific one) looks like. It's kind of unwieldy when there are a bunch of databases, but I'm thinking it'll do for a first go. 😄

2021-06-08-api-keys-concept_layout_v2-all_databases

@LeMoussel
Copy link

What kind of error occurs to indicate that function are not allowed for a given API key?

@justinclift
Copy link
Member Author

justinclift commented Jun 8, 2021

@LeMoussel So far, I haven't yet written the part of the API code which validates things. eg I've just hooked up the webUI pieces so far, as I had to start somewhere and that helped me think some initial bits through. 😉

So... what would you like? 😄

@justinclift
Copy link
Member Author

Oh, an "All permissions" toggle up the top might be nice for people too.

Hmmm, might be better to do that later though. Like, after we've actually worked the problem through end-to-end first and are mostly sure of the shape this should take.

@justinclift
Copy link
Member Author

justinclift commented Jun 12, 2021

Finally got those initial on/off permission buttons working ok with multiple api keys on the page. They get saved to and loaded from the backend properly, and show up ok, etc.
(ironically, it's the frontend javascript which takes the most time and causes the most frustration)

Things still to do:

  • Adjust our API code to make use of these permissions
  • Show the api key owner when it was last called. Should help when them knowing if a key is in-use vs old-and-unused, etc
  • Add the IP address range permissions pieces
    • Likely do this adding the initial permission checks to the API code
  • Add the api key revoking pieces. eg the "Remove this API key" button needs its backend/frontend code written

Untested.  It compiles, but that's it.  *Might* work for some of
the functions, but unlikely to work for the Databases(), Releases()
and Tags() functions as they'll need special handling. ;)
Things aren't just "the server" any more, so the old wording is
kind of outdated. ;)
…ready

Not feeling well atm, so prob not doing anything more than this tonight.
@MKleusberg
Copy link
Member

I had a long look at this PR now and still not quite sure how to best proceed. Especially when adding more API endpoints in the future the UI becomes even more difficult to navigate. In the long run it might be better to have a separate page for defining roles and then reduce the API configuration to setting a role. I'd suggest leaving it open for now but in the meantime I'm preparing a commit that just adds read-only permissions as a new permission level.

@justinclift
Copy link
Member Author

In the long run it might be better to have a separate page for defining roles and then reduce the API configuration to setting a role.

Yeah, that's probably the right approach.

I'd suggest leaving it open for now but in the meantime I'm preparing a commit that just adds read-only permissions as a new permission level.

Been trying that code (with the read only keys in it) out on my local dev system, and it's looking pretty good. 😄

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

Successfully merging this pull request may close these issues.

4 participants