ryan-roemer / django-cloud-browser

Django application browser for cloud datastores (Rackspace, AWS, etc.).
http://ryan-roemer.github.com/django-cloud-browser/
MIT License
81 stars 27 forks source link

More flexible CLOUD_BROWSER_VIEW_DECORATOR #2

Closed bluecamel closed 12 years ago

bluecamel commented 12 years ago

Importing staff_member_required for this caused an import error for me. I modified cloud_browser.views.settings_view_decorator to allow CLOUD_BROWSER_VIEW_DECORATOR to be set to either a callable or a string. If a string, it assumes that it is the full path to the decorator, which it imports.

ryan-roemer commented 12 years ago

Hi Branton,

Thanks for taking the time to diagnose and wrap this up. The real problem here is the order of imports when loading settings.py. I've separately found that importing django.contrib.admin.views.decorators does cause havoc in some settings files. (And apologies for not documenting this better earlier).

The real solution is to delay importing of the decorator until after the settings has finished loading. Your pull request in effect does this by delaying the import evaluation for string module paths into the settings_view_decorator call, which is after settings load.

The proper way to do this is to formally prevent the settings-time loading with a wrapper like so in "settings.py":

def lazy_staff_member_required(view_func):
    """Restrict all cloud browser views to staff users.

    .. note:: Importing certain decorators into settings.py can have weird
        ordering conflicts. Using this wrapping decorator, we can defer all
        imports until **after** settings have been loaded.
    """
    from django.contrib.admin.views.decorators import staff_member_required
    return staff_member_required(view_func)

CLOUD_BROWSER_VIEW_DECORATOR = lazy_staff_member_required

I'm not a real proponent of mucking with __import__ and pushing raw string imports, especially here because it's a loading time issue and not a "need variably-named module"-type issue that is the usual reason for allowing raw __import__.

If I updated the documentation with the example above, would that seem to work for you to resolve the issue? (And, does the above actually work for your current use case?)

Thanks!, Ryan

bluecamel commented 12 years ago

Ah, that was actually my initial solution, but I didn't want to clutter an already cluttered settings file. I agree with your decision to keep away from unnecessary string imports, but made that decision based on two assumptions: 1) settings.py is a controlled environment, and most everything in it is generally a constant, 2) that's how django handles context processors, template loaders, etc, so that makes it cool ;) (actually, I haven't looked at that code, but it must)

But actually, since you're so responsive and open to input, I want to use the project to (at the very least) allow for specifying different decorators for individual containers (I'm using S3, by the way). But really, I'm tempted by the thought of taking it as far as directory level permissions tied to django users. I just haven't decided whether that's crazy or crazy awesome ;)

Anyhow, I've not looked much at the code involved, but just thought I'd throw that out and see if you had any plans/thoughts. I'm also happy to throw together a fork when I get to it and we can chat from there.

Thanks for a great project!

ryan-roemer commented 12 years ago

That's a reasonable point. I manually merged the pull request, then tweaked the patch slightly to use Django's own module loading tools. I've did a version bump to v0.2.1 and pushed it to PyPi, so it should be available for normal Pip installs. I'm closing the issue, but let me know if you have any problems going forward.

With respect to the other functionality (per container / path decorators / hooks to Django users), I have previously given this some thought (with an old project that tied real-filesystem path auth to Django users), so I have mucked around with this stuff before. My first thought is to place something like this in cloud_browser.app_settings.Settings, and at the container level the blacklist / whitelist code gives a basic intro to how a hook may work.

That said, I'm probably not going to do this myself (as the project got me around my pain point which is Rackspace's meager Cloud Files control panel), so you're most welcome to dive in here when you get a chance. Feel free to email me for more contact information and we can chat further if you'd like.

bluecamel commented 12 years ago

Sorry for the super delayed response, but apparently I haven't needed those changes enough to spend time on it yet!

I'll definitely get in touch if/when I get back around to it.