Experience Recommendations - Secure FY20-Q2 - Merge Request Widget
Note: Please assess the concepts, their viability and if the direction proposed is sound and desirable.Baseline Review Summary
Inherent workflow issues overshadowed other experience shortcomings enough for a complete reconsideration of the entire security workflow with the MR.
How we got here:
Background:
From this issue: #400 (closed), you can see there was more confusion around what we want the user to do with security findings that there were usability issues within the UI. From this assessment, I concluded that a redesign or layout change for the security widget would prove insufficient in addressing the real problem of a lack of prescribed workflow.
Ideation process:
There were four main categories that problems were bucketed into. These buckets were analyzed and then prioritized based on the severity of the problems within them. From the prioritization, it was determined that addressing the minor and semi-minor UI and experience issues would still leave a large workflow gap. Knowing this, I chose to address the workflow first and fold in as many solutions to the remaining problems as possible.
Ideation outcome:
From the ideation, I was able to identify a problem statement and begin brainstorming solutions.
Problem to solve:
How might we create a workflow that simplifies the remediation process and is inclusive to all users no matter their security ability?
Solution idea 1:
Define areas of the MR for appropriate security information consumption and action. Concept a new security workflow that is meant to address vulnerabilities detected in changes from the commits only.
- Change the security widget from an action area to an overview area.
- Move vulnerability actions to the bottom tabs of the MR and leverage existing code review experience to create a security code review process.
- Bonus: Consider incremental/targeted security testing coupled with a security testing strategy, instead of full branch scans depending on the type of scan being done. See this issue
| Today | Tomorrow |
|---|---|
 |
 |
