-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat(cedarling): initial implementation for cedarling #8691
Conversation
Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.
Note 🟢 Risk threshold not exceeded. Change Summary (click to expand)The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective. Summary: The changes in this GitHub pull request cover a wide range of files and functionality within the "jans-lock/cedarling" project. The key changes include:
From an application security perspective, the key areas to focus on are:
Overall, the changes appear to be focused on enhancing the functionality and reliability of the "cedarling" project, but it's important to carefully review the implementation and security implications of these changes to maintain the overall security posture of the application. Files Changed:
Powered by DryRun Security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future we want smaller PR's.
Yeah, will do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sokorototo thanks for a initial code. It's looking good. It will good to add some test cases and README.md
with setup, run test cases, and usage of library instructions.
jans-lock/cedarling/src/utils.rs
Outdated
fn fetch_with_request_and_init(input: &Request, init: &RequestInit) -> js_sys::Promise; | ||
} | ||
|
||
pub async fn get_str(url: &str, _: &Tokens) -> Option<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add unit test for get_str func?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
text.as_string() | ||
} | ||
|
||
pub async fn fetch_schema() -> cedar_policy::Schema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be good to add a unit test for fetch_schema.
#[wasm_bindgen_test] | ||
async fn test() { | ||
let schema = utils::fetch_schema().await; | ||
console_log!("Fetched Schema: {:?}", &schema) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we check the let schema
with some values? Adding some assert()
statements here will be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it was a temporary test, just to make sure utils::fetch_schema
was working. I will validate the data next!
jans-lock/cedarling/src/lib.rs
Outdated
} | ||
|
||
#[wasm_bindgen] | ||
pub async fn init(auth: JsValue, entities: Option<String>, policies: Option<String>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to add an integration test for lib.rs
functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can setup some unit tests for these.
@kdhttps do you mind merging this currently, and then creating a new issue for the unit tests? Otherwise the commit might get larger |
…tatic data Signed-off-by: sokorototo <nyachiengatoto@gmail.com>
agreed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Once we have a basic POC, can you add a README on how to compile and run this locally?
You can create a new issue, I am sure to follow up! |
Prepare
Description
Target issue
It doesn't close any issue, as this is new territory.
Implementation Details
Initial implementation for
cedarling
Test and Document the changes