pycasbin / django-orm-adapter

Django ORM Adapter for PyCasbin
https://github.com/casbin/pycasbin
Apache License 2.0
30 stars 19 forks source link

automatically initialize adapter + enforcer #6

Closed gerbyzation closed 4 years ago

gerbyzation commented 4 years ago

as mentioned in #2 there are still some challenges on how to initialize the enforcer:

One issue is that the initialisation of the enforcer attempts to load all policies from database. The way django works this causes problems, as this would also execute before migrations can be applied, causing the code to query non-existing tables and crashing.

At the moment I'm avoiding this by using a singleton that initialises the enforcer on the first call.

This is an attempt to solve that problem and making the package easier to use. It adds a proxy wrapper around the enforcer that defers it's initialization until django fires it's ready event and checks the migrations have been run.

Configuratin now happends through django settings, where CASBIN_MODEL, CASBIN_LOG_ENABLED, CASBIN_WATCHER and CASBIN_ROLE_MANAGER can be set. When the enforcer is initialised they will be passed to the enforcer or appropriate methods called to set them.

Let me know what you think of this approach. I'll be testing this in our app this week.

If you're happy to pursue this I can have a look at adding some more test coverage.

hsluoyz commented 4 years ago

@techoner @nodece @GopherJ @DivyPatel9881 please review.

hsluoyz commented 4 years ago

@gerbyzation please resolve conflicts.

gerbyzation commented 4 years ago

@hsluoyz done

hsluoyz commented 4 years ago

Why don't expose adapter + enforcer? How does a user do if he wants to customize the enforcer?

gerbyzation commented 4 years ago

@hsluoyz Pycasbin by design queries all the existing policy rules from the linked storage into memory when it's initialised. In django this is problematic because it will query the database before django is ready. Several options to mitigate this:

  1. the user needs to prevent directly or indirectly importing the file that is responsible for the enforcer initialisation in their codebase. Would require discipline, knowledge of which files are loaded before or after AppConfig.ready() and restrict where the enforcer can be used. Everyone using this adapter would basically have to figure out how to deal with this problem.
  2. Lazy load the plugin with for example a singleton pattern. Downside is that it defers initialization until the first time it's called.
  3. Defer loading until AppConfig.ready() (this solution). This tries to mitigate the downside of option 2 by loading it as soon as the app is ready.

To be able to implement option 3 we need to control the enforcer so we can manage the timing of it's initialization. I could think of two ways of doing that:

  1. Record all method calls on enforcer until django is ready (kinda like mock does), once it is ready 'replay' all method calls and let future calls proceed like normal. Bit of a hacky thing
  2. Manage configuration the django way; through settings.py, the normal place for plugins to be configured in the django eco system. When AppConfig.ready() fires we read the configuration options and configure the enforcer by calling the appropriate methods.

As it's less hacky and fits well with the django ecosystem option 2 seemed like the obvious choice to me.

To configure casbin this PR adds several settings: CASBIN_MODEL, CASBIN_LOG_ENABLED, CASBIN_WATCHER and CASBIN_ROLE_MANAGER. Also want to add CASBIN_ADAPTER as initializing the enforcer for testing adds a few challenges on it's own so I use a csv file for that. If there are any other configuration options that should be exposed let me know and we can add those.

To summarise this attempts to offer a ready made solution to the initialization problem for users, plus also provide a more way more fitting for django on how casbin is added to an application.

Also note that the use casbin_adapter.enforcer is not mandatory, if desired the normal enforcer and adapter can be used directly by importing them and not loading the custom app config CasbinAdapterConfig in INSTALLED_APPS.

Hope that answers your question?

hsluoyz commented 4 years ago

@techoner @nodece @GopherJ @DivyPatel9881 please review.

hsluoyz commented 4 years ago

looks good.

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 1.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: