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

Refactor and audit access code #210

Merged
merged 51 commits into from
Mar 6, 2018

Conversation

sheagcraig
Copy link
Contributor

This PR creates a level_required decorator, and staff_required decorator, and an access_required decorator to greatly eliminate redundant permissions testing, and redirection or error handling code.

  • level_required allows you to decorate a view function with UserProfile.level values that should be allowed to proceed.
  • staff_required allows you to decorate a view function that should only allow staff users to access them (these are entirely user management views).
  • access_required allows you to specify that a view should only proceed for users who are either global admins, or have permission on the BusinessUnit that a view is retrieving data from. (This could be a BusinessUnit, a MachineGroup, or a Machine).

Additionally, as this was no minor undertaking, I broke up the server.views into three separate modules based on function (UI views, settings views, and non-UI views). Whenever I worked on some code, if it had unused code, redundant code, poor style, etc, I updated it.

For example, in several places the context for a view is initially populated with csrf(request). But then later in the view function the context is assigned a dict, overwriting the previous data. So those were all removed.

Tests are included for the decorators.

Annoyingly, I discovered a mistake in the (re-enabled due to another PR from me) the setting of "never" for your Sal version notification policy, which is fixed with a6d4b77

A number of lax permissions mistakes were discovered and corrected. I can summarize more specifically if desired, but these basically fell into several permutations:

  1. Views which had no links to unprivileged users presented in the UI were still available if a properly formed request was made.
  2. Some settings endpoints were available to any authenticated user.
  3. Views were missing login_required decorator; this is minor as they all had more restrictive decorators additionally, but I put login_required in for consistency. (explicit is better than implicit and whatnot).
  4. A properly formed request could get machine_detail_facter and machine_detail_conditions data with an authenticated but unprivileged user.
  5. An unprivileged user who was a member of a Business Unit could delete machines!

During the course of this PR I also figured out how to override the User admin page, so I removed the UserProfile admin page and replaced the stock UserAdmin with a customized one that includes an inline UserProfile. (It's better!)

The "X Machine" delete-this-computer displayed on a machine detail view, even if the viewing user didn't have permission to actually delete a machine. The modal would not display, but I have thus removed the button entirely from the navbar.

The base template included a line that was supposed to show the logged in user's level. However, it had a typo and didn't display. Also, there's no easy way to know who you are logged in as, so I added a menu item that includes username and level to the menu.

Outstanding Issues

  • We need to determine what Sal should do when a user should not have access to something. As it stands, there is a mix of redirects, 403, and 404.
  • RW users are able to create, edit, and delete machines, and create and edit machine groups; but not delete them. Is this an oversight, or intended?

@sheagcraig
Copy link
Contributor Author

There is still some work that can be done here; The plugin code expects
to come across plugins which are missing a plugin_type method, and which
have a wide range of type values which don't seem to be consistent.

I'm currently adding a plugin_type method onto those, which seems like a waste of processing time. In the future, I'll just create a SalPlugin class and all plugins will be expected to subclass it and override things if needed. It looks like most of the Sal plugins out in the wild are just copied and pasted from the ones that ship with Sal, so this kind of standardization would be good.

Regular plugins are excluding plugins with types machine_detail or report.
Reports are excluding plugins which don't have name == report
Machine detail plugins were excluding builtin, and full_page. I'm guessing this was historical, and I changed full_page to report.

I'm not seeing a huge need to override/implement a plugin_type method, so it seems like these could just be a class property, and could use a PluginType class/pseudo-typedef
(e.g.

class PluginType():
    builtin = 'builtin'
    machine_detail = 'machine_detail'
    # etc

class MyPlugin(SalPlugin):
    plugin_type = PluginType.machine_detail

I get why this stuff is here-we don't want to break existing working plugins. Perhaps this is something that could be overhauled during the upcoming next major version work, where there's less expectations that things will continue to work with no changes.

As it stands, I left # TODO comments about these in place so we can find them later.

I want to look over the code too and make sure that the plugin model reload is happening at the appropriate UI places, and not getting run elsewhere. It's a minor impact if it does, but it would help with the plugin refactor.

@grahamgilbert
Copy link
Member

Are you happy with this now? I can start testing this during airport time on Monday.

@sheagcraig
Copy link
Contributor Author

I thought you were going to be AFK all week, so I haven't pushed the last bit of stuff. But knowing you have airport time, I'll get it finished up ASAP.

Newly divided into:
1. Settings UI views.
1. "regular" UI views.
1. Non UI views (checkins, csv, etc).

I then cleaned out all of the now-unused imports for each of these.

Now vim doesn't freak out when I open views.py ;)
Specifically, update:
- `bu_dashboard`
- `group_dashboard`
- `machine_detail`

Also, did some code cleanup and refactoring in those views. The main one
was compressing the UpdateHistory/UpdateHistoryItem queries into a
one-liner where used.
Fix incorrect trying to raise an HTTP Response object.
As it stands, the Django staff property is used to control who is
allowed to edit users. However, this property was mistakenly being used
to control whether to display the settings menu item. Further, the
various user-editing routes, while being blocked for non-staff GA users,
showed up once you got into the settings menu (via just entering the
URL).

This shows the settings menu for all GA users, and only shows the user
editing navbar item for Staff users.

This is of course just to fix Sal to use the intended behavior. At some
point these permissions may be merged/clarified in some way.
This commit removes the `UserProfile` admin page which was trying to
work around not knowing how to change the `User` admin page. After
figuring that out, it's redundent and less-featured.

The `is_superuser` property of User is suppressed because it doesn't
actually do anything useful in Sal at this time. That being said, some
documentation probably needs to be created to indicate what it is that
staff is doing (as in addition to admin page privileges, it also grants
user-editing permissions).

This also sorts some imports.
This will replace the `ga_required` and (needed but not in)
`rw_required` with a general purpose decorator that can specify at
decoration time which levels should be allowed.

It also adds a `ProfileLevel` class to use instead of literals for
userprofile levels.
Specifically, this includes `new_machine_group`, `edit_machine_group`,
and `new_machine`.

Also, remove a lot of copy & paste cruft that isn't being used, and
reorder to minimize processing.

Remove commented out `save_m2m` on forms since there aren't many-to-many
relationships and it's commented out.
`filter` knows to get the queryset from the manager without explicitly
doing so with `all`.
In all of the changed code, the all is not needed because the following
method of the Queryset API will get the queryset from the manager first.
Strangely, enabling plugins or reports did not require a user be
anything but authenticated, so `required_level` was added to those views
as well.
Most of this is just renaming really.

However, several views were missing things:
- `machine_detail_facter`
- `machine_detail_conditions`
- `delete_machine`
Sort imports. Should be easier to read and maintain, if not at least a
wee bit faster.
@sheagcraig
Copy link
Contributor Author

Let's call this good for now.

I have another chunk of code that I've identified to work on, but it can easily be completely separate from this. hint it's plugins.

@grahamgilbert grahamgilbert merged commit bfa078a into salopensource:master Mar 6, 2018
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.

2 participants