fix: enforce() should return True if any result is True#119
fix: enforce() should return True if any result is True#119adou0510 wants to merge 1 commit intocasdoor:masterfrom
Conversation
Align Python SDK enforce() behavior with Node.js SDK. When Casdoor returns multiple values, return True if any value is True, instead of only checking the first value.
|
Adou seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
@adou0510 fix linter: 
|
There was a problem hiding this comment.
Pull request overview
Aligns the Python SDK’s enforce() semantics with the Node.js SDK by treating multi-value Casdoor responses as “allowed if any result is True”.
Changes:
- Update
CasdoorSDK.enforce()to returnTruewhen any boolean in the returneddatalist isTrue. - Improve handling of non-list
datavalues (usedatadirectly when present). - Reflow a few calls (oauth token payload, JWT cert load, enforce/batch-enforce params) with manual line breaks.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| params = self._get_payload_for_access_token_request(code=code, username=username, password=password) | ||
| params = self._get_payload_for_access_token_request( | ||
| code=code, username=username, password=password) |
There was a problem hiding this comment.
These manual line breaks don’t match the repository’s Black formatting (see pre-commit/CI), and Black will likely collapse this call back to a single line (or reflow it with the closing paren on its own line). Please run Black or format this call in a Black-compliant way to avoid CI/pre-commit failures.
| code=code, username=username, password=password) | |
| code=code, | |
| username=username, | |
| password=password, | |
| ) |
| """ | ||
| certificate = x509.load_pem_x509_certificate(self.certification, default_backend()) | ||
| certificate = x509.load_pem_x509_certificate( | ||
| self.certification, default_backend()) |
There was a problem hiding this comment.
This call formatting appears non-Black-compliant (multiline without a trailing comma and with the closing parenthesis on the last argument line). Since CI runs Black via pre-commit, please reformat with Black to prevent formatting check failures.
| self.certification, default_backend()) | |
| self.certification, | |
| default_backend(), | |
| ) |
| url = self.endpoint + "/api/enforce" | ||
| params = _build_enforce_params(permission_id, model_id, resource_id, enforce_id, owner) | ||
| params = _build_enforce_params( | ||
| permission_id, model_id, resource_id, enforce_id, owner) |
There was a problem hiding this comment.
This multiline _build_enforce_params(...) call is unlikely to be Black-compliant (Black will reflow it, typically either to a single line or with a trailing comma + closing paren on its own line). Please run Black or adjust formatting accordingly so CI/pre-commit passes.
| permission_id, model_id, resource_id, enforce_id, owner) | |
| permission_id, | |
| model_id, | |
| resource_id, | |
| enforce_id, | |
| owner, | |
| ) |
| url = self.endpoint + "/api/batch-enforce" | ||
| params = _build_enforce_params(permission_id, model_id, "", enforce_id, owner) | ||
| params = _build_enforce_params( | ||
| permission_id, model_id, "", enforce_id, owner) |
There was a problem hiding this comment.
This _build_enforce_params(...) call has the same non-Black-compliant multiline formatting pattern as above. Please run Black or reformat so the pre-commit Black hook in CI doesn’t fail.
| permission_id, model_id, "", enforce_id, owner) | |
| permission_id, | |
| model_id, | |
| "", | |
| enforce_id, | |
| owner, | |
| ) |
| # Iterate through all results, return True if any is True | ||
| for result in data: | ||
| if not isinstance(result, bool): | ||
| error_str = "Casdoor response error:\n" + r.text | ||
| raise ValueError(error_str) | ||
| if result: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
CasdoorSDK.enforce() now returns True if any boolean in data is True, but AsyncCasdoorSDK.enforce() (src/casdoor/async_main.py) still returns only data[0]. This creates inconsistent behavior between the sync and async SDKs for the same API; consider updating the async implementation in the same way to keep parity.
| # Iterate through all results, return True if any is True | ||
| for result in data: | ||
| if not isinstance(result, bool): | ||
| error_str = "Casdoor response error:\n" + r.text | ||
| raise ValueError(error_str) | ||
| if result: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
The updated multi-result aggregation behavior (return True if any result is True) isn’t covered by existing tests: current enforce tests only assert the return type. Please add a test that mocks/fixtures the Casdoor response with data: [False, True] (and [False, False]) to verify the new semantics and prevent regressions.
Align Python SDK enforce() behavior with Node.js SDK. When Casdoor returns multiple values, return True if any value is True, instead of only checking the first value.