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

Provide an API to apply policy to a results list #2094

Closed
ghost opened this issue Sep 30, 2020 · 6 comments · Fixed by #2109
Closed

Provide an API to apply policy to a results list #2094

ghost opened this issue Sep 30, 2020 · 6 comments · Fixed by #2109
Assignees

Comments

@ghost
Copy link

ghost commented Sep 30, 2020

The API would both filter the list for rules that are disabled, and adjust level.

@michaelcfanning FYI

@ghost ghost added the enhancement label Sep 30, 2020
@eddynaka eddynaka self-assigned this Oct 12, 2020
@eddynaka
Copy link
Collaborator

Hi @lgolding and @michaelcfanning , I have some questions:

  • Imagine that we have two policies that point to the same result/rule. One is saying the rule is an error and the other is saying it is a note. What would be the result?
    What should be expected in this case?

Some information:

  • To be able to do that, we must have the policies and the results in hand. Based on that, the method would be created in the Run class.

With that in mind, I though in something like this:

public bool ApplyPolicies()
{
    // TODO: add logic to apply all policies modifying the current `Results` list
    return true;
}

@ghost
Copy link
Author

ghost commented Oct 12, 2020

My opinions:

We have various options:

  1. Allow only one policy at a time to be applied. Then the API is ApplyPolicy(string policyName = null). If policyName (or should it be id?) is absent, then: if there is more than one policy, throw an exception. If there is only one policy, apply that policy.

  2. Allow multiple policies to be applied. Then the API is ApplyPolicies(IEnumerable<string> policyNames) (or should it be ids?). If the same rule appears in more than one policy, we could choose:

    • The first one in the list wins
    • The last one in the list wins
    • The most severe wins
    • Or the API could even take a delegate that computes the winner.

My 2 cents.

@eddynaka
Copy link
Collaborator

Based on your comments:

  1. This would permit the customer to select the running policy, or bypass it. It's strange to enable such thing.
  2. The same from (1) is applied here. If we have three policies, the customer would choose to run the one with weaker validation.

Since we don't have an output saying which policy ran. I would expect that when that happens, it ran all policies that you have in place.

@ghost
Copy link
Author

ghost commented Oct 12, 2020

The SARIF standard does not say "if there is a policy present, you must apply it". It's more like "here are some policies you might find useful, depending on your situation".

For instance: "here's the government policy. If we are selling to the government, this policy is applicable. Otherwise it is not applicable."

But at this point I defer to @michaelcfanning. Those are my opinions.

@michaelcfanning
Copy link
Member

@cfaucon

Hello! I think the cleanest thing to do here is to create a computed policy cache. This cache would be initialized by an order-sensitive list of policy objects. We may need an optional behavioral flag that specifies things like 'only tighten, never loosen failure levels' (meaning that the maximal severity associated with each rule id would be honored) or 'last policy wins' in which every policy, when processed, actively updates any already computed policy for a rule id.

To build the computed policy cache, you will need to cycle through the policies in order. Compute a key for each tool + unique rule by its id (the only required rule property). Use this key to persist an object that contains relevant policy information. In our first utilization, it will simply be a potentially overridden failure level (after we do this first work, we can discuss adding the rule enabled vs, disabled designation).

The cache object can provide a simple helper that accepts a rule id and returns the overridden level. Please do not use an interface to make this testable, declare the policy cache in such a way that it can be overridden/mocked without taking on the costs of defining new interface types.

This is the core code that I suggest you write and test. Perhaps you can also write a simple visitor that overwrites every result failure level (if necessary) based on the policy cache.

Remember: there's some complexity to retrieving a result rule id, since that can be expressed on the result directly, indirectly via the result.rule property or result.ruleIndex property. Be sure to locate the helpers that exist in the SDK for handling this logic, which is complex. Let me know if you can't find it.

I hope this is enough for you to get started. Contact me and @cfaucon with any questions. I'll send you an invite to a teleconference where we can discuss this work, if you have time to drop in (entirely optional).

@ghost
Copy link
Author

ghost commented Oct 12, 2020

The helper is in the Core\Rule.cs partial class: ResolveRuleId(Run run).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants