Add admin users #2358

Merged
merged 1 commit into from Jul 17, 2015

Projects

None yet

4 participants

@seanh
Contributor
seanh commented Jul 13, 2015

No description provided.

@seanh seanh added the WIP label Jul 13, 2015
@seanh seanh self-assigned this Jul 13, 2015
@landscape-bot

Code Health
Repository health decreased by 0.02% when pulling 58f00c1 on add-admin-users into 9f94961 on master.

@tilgovi tilgovi commented on the diff Jul 13, 2015
h/accounts/views.py
@@ -574,6 +575,45 @@ def _update_subscription_data(request, subscription):
request.session.flash(_('Changes saved!'), 'success')
+@view_config(attr='index', route_name='admin_users_index',
@tilgovi
tilgovi Jul 13, 2015 Contributor

If there's no real common code for these, you're welcome to decide that you think simple functions are easier and clearer than a class. It saves having to dereference the request from self.

@seanh
seanh Jul 13, 2015 Contributor

Yeah I don't like the classes, but was just following the style of the rest of the views in this file. It does kind of make sense to group related views together in a file that has many

@tilgovi
Contributor
tilgovi commented Jul 13, 2015

I think the delete route is superfluous because you can use the button name an value to disambiguate within a single POST hanlder.

@nickstenning nickstenning commented on an outdated diff Jul 13, 2015
h/accounts/models.py
@@ -77,6 +77,8 @@ class User(Base):
server_default=sa.sql.expression.false(),
nullable=False)
+ admin = sa.Column(sa.BOOLEAN, default=False, nullable=False)
@nickstenning
nickstenning Jul 13, 2015 Member

Can we get a server_default here?

@nickstenning
nickstenning Jul 13, 2015 Member

To be clear, this has to have a server_default if it's not nullable, otherwise we'll break user registration in prod as soon as we run the migration for stage (they use the same database).

@seanh
Contributor
seanh commented Jul 13, 2015

TODO:

  • Stop users from removing themselves from admins Don't let the last admin user be removed
  • Nicer error when the user you try to add doesn't exist
  • CLI for making admin users (needed to make the first admin user)
  • DB migration script
  • Tests
@seanh
Contributor
seanh commented Jul 13, 2015

Do we really want admins to be able to do everything? I think the current implementation might let admins delete and edit other users' annotations and stuff (not tested). They don't necessarily need that power, we could just give them access to the admin and nipsa views specifically

@landscape-bot

Code Health
Repository health increased by 0.02% when pulling 399cf60 on add-admin-users into 9f94961 on master.

@seanh seanh changed the title from Add simple model and views for admin users to Add admin users Jul 13, 2015
@tilgovi
Contributor
tilgovi commented Jul 13, 2015

I don't think admins have any special permissions on annotations right now. See h.models#Annotation.__acl__.

@nickstenning nickstenning commented on an outdated diff Jul 14, 2015
h/accounts/models.py
@@ -77,6 +78,10 @@ class User(Base):
server_default=sa.sql.expression.false(),
nullable=False)
+ admin = sa.Column(
@nickstenning
nickstenning Jul 14, 2015 Member

I'm a pedant, I know, but could you format this in the same way as the others?

@seanh
Contributor
seanh commented Jul 15, 2015

I don't think admins have any special permissions on annotations right now. See h.models#Annotation.acl.

I see. I guess the ACL on the annotation (lower down in the tree) overrides the one on the root node, not the other way round.

So the current implementation gives admins permission to do anything that there isn't a more-specific ACL stopping them from doing. That's probably fine, although we could tighten it up to just the admin and nipsa views specifically if we wanted.

@seanh
Contributor
seanh commented Jul 15, 2015

@nickstenning @tilgovi Waiting for a landscape comment, but this can be reviewed now and then I'll rebase it after

Notes:

  • Adds an admin column to the user table (and a .admins() classmethod on the User class for convenience)
  • Simple HTML page for listing, adding and removing admins. Doesn't let you remove the last admin. Flash error if trying to add a user that doesn't exist. Uses usernames like "seanh" not "acct:seanh@hypothes.is".
  • Simple hypothesis admin bob CLI for adding admins as well. This is what we'll use to add the first admin (I'm assuming we can do this on our prod deployment).
  • effective_principals() in h/auth.py accesses the accounts model to return "group:admins" if the user is an admin. If we want to do something more complicated (communicate this information via OAuth scopes or something), can we do that later?
  • "group:admins" gets all permissions in the ACLs of both root resources (app and api). It was already there in the API one, and that's where the choice of "group:admins" as the string comes from (I think it might originate from Pyramid docs).
  • So you just add an arbitrary permission that is never used as a principal to a view function, like @view_config(..., permission='nipsa_user_index') and this'll mean that only admins can use the view (and we could give that specific permission to specific other users later if we wanted). This is what's done for the admin management views in this PR. There's no end-to-end test with a test view like this and checking that Pyramid allows and denies access. Maybe there should be?
  • make_admin() is implemented in accounts/__init__.py. This is called in two places (by the view code and the CLI code) so I wanted it shared and wanted the CLI code to just import h.accounts not h.accounts.something.
  • I don't like the amount of patching necessary in views_test.py, these tests look really brittle to me, but without breaking from our coding style for view functions and making these dependencies explicit and easily injectable I don't think it can be avoided.
  • There's a simple db migration but (following our existing migrations) no tests for it (I have tested it manually but encourage you to do so as well, I have not tested it with postgres)
  • New User factory for tests
@seanh seanh removed the WIP label Jul 15, 2015
@nickstenning
Member

This looks pretty good. Thoughts:

  • Consolidate the views into one permission, 'admin', for now. Having separate permissions for the three routes doesn't make a lot of sense to me -- why would I have permission to see a page that allowed me to remove admin users, but not to actually remove them?
  • It would be nice to keep script.py mostly alphabetised.

I've tested the migration with postgres and it works fine.

If you make those changes and rebase this to clean it up, this'll go right in. Thanks @seanh!

Sean Hammond Add admin users feature
- admin column on users table in db

- Simple HTML page for listing, adding, removing admin users. Error
  message if trying to add a user who doesn't exist. Doesn't let you
  remove the last admin user. Only accessible to admin users.

- `hypothesis admin` command for making a user an admin (needed to make
  the first admin)

- effective_principals() gives the "group:admin" principal if the user
  is an admin user. Also improved the docstring and variable names in
  this function.

- The "group:admin" principal has security.ALL_PERMISSIONS on both root
  resources (app and API)
a928eb5
@seanh
Contributor
seanh commented Jul 17, 2015
@nickstenning
Member

LGTM.

@nickstenning nickstenning merged commit 1c42d14 into master Jul 17, 2015

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 59.042%
Details
@nickstenning nickstenning deleted the add-admin-users branch Jul 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment