casbin / casbin-rs

An authorization library that supports access control models like ACL, RBAC, ABAC in Rust.
https://casbin.org
Apache License 2.0
828 stars 67 forks source link

bugfix: defer call `load_policy` #308

Closed mooleetzi closed 9 months ago

mooleetzi commented 1 year ago

Fix: https://github.com/casbin/casbin-rs/issues/304

in casbin-rs, there is no FilterAdapter, all adapters implement trait adapter which has method load_filtered_policy, thus we cannot determine if we can call load_policy in Enforcer:new by object type, all we can do is not call load_policy or load_filtered_policy until use.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

github-actions[bot] commented 1 year ago

Benchmark for 2bd7ef5

Click to view benchmark | Test | PR Benchmark | Master Benchmark | % | |------|--------------|------------------|---| | b_benchmark_rbac_model_large | 16.2±1.10ms | **14.2±1.00ms** | **+14.08%** | | benchmark priority model | **6.9±0.52µs** | 7.6±0.75µs | **-9.21%** | | benchmark_abac_model | **4.1±0.28µs** | 4.5±0.37µs | **-8.89%** | | benchmark_basic_model | **5.8±0.45µs** | 7.6±0.79µs | **-23.68%** | | benchmark_key_match | **7.3±0.52µs** | 27.4±2.27µs | **-73.36%** | | benchmark_raw | 4.3±0.36ns | 4.4±0.45ns | -2.27% | | benchmark_rbac_model | **6.6±0.70µs** | 10.9±0.88µs | **-39.45%** | | benchmark_rbac_model_medium | **1280.3±125.27µs** | 1425.7±125.82µs | **-10.20%** | | benchmark_rbac_model_with_domains | **9.2±0.70µs** | 12.0±0.99µs | **-23.33%** | | benchmark_rbac_with_deny | **6.6±0.47µs** | 14.0±1.20µs | **-52.86%** | | benchmark_rbac_with_resource_roles | **7.2±0.68µs** | 8.3±0.78µs | **-13.25%** | | benchmark_role_manager_large | 6.2±0.46ms | 6.4±0.49ms | -3.13% | | benchmark_role_manager_medium | 497.4±49.92µs | 478.1±37.60µs | +4.04% | | benchmark_role_manager_small | 48.3±4.45µs | 49.2±4.66µs | -1.83% |
codecov[bot] commented 1 year ago

Codecov Report

Merging #308 (b85da0e) into master (2bd7ef5) will decrease coverage by 0.32%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   67.34%   67.02%   -0.32%     
==========================================
  Files          24       24              
  Lines        1779     1777       -2     
==========================================
- Hits         1198     1191       -7     
- Misses        581      586       +5     
Impacted Files Coverage Δ
src/management_api.rs 67.42% <ø> (-2.28%) :arrow_down:
src/model/default_model.rs 92.81% <ø> (-0.66%) :arrow_down:
src/rbac_api.rs 83.08% <ø> (ø)
src/enforcer.rs 70.24% <100.00%> (-0.29%) :arrow_down:

... and 2 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

prabirshrestha commented 1 year ago

Haven’t tried it out but any reason this is closed? Would really love to have this bug fixed.

github-actions[bot] commented 1 year ago

Benchmark for 6690e97

Click to view benchmark | Test | PR Benchmark | Master Benchmark | % | |------|--------------|------------------|---| | b_benchmark_rbac_model_large | **15.8±1.18ms** | 17.4±1.17ms | **-9.20%** | | benchmark priority model | 7.9±0.84µs | 8.3±0.41µs | -4.82% | | benchmark_abac_model | **3.8±0.27µs** | 4.1±0.25µs | **-7.32%** | | benchmark_basic_model | 6.2±0.55µs | 6.6±0.53µs | -6.06% | | benchmark_key_match | **8.0±0.76µs** | 15.3±0.89µs | **-47.71%** | | benchmark_rbac_model | **7.3±0.63µs** | 11.3±0.64µs | **-35.40%** | | benchmark_rbac_model_medium | 1354.2±122.40µs | 1408.1±55.45µs | -3.83% | | benchmark_rbac_model_with_domains | **9.4±0.47µs** | 11.7±0.74µs | **-19.66%** | | benchmark_rbac_with_deny | **7.2±0.55µs** | 14.5±0.72µs | **-50.34%** | | benchmark_rbac_with_resource_roles | **7.4±0.85µs** | 8.2±0.67µs | **-9.76%** | | benchmark_role_manager_large | 8.5±0.42ms | **7.0±0.34ms** | **+21.43%** | | benchmark_role_manager_medium | 527.2±24.93µs | 549.2±32.71µs | -4.01% | | benchmark_role_manager_small | 55.0±4.29µs | 51.2±3.88µs | +7.42% |
hsluoyz commented 1 year ago

@prabirshrestha this PR has CI errors and it seems that the original author has closed it. Would you like to create a new PR?

image

prabirshrestha commented 1 year ago

I'm not comfortable signing CLA without discussing with the legal team.

Looking at the build errors seems like it is only related to clippy issues and is an easy fix. Wondering if @mooleetzi or owners can fix it. More than happy to test it out.

github-actions[bot] commented 1 year ago

Benchmark for d491cd1

Click to view benchmark | Test | PR Benchmark | Master Benchmark | % | |------|--------------|------------------|---| | b_benchmark_rbac_model_large | 17.4±0.77ms | **15.3±0.61ms** | **+13.73%** | | benchmark priority model | **7.7±0.49µs** | 8.5±0.60µs | **-9.41%** | | benchmark_abac_model | 4.1±0.17µs | 4.0±0.22µs | +2.50% | | benchmark_basic_model | **6.4±0.26µs** | 7.1±0.38µs | **-9.86%** | | benchmark_key_match | **7.9±0.33µs** | 14.1±0.83µs | **-43.97%** | | benchmark_rbac_model | **7.5±0.67µs** | 11.4±0.62µs | **-34.21%** | | benchmark_rbac_model_medium | 1359.4±78.27µs | 1343.8±48.05µs | +1.16% | | benchmark_rbac_model_with_domains | **10.3±0.66µs** | 11.8±0.74µs | **-12.71%** | | benchmark_rbac_with_deny | **7.6±0.41µs** | 15.8±1.04µs | **-51.90%** | | benchmark_rbac_with_resource_roles | **7.7±0.31µs** | 8.6±0.45µs | **-10.47%** | | benchmark_role_manager_large | 7.5±0.35ms | 7.3±0.36ms | +2.74% | | benchmark_role_manager_medium | 577.4±41.39µs | 576.2±38.95µs | +0.21% | | benchmark_role_manager_small | 54.5±2.95µs | 54.5±3.08µs | 0.00% |
hsluoyz commented 1 year ago

@mooleetzi @prabirshrestha we should add the FilterAdapter interface and follow the design of Go Casbin.