1.1 Change security widget overview area.
By making this area essentially read-only we provide a clear indication to the user that the information in this area is meant to be consumed and analyzed then preempt action by the developer if critical or high risks are detected.
I've broken this area out into its own section. There is enough of a difference to separate the security widget from both license management and code quality or any other attached section for that matter. This will allow the user for better parsing of the security information when findings are detected.
In the security area, there are two definitive sections
- Test overview. This lets the user know if the security tests have passed, passed with warnings, or have failed. If there is a problem the user can expand the job details section
- Test results. This area alerts the user to new findings that have been detected and any fixed or dismissed vulnerabilities. A "[severity] risk detected" notification will appear in this area to notify the user how much risk was potentially introduced into the MR. This section also features a "View security report" button that will anchor the user to the security report tab at the bottom of the MR.
Full MR view
1.2 Security code review in the security reports tab
Contextual security code review is essential in understanding the circumstances around the vulnerability; thus, improving decision making while promoting good security practices.
There is a lot of complex interaction still to consider here, especially with the code review flow. Let's not dive too deep into those here. There will be an issue created to discuss this in further detail. For now, let's take the time to consider the concept of security code reviews and its merits as it's portrayed here.
Security report tab
This area is meant to house all the actions attributed to working with vulnerabilities. By moving the actions down to this area, we are keeping consistent with how other work is handled in the MR. This will lead to better define workflow and serve as a go-to place for vulnerability management in the pipeline.
Overview Section
Within the security tab, we provide both information about the triage process (handling findings found in this MR), an overview of the type of findings detected as well as actions to view the list or see the file view.
- Security tab with counts of security findings
- Actions to switch between the list view and file view
- Findings overview with three categories:
- Weaknesses: Security findings related to vulnerabilities (SAST, DAST, Container Scanning)
- Dependencies: Security findings related to vulnerable components found in dependencies (Dependency Scanning)
- Secrets: Any secrets detected as a result of a change made in this MR. (Secret_Detection)
- Triage Status: Show how many unconfirmed vulnerabilities are detected and how many vulnerabilities have been confirmed (review started, dismissed, sent to dashboard to fix later). Here the lines are meant to be a visual representation of their corresponding numbers, kind of like a horizontal bar chart with 2px tall bars. As the confirmed number increases, the lines with change accordingly.
List View
The list view is comprised of 2 main elements, filters and the list itself. Pretty simple right!?! 
Filters
The filters will work much like they do today in the issue dashboard and boards dashboard. The hope is that search would also be available in this bar as well, so users can search for a specific term if desired. Using a filter bar as opposed to another pattern will allow for more flexibility, and as we move to first-class vulnerabilities, we can iterate further on filters instead of making wholesale changes to what we have today.
| Findings | Definition |
|---|---|
| All | Show all findings |
| New (default) | All new vulnerabilities detected |
| Fixed | All vulnerabilities detected with a fixed status (vulns no longer there when making the comparison in the BE) |
| Dismissed | All vulnerabilities detected with dismissed status |
| Triage Status | Definition |
|---|---|
| All | All triage status |
| Unconfirmed (default) | All vulnerabilities that have no action attributed to them (Dismiss, confirm (start a review, add a comment, Send to dashboard-fix later) |
| Confirmed | All vulnerabilities that have an action attributed to them |
| Severity | Definition |
|---|---|
| All (default) | All severity types |
| Critical | Only Critical severity types |
| High | Only High severity types |
| ... | ... Ect through to unknown |
| Threat types | Definition |
|---|---|
| All (default) | All threat types |
| Weaknesses | Security findings related to vulnerabilities from (SAST, DAST, Container Scanning) |
| Dependencies | All findings related to vulnerabilities from (Dependency Scanning) |
| Secrets | All findings related to vulnerabilities from (Secret Detection) |
Working with vulnerabilities in the list:
When a user selects a vulnerability, they will see the more info side drawer where they can investigate the finding further and take action.
Resolving a vulnerability:
If the user wishes to resolve a vulnerability, they can now take a few different paths.
| Resolve and... | Resulting action |
|---|---|
| Start a review | User is taken to the file view where a code review is awaiting their comment and initiation. |
| Send to security dashboard | Vulnerability is marked as confirmed and sent to the security dashboard to be addressed later. This action should not be allowed for vulnerabilities that trigger security gates. |
You'll notice that create issue has been removed. This has been done because we want to inherit as much of the code review flow as possible here. If an issue is required to be created, then it can be done during the discussion of the code review. This is only the case for vulnerabilities in the MR. Issues can still be created from the security dashboard.
Security Code Review:
By far, the biggest and most impactful recommendation of the review is the ability to conduct a security code review. Here we are essentially taking the current code review process and adding vulnerability information inline into the diff. Security code review right in the MR is Vital feature to enabling DevSecOps workflows and generally good security practices overall.
Code reviews increase developer accountability and provide
transparency into changes. Mandatory reviews ensure that a change
can’t be pushed out without at least one other person being aware of
what was done and why it was done. This significantly reduces the
risk of insider threats; for example, someone trying to introduce a
logic bomb or a back door in the code. Just knowing that their code
will be reviewed also encourages developers to be more careful in
their work, improving the quality of the code.Also, context is crucial for assessing a finding and either confirming or dismissing it. This is what we've been missing by relying solely on a list of vulnerabilities.
File view and Security Code Reviews
Again, this is really a port of the current code review process with the addition of security findings information.
| Area | Description |
|---|---|
 |
File view: This gives the user an indication of what files have detected vulnerabilities, how many as well as acts as navigation to those files in this view. |
 |
File actions: The same file actions from the changes view are here as well. In this area, we also add in how many vulnerabilities are attributed to this file next to the lock icon. |
 |
Inline vulnerability information: The user will notice that the vulnerability location has been highlighted with vulnerability information appearing just below. If the user desires to see more information, they can click More info, and the side drawer will appear with the vulnerability details, similar to the list view. |
 |
Working with vulnerabilities: Hovering and selecting the vulnerable line of code will expand the area, and a comment box with action will appear. Here the user can confirm and start a review, confirm and comment, confirm and send to the dashboard with a comment or not, and dismiss with a comment. Note: all actions are disabled until a comment is entered into the comment box, just like in the changes tab. |
 |
Confirm action detail |
 |
Collapsed file view: When a file has been collapsed the vulnerability information will serve as an ad-hoc list where users can parse the vulnerabilities without scrolling through the files if desired. |
Post-review
When a review has been initiated, the comments and discussion move into the discussion tab and follow the same process present today for resolving the discussion. Inheriting this workflow will lead to a clear workflow and remove barriers that may be created from the creation of a security specific code review and resolution process. The goal here is to the MR merged in with as little risk or "acceptable" risk as possible.
Next Steps:
Review and discuss
Note: Please review the concepts and ideas presented here as if they were all at an MVC / testable level of fidelity. There are a lot of nuances and details still needed to be worked out and considered. I chose to stay away from the tactical design and experience elements of this proposal as possible so that we can discuss at a high-level. Getting into the weeds now will hinder any momentum we wish to generate. Individual issues for each idea and an MVC will be created.
A path toward action:
🗒 Track the progress of these recommendations in this epic
If what has been proposed is desirable then we can begin to flush out more details within their own issues. An epic will also be created to track progress.
| Components | MVC 1.0 has | MVC 1.1 has | MVC 1.2 has |
|---|---|---|---|
| Security widget overview area redesign | |||
| Security tab addition | |||
| Vulnerability list moved to Security tab | |||
| Vulnerability more info side drawer added | |||
| Vulnerability list filter bar added | |||
| Overview metrics added to security reports tab | |||
| File view added with Security review action |
Wait there's more!
Also from this analysis and proposal comes the idea of a Security Testing Strategy: the implementation and prescribed use of security testing and scanning in the SLDC and beyond. Link to the issue here: https://gitlab.com/gitlab-org/gitlab-ee/issues/12857