Skip to content

Conversation

@gerbyzation
Copy link
Contributor

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
Copy link
Member

hsluoyz commented May 17, 2020

@techoner @nodece @GopherJ @divypatel9881 please review.

@hsluoyz
Copy link
Member

hsluoyz commented May 18, 2020

@gerbyzation please resolve conflicts.

@gerbyzation
Copy link
Contributor Author

@hsluoyz done

@hsluoyz
Copy link
Member

hsluoyz commented May 18, 2020

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

@gerbyzation
Copy link
Contributor Author

gerbyzation commented May 18, 2020

@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
Copy link
Member

hsluoyz commented May 19, 2020

@techoner @nodece @GopherJ @divypatel9881 please review.

Copy link

@divy9881 divy9881 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine.

@hsluoyz
Copy link
Member

hsluoyz commented May 22, 2020

looks good.

@hsluoyz hsluoyz merged commit c8c47c9 into pycasbin:master May 22, 2020
@github-actions
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants