nycmeshnet / meshdb

A convenient, stable, and sane database for tracking Members and Nodes for use with robots and humans
https://db.nycmesh.net
MIT License
8 stars 13 forks source link

Ensure current roles match design #60

Closed WillNilges closed 10 months ago

WillNilges commented 1 year ago

There's some decisions we need to make about security. Django seems to have us covered there but we need to solidify what roles, access, permissions, and other synonyms we want to have.

Do we want to use IsAdminUser for some things? How do we do tokens (I think we can create a view to generate them)? We need to make sure that the way our InstallList class works doesn't expose Member info to people who shouldn't have access to it. @Andrew-Dickinson

...there's a chance it recursively resolves the member details into this view, which would expose all fields of the member model here

The docs for this could be better. I'm not really sure how to set up perm levels, or how we want to. https://www.django-rest-framework.org/api-guide/permissions/ https://testdriven.io/blog/custom-permission-classes-drf/

We need to have a meeting about security, permissions, authentication, bla bla bla.

WillNilges commented 1 year ago

https://github.com/andybaumgar/meshdb/blob/90e51008b810cb1c305e8950396c85f0a7354263/src/meshapi/permissions.py

andybaumgar commented 1 year ago

I agree it's worth having a meeting to talk about this.

Andrew-Dickinson commented 1 year ago

Administrator

Install Leader

Unauthenticated

from: https://docs.google.com/document/d/1-nkoCj3MWzZTffJGhArR3z6IsXq8W3szdYZIAGmIaJQ/edit

Andrew-Dickinson commented 1 year ago

The above permissions apply to both human and non-human usesrs

Andrew-Dickinson commented 1 year ago

Work left in this issue:

WillNilges commented 1 year ago

Some work was done in #70

WillNilges commented 1 year ago

Wow, there are all kinds of permissions that a user can have:

Image

WillNilges commented 1 year ago

We can also create groups. This can get pretty granular 👀 What if we just create a group and then apply perms that way?

WillNilges commented 1 year ago

I hope there's some way to export this configuration.

andybaumgar commented 1 year ago

Oh that's a cool feature. I guess it depends on whether we'd prefer our permissions to be defined in code or a DB that we need to migrate etc.

andybaumgar commented 1 year ago

Assuming we want to define all the permissions in the code to enable unit testing etc. I think the status quo of defining the permissions with permissions classes etc will be simpler. I like the admin GUI that comes with the built in Django groups, but I feel like that option would require either 1. an extra database import step before any other tests etc. or 2. force us to define all the groups in a migration, somewhat negating the value of the easy Django admin interface.

If we had a bunch of fine-grained evolving permissions I could see the DB group permissions being more useful, but I think our general permission model is pretty simple, so I don't think we should need it.

Anecdotally WordPress uses a database for lots of things like this as well as settings etc., and it creates a poor developer experience. Deploying your app requires migrating a database each time. Not saying this is the same situation, but something to be aware of.

WillNilges commented 1 year ago

Tracking a schema probably wouldn't be that bad. It's only one extra step. Set it up in the app, dump it, and push it to github. And we get the ease of setting it all up unambiguously in a GUI.

andybaumgar commented 1 year ago

I agree that it would be nice if we could have it dump into a migration.

I don't care that much either way, but I'm still not totally sold on the need for the GUI permissions interface since our permissions requirements are pretty simple. If our requirements were more complex I think the situation wold be different. With the GUI interface will will also need to "lock" the production system since any changes will be overwritten by our migration. Also making any changes will require running the app, logging in, make changes, export migrations vs simply changing a line in a file.

Logically the DB groups are more similar to the existing spreadsheet; free basic user experience at the cost of slightly worse developer experience.

WillNilges commented 1 year ago

That's fair. Let me at least see if I can code this.

WillNilges commented 1 year ago

https://github.com/andybaumgar/meshdb/pull/75

WillNilges commented 1 year ago

So as you might've guessed from the PR, programmatically setting up the Member group works fine. I am currently in the somewhat tedious process of writing a whole bunch of tests to ensure that permissions behave correctly. Django kicks ass for this.

WillNilges commented 11 months ago

An installer shouldn't have access to delete a member, building, or request, right?

Also, do we want to call it "installer" or "install_leader"?

WillNilges commented 11 months ago

I think we need new routes for NN provisioning access. Installer will have read (get) access and access to a NN route, but that's it.