Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ABAC #102

Merged
merged 1 commit into from
Apr 12, 2020
Merged

Implement ABAC #102

merged 1 commit into from
Apr 12, 2020

Conversation

xcaptain
Copy link
Contributor

@xcaptain xcaptain commented Apr 10, 2020

#78 Because rust doesn't support variadic parameters, we finally decide to use a json string to represent a request object, it can be parsed into a json object and then do evaluation in rhai.

@GopherJ please take a look

@schungx I found #{} works in 0.11.1, so no need to upgrade to 0.12.0 right?

Cargo.toml Outdated Show resolved Hide resolved
src/enforcer.rs Outdated Show resolved Hide resolved
@xcaptain xcaptain force-pushed the feature/abac branch 2 times, most recently from a99621c to 522985c Compare April 11, 2020 03:22
@codecov
Copy link

codecov bot commented Apr 11, 2020

Codecov Report

Merging #102 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   86.41%   86.45%   +0.03%     
==========================================
  Files          20       20              
  Lines        2841     2849       +8     
==========================================
+ Hits         2455     2463       +8     
  Misses        386      386              
Impacted Files Coverage Δ
src/enforcer.rs 91.64% <100.00%> (ø)
src/model/default_model.rs 95.28% <100.00%> (+0.07%) ⬆️
src/util.rs 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6087cc9...ef21490. Read the comment docs.

@schungx
Copy link
Contributor

schungx commented Apr 11, 2020

@schungx I found #{} works in 0.11.1, so no need to upgrade to 0.12.0 right?

That's correct. Object maps work in 0.11.1. I'll put out a 0.12.0 soon.

@schungx
Copy link
Contributor

schungx commented Apr 11, 2020

let scope_exp = format!("let {} = \"{}\";", token, rvals[i].as_ref());
engine.eval_with_scope::<()>(&mut scope, &scope_exp)?;

This is not necessary and you run the risk of not properly sanitizing your input (e.g. if rval[i] contains a quote character).

I'd suggest something like this:

scope.push_constant(token, rvals[i].clone());

assuming rvals[i] is a String.

@xcaptain
Copy link
Contributor Author

xcaptain commented Apr 11, 2020

Thanks, push_constant is much more secure then write an assignment statement and then eval_with_scope. After rhai 0.12.0 released we can then eval a json object directly without need to write const {} = #{};

@GopherJ Since json string is usually derived from a struct, so we are less likely to see sanitize problems here, I think we can keep this style first and when parse_json get stable then migrate to the new style.

@GopherJ
Copy link
Member

GopherJ commented Apr 11, 2020

I agree maybe we don't need to validate, rhai will return an error if it's not the correct format

src/enforcer.rs Outdated Show resolved Hide resolved
@schungx
Copy link
Contributor

schungx commented Apr 11, 2020

I agree maybe we don't need to validate, rhai will return an error if it's not the correct format

True... Rhai is sandboxed meaning that you can't really harm the system other than getting a syntax error.

in abac r_sub, r_obj, r_act can be object
use push_constant to avoid security problem if input is not properly sanitized
@GopherJ GopherJ merged commit 9332bf6 into casbin:master Apr 12, 2020
@GopherJ
Copy link
Member

GopherJ commented Apr 12, 2020

thanks @xcaptain

@GopherJ GopherJ mentioned this pull request Apr 12, 2020
@xcaptain xcaptain deleted the feature/abac branch April 12, 2020 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants