diff --git a/README.md b/README.md index ef396da..4fb28d6 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,38 @@ We also provide a [web-based UI](https://casbin.org/docs/admin-portal) for model https://casbin.org/docs/adapters +### Error Handling for Adapters + +When implementing or using custom adapters (e.g., database adapters for PostgreSQL, MySQL, etc.), it's important to ensure proper error handling: + +**For Adapter Developers:** +- Adapters MUST raise errors using `error()` when operations fail (e.g., database connection failures, authentication errors). +- Do NOT silently log errors without raising them, as this prevents users from catching errors with `pcall()`. +- Example of correct error handling: +```lua +function MyAdapter:loadPolicy(model) + local conn, err = connect_to_database(self.config) + if not conn then + error("Database connection failed: " .. tostring(err)) + end + -- ... continue with loading policy +end +``` + +**For Adapter Users:** +- You can catch errors from adapter operations using `pcall()`: +```lua +local ok, err = pcall(function() + local adapter = MyAdapter:new(config) + local enforcer = Enforcer:new("model.conf", adapter) +end) + +if not ok then + print("Error loading policy: " .. tostring(err)) +end +``` +- If errors appear in logs but `pcall()` doesn't catch them, the adapter may not be raising errors properly. Contact the adapter maintainer to fix this. + ## Policy consistence between multiple nodes https://casbin.org/docs/watchers @@ -194,6 +226,22 @@ Authz middlewares for web frameworks: https://casbin.org/docs/middlewares https://casbin.org/docs/adopters +## Troubleshooting + +### Errors not caught by pcall() + +**Problem:** When using an adapter (e.g., PostgreSQL adapter) with incorrect configuration (wrong password, etc.), errors appear in `error.log` but cannot be caught with `pcall()`. + +**Cause:** The adapter is logging errors (e.g., using `ngx.log()` in OpenResty) instead of raising them with `error()`. + +**Solution:** +1. Verify the adapter is implementing the Casbin adapter interface correctly - it should raise errors using `error()` when operations fail +2. If you're using a third-party adapter, report this issue to the adapter maintainer +3. As a workaround, check logs for errors and handle them at the application level +4. Consider switching to an adapter that properly raises errors + +For adapter developers: See the [Policy persistence](#policy-persistence) section for requirements on proper error handling. + ## How to Contribute Please read the [contributing guide](CONTRIBUTING.md). diff --git a/src/persist/Adapter.lua b/src/persist/Adapter.lua index fc0e74d..08f9cb6 100644 --- a/src/persist/Adapter.lua +++ b/src/persist/Adapter.lua @@ -52,6 +52,9 @@ end * loadPolicy loads all policy rules from the storage. * * @param model the model. + * @raises error if the policy cannot be loaded (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:loadPolicy(model) @@ -61,6 +64,9 @@ end * savePolicy saves all policy rules to the storage. * * @param model the model. + * @raises error if the policy cannot be saved (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:savePolicy(model) @@ -73,6 +79,9 @@ end * @param sec the section, "p" or "g". * @param ptype the policy type, "p", "p2", .. or "g", "g2", .. * @param rule the rule, like (sub, obj, act). + * @raises error if the policy cannot be added (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:addPolicy(sec, ptype, rule) @@ -85,6 +94,9 @@ end * @param sec the section, "p" or "g". * @param ptype the policy type, "p", "p2", .. or "g", "g2", .. * @param rule the rule, like (sub, obj, act). + * @raises error if the policy cannot be removed (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:removePolicy(sec, ptype, rule) @@ -99,6 +111,9 @@ end * @param fieldIndex the policy rule's start index to be matched. * @param fieldValues the field values to be matched, value "" * means not to match this field. + * @raises error if the policy cannot be removed (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:removeFilteredPolicy(sec, ptype, fieldIndex, fieldValues) diff --git a/src/persist/FilteredAdapter.lua b/src/persist/FilteredAdapter.lua index b80874d..aef4216 100644 --- a/src/persist/FilteredAdapter.lua +++ b/src/persist/FilteredAdapter.lua @@ -24,6 +24,9 @@ setmetatable(FilteredAdapter, Adapter) * loadFilteredPolicy loads only policy rules that match the filter. * @param model the model. * @param filter the filter used to specify which type of policy should be loaded. + * @raises error if the policy cannot be loaded (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function FilteredAdapter:loadFilteredPolicy(model, filter) diff --git a/tests/persist/error_handling_spec.lua b/tests/persist/error_handling_spec.lua new file mode 100644 index 0000000..63b291f --- /dev/null +++ b/tests/persist/error_handling_spec.lua @@ -0,0 +1,132 @@ +--Copyright 2021 The casbin Authors. All Rights Reserved. +-- +--Licensed under the Apache License, Version 2.0 (the "License"); +--you may not use this file except in compliance with the License. +--You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +--Unless required by applicable law or agreed to in writing, software +--distributed under the License is distributed on an "AS IS" BASIS, +--WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +--See the License for the specific language governing permissions and +--limitations under the License. + +local Enforcer = require("src.main.Enforcer") +local Adapter = require("src.persist.Adapter") +local path = os.getenv("PWD") or io.popen("cd"):read() + +describe("Error handling tests", function () + it("should catch errors from adapter during enforcer initialization", function () + -- Create a failing adapter that throws an error during loadPolicy + local FailingAdapter = {} + setmetatable(FailingAdapter, Adapter) + FailingAdapter.__index = FailingAdapter + + function FailingAdapter:new() + local o = {} + setmetatable(o, self) + self.__index = self + return o + end + + function FailingAdapter:loadPolicy(model) + error("Database connection failed: authentication failed") + end + + local model = path .. "/examples/rbac_model.conf" + local a = FailingAdapter:new() + + -- Test that the error can be caught with pcall and has the expected message + local ok, err = pcall(function() + local e = Enforcer:new(model, a) + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "Database connection failed")) + + -- Also verify using assert.has_error pattern + assert.has_error(function() + local a2 = FailingAdapter:new() + local e2 = Enforcer:new(model, a2) + end, "Database connection failed") + end) + + it("should catch errors from adapter during explicit loadPolicy call", function () + -- Create an adapter that succeeds initially but fails on explicit loadPolicy + local DelayedFailingAdapter = {} + setmetatable(DelayedFailingAdapter, Adapter) + DelayedFailingAdapter.__index = DelayedFailingAdapter + + function DelayedFailingAdapter:new() + local o = {} + setmetatable(o, self) + self.__index = self + -- Set isFiltered = true to prevent automatic loadPolicy during Enforcer initialization. + -- This allows us to test explicit loadPolicy() calls separately. + o.isFiltered = true + return o + end + + function DelayedFailingAdapter:loadPolicy(model) + error("Database connection failed: wrong password") + end + + local model = path .. "/examples/rbac_model.conf" + local a = DelayedFailingAdapter:new() + + -- Create enforcer (won't load policy due to isFiltered=true) + local e = Enforcer:new(model, a) + + -- Test that calling loadPolicy explicitly raises an error that can be caught + local ok, err = pcall(function() + e:loadPolicy() + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "Database connection failed")) + + -- Also verify using assert.has_error pattern + assert.has_error(function() + e:loadPolicy() + end, "wrong password") + end) + + it("should catch errors from adapter during loadFilteredPolicy call", function () + -- Create an adapter that fails during loadFilteredPolicy + local FilteredFailingAdapter = {} + setmetatable(FilteredFailingAdapter, Adapter) + FilteredFailingAdapter.__index = FilteredFailingAdapter + + function FilteredFailingAdapter:new() + local o = {} + setmetatable(o, self) + self.__index = self + -- Set isFiltered = true to prevent automatic loadPolicy during Enforcer initialization. + -- This allows us to test loadFilteredPolicy() calls independently. + o.isFiltered = true + return o + end + + function FilteredFailingAdapter:loadFilteredPolicy(model, filter) + error("Database query failed: connection timeout") + end + + local model = path .. "/examples/rbac_model.conf" + local a = FilteredFailingAdapter:new() + local e = Enforcer:new(model, a) + + -- Test that error from loadFilteredPolicy can be caught + local ok, err = pcall(function() + e:loadFilteredPolicy({}) + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "Database query failed")) + + -- Also verify using assert.has_error pattern + assert.has_error(function() + e:loadFilteredPolicy({}) + end, "connection timeout") + end) +end)