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

⭐️ human readable impact value #5046

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions explorer/impact.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ import (
"gopkg.in/yaml.v3"
)

// Impact represents severity rating scale when impact is provided as human-readable string value
var impactMapping = map[string]int32{
"none": 0,
"low": 10,
"medium": 40,
"high": 70,
"critical": 100,
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend to place the numbers in the middle of each of the ranges. Currently critical is on the top end of the range, while the others are on the low end. I think it would help better balance things. (That said, I could see the argument to leave critical the way you have it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now, I used always the lowest number except for critical because it felt strange to have 90 for critical. I see both options: lowest or middle values working. I would appreciate if you could propose the middle values so I can just adjust to those.

Copy link
Member

Choose a reason for hiding this comment

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

The middle would be: 95 critical, 80 high, 55 medium, 20 low, 0 none

Copy link
Member

Choose a reason for hiding this comment

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

The problem with having the lower values on everything is that it skews the calculation on all evaluations this way. I.e. it's very easy to have something that is high (low end 70) have a much too lower influence on the overall calculation (of an asset or a policy) because it is at the very bottom end of the scale. That's why I'd recommend against the lower end of the scale. Btw same reason applies to the top end of the scale. Third reason to pick the middle is to give us some wiggle room to adjust severities in policies within each band to prioritize things higher/lower

}

func (v *Impact) HumanReadable() string {
if v.Value == nil {
return "unknown"
Expand All @@ -22,10 +31,10 @@ func (v *Impact) HumanReadable() string {
return "high"
case v.Value.Value >= 40:
return "medium"
case v.Value.Value > 0:
case v.Value.Value >= 10:
return "low"
default:
return "info"
return "none"
}
}

Expand Down Expand Up @@ -65,17 +74,29 @@ func (v *Impact) Checksum() uint64 {
return uint64(res)
}

// UnmarshalJSON implements the json.Unmarshaler interface for impact value. It supports human-readable string, int and
// complex struct.
func (v *Impact) UnmarshalJSON(data []byte) error {
var res int32
if err := json.Unmarshal(data, &res); err == nil {
v.Value = &ImpactValue{Value: res}
var intRes int32
if err := json.Unmarshal(data, &intRes); err == nil {
v.Value = &ImpactValue{Value: intRes}

if v.Value.Value < 0 || v.Value.Value > 100 {
return errors.New("impact must be between 0 and 100")
}
return nil
}

var stringRes string
if err := json.Unmarshal(data, &stringRes); err == nil {
val, ok := impactMapping[stringRes]
if !ok {
return errors.New("impact must use critical, high, medium, low or none")
}
v.Value = &ImpactValue{Value: val}
return nil
}

type tmp Impact
return json.Unmarshal(data, (*tmp)(v))
}
Expand Down
19 changes: 19 additions & 0 deletions explorer/impact_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,20 @@ func TestImpactParsing(t *testing.T) {
Value: &ImpactValue{Value: 40},
},
},
{
"critical rating",
`"critical"`,
&Impact{
Value: &ImpactValue{Value: 100},
},
},
{
"low rating",
`"low"`,
&Impact{
Value: &ImpactValue{Value: 10},
},
},
}

for i := range tests {
Expand Down Expand Up @@ -71,6 +85,11 @@ func TestImpactParsing(t *testing.T) {
`{"value": 101, "weight": 90}`,
"impact must be between 0 and 100",
},
{
"invalid string value",
`"mycustomcritical"`,
"impact must use critical, high, medium, low or none",
},
}

for i := range errTests {
Expand Down
Loading