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

Upgraded cedar to 4.2.0 #83

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Upgraded cedar to 4.2.0 #83

wants to merge 9 commits into from

Conversation

l-kli
Copy link

@l-kli l-kli commented Nov 13, 2024

Description of changes

Issue #, if available

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A breaking change requiring a major version bump to cedar-local-agent (e.g., changes to the signature of an
    existing API).
  • A backwards-compatible change requiring a minor version bump to cedar-local-agent (e.g., addition of a new API).
  • A bug fix or other functionality change requiring a patch to cedar-local-agent.
  • A change "invisible" to users (e.g., documentation, changes to "internal" crates, etc.)
  • A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor
    version bumps).
  • Does not update the CHANGELOG because my change does not significantly impact released code.

Testing

Add a description on how the code changes were tested, if applicable.

Hint: run ./scripts/build_and_test.sh script to validate your changes locally before submitting a PR.

@l-kli l-kli force-pushed the main branch 4 times, most recently from 061ae6f to 1d705f9 Compare November 14, 2024 17:41
@ShiromMakkad ShiromMakkad self-assigned this Nov 19, 2024
@ShiromMakkad ShiromMakkad self-requested a review November 19, 2024 22:12
@l-kli l-kli force-pushed the main branch 6 times, most recently from faead02 to 445ca2a Compare November 19, 2024 22:49
Copy link

Coverage after merging main into main

93.01%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/public
   mod.rs8.33%100%8.33%100%
   simple.rs60%100%58.33%71.43%176–179
src/public/events
   receive.rs85.19%100%88.89%77.78%28, 36
   core.rs76.67%100%90%70%156–158, 163–165, 170, 54–55, 77–79
src/public/file
   policy_set_provider.rs88.79%100%81.82%98%175
   entity_provider.rs84.86%100%75.26%95.45%228, 247–249
src/public/log
   event.rs90.63%100%75%100%
   error.rs76.92%100%57.14%100%
   schema.rs47.81%100%25.55%95.29%202, 236–239, 372–375, 532, 917, 926
   mod.rs65.63%100%61.40%100%

deny.toml Show resolved Hide resolved
src/public/log/schema.rs Outdated Show resolved Hide resolved
src/public/log/schema.rs Outdated Show resolved Hide resolved
src/public/log/schema.rs Outdated Show resolved Hide resolved
src/public/simple.rs Outdated Show resolved Hide resolved
src/public/simple.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@adamrothman
Copy link
Contributor

Thanks for opening this @l-kli – I'm looking forward to being able to upgrade to Cedar 4.

Copy link

Coverage after merging main into main

93.81%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/public
   mod.rs8.33%100%8.33%100%
   simple.rs60%100%58.33%71.43%176–179
src/public/events
   receive.rs85.19%100%88.89%77.78%28, 36
   core.rs76.67%100%90%70%156–158, 163–165, 170, 54–55, 77–79
src/public/file
   policy_set_provider.rs88.79%100%81.82%98%175
   entity_provider.rs84.86%100%75.26%95.45%228, 247–249
src/public/log
   event.rs90.63%100%75%100%
   error.rs76.92%100%57.14%100%
   schema.rs48.38%100%25.69%96.86%202, 372–375, 532, 917, 926
   mod.rs65.63%100%61.40%100%

Copy link

Coverage after merging main into main

93.81%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/public
   mod.rs8.33%100%8.33%100%
   simple.rs60%100%58.33%71.43%176–179
src/public/events
   receive.rs85.19%100%88.89%77.78%28, 36
   core.rs76.67%100%90%70%156–158, 163–165, 170, 54–55, 77–79
src/public/file
   policy_set_provider.rs88.79%100%81.82%98%175
   entity_provider.rs84.86%100%75.26%95.45%228, 247–249
src/public/log
   event.rs90.63%100%75%100%
   error.rs76.92%100%57.14%100%
   schema.rs48.38%100%25.69%96.86%202, 372–375, 532, 917, 926
   mod.rs65.63%100%61.40%100%

1 similar comment
Copy link

Coverage after merging main into main

93.81%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src/public
   mod.rs8.33%100%8.33%100%
   simple.rs60%100%58.33%71.43%176–179
src/public/events
   receive.rs85.19%100%88.89%77.78%28, 36
   core.rs76.67%100%90%70%156–158, 163–165, 170, 54–55, 77–79
src/public/file
   policy_set_provider.rs88.79%100%81.82%98%175
   entity_provider.rs84.86%100%75.26%95.45%228, 247–249
src/public/log
   event.rs90.63%100%75%100%
   error.rs76.92%100%57.14%100%
   schema.rs48.38%100%25.69%96.86%202, 372–375, 532, 917, 926
   mod.rs65.63%100%61.40%100%

@@ -1078,20 +1079,31 @@ mod test {
policy_ids.insert(PolicyId::from_str("policy1").unwrap());
policy_ids.insert(PolicyId::from_str("policy2").unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need policy_ids anymore

let auth_res = authorizer.is_authorized(&request, &policy_set, &Entities::empty());
let auth_err = auth_res.diagnostics().errors().next().unwrap();

let errors: Vec<AuthorizationError> = (0..num_of_error).map(|_| auth_err.clone()).collect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is perfect!

targets = []

[advisories]
vulnerability = "deny"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the changes to deny.toml are still here. Ideally, I don't want any changes on this file. We still don't want deps with vulnerabilities, that are unlicensed, unmaintained, etc to be allowed in the crate.

UserName = 4,
/// Email address. For example: john_doe@example.com
/// Email address. For example: john\_doe@example.com
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is this needed to appease clippy if you don't have backticks? Tbh, backticks were a better solution than escaping the _.

@@ -1140,6 +1158,15 @@ mod test {
assert_eq!(ocsf_log.status_code.unwrap(), "Deny".to_string());
}

#[test]
fn build_ocsf_severity_multiple_errors() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@@ -2,13 +2,14 @@ use rand::Rng;

/// Alphabet as an &str
pub const ALPHA: &str = "abcdefghijklmnopqrstuvwxyz";
#[allow(clippy::single_char_add_str)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, right approach here

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.

3 participants