Spike: Rearchitect Security Configuration frontend
Why are we doing this work
The existing Security Configuration page is currently implemented twice: once for GitLab Ultimate users, and again for non-Ultimate users.
The decision to have a separate, static implementation of the page for non-Ultimate users was taken so as to minimise the effort/time to make something available for non-Ultimate - i.e., it's an MVC.
Having two implementations increases the maintenance burden in the long term, so this issue is about investigating a different frontend architecture that would accommodate sharing code/implementations between the two versions.
What's the goal of the spike?
To determine whether the proposed architectures will make it easier to:
- Share code between Ultimate/non-Ultimate
- Add new or update existing scanners (e.g., for &4908 (closed) and #263251 (closed))
Relevant links
Non-functional requirements
- [-] Documentation:
- [-] Feature flag:
- [-] Performance:
-
Testing: See #322462 (closed)
Proposals
1. Using scanner-specific rows
Since this is a spike, the plan isn't concrete. But in general, the approach to investigate will move user-facing strings to the frontend, and to move away from the feature
abstraction and use concrete scanner-specific rows. The old implementation is column-centric, leading to various code smells; this rearchitecturing will be row/scanner-centric.
- Create scanner-specific rows for the configuration table
- Move user-facing strings out of
ConfigurationPresenter
to frontend rows
2. Using scanner-specific cells
This is closer to what the CE implementation does already, though for both the Status and Manage cells. This was the approach taken in !55623 (closed).
Next steps
-
Open follow-up issue(s) to implement new architecture (unless the effort can just be done and finished off as part of this issue) -
Move name, description and help page paths from backend to frontend (and sharing between CE/EE frontends) -
Move scanner status strings from backend to frontend -
Move/split up manage_feature.vue
into scanner-specific components (interacts/conflicts with #322902 (closed), possibly?)
-
🤖
Auto-Summary Discoto Usage
Points
Discussion points are declared by headings, list items, and single lines that start with the text (case-insensitive)
point:
. For example, the following are all valid points:
#### POINT: This is a point
* point: This is a point
+ Point: This is a point
- pOINT: This is a point
point: This is a **point**
Note that any markdown used in the point text will also be propagated into the topic summaries.
Outcomes
Outcomes define the decisions or resolutions of a discussion. Once outcomes are defined, sub-topics and points are collapsed underneath the outcomes.
Outcomes are declared in a similar manner as points:
#### OUTCOME: This is an outcome
* outcome: This is an outcome
+ Outcome: This is an outcome
- oUTCOME: This is an outcome
outcome: This is an outcome
Note that multiple outcomes may be declared for each topic.
Topics
Topics can be stand-alone and contained within an issuable (epic, issue, MR), or can be inline.
Inline topics are defined by creating a new thread (discussion) where the first line of the first comment is a heading that starts with (case-insensitive)
topic:
. For example, the following are all valid topics:
# Topic: Inline discussion topic 1
## TOPIC: **{+A Green, bolded topic+}**
### tOpIc: Another topic
Quick Actions
Action Description /discuss sub-topic TITLE
Create an issue for a sub-topic. Does not work in epics /discuss link ISSUABLE-LINK
Link an issuable as a child of this discussion Discussion-Size Indicators
The relative size of the discussion occurring within a topic and its sub-topics is indicated via braille dots.
More dots means that more points or sub-topics exist within a given topic.
Examples:
- TOPIC
⣿⣿⡆
A large discussion occurred here- TOPIC
⣇
A smaller discussion occurred here
Last updated by this job
TOPIC
⡆
⢠
Defining UI strings in the frontend #322624 (comment 521279530)
OUTCOME: Do this! #322624 (comment 521403030)- Would make the frontend the SSOT for the name, description and help link paths for each of the scanners, and also the status messages. #322624 (comment 521299683)
- We currently have two SOTs for these: in the backend for the GitLab Ultimate version, and in the frontend for the CE version. There are _already_ some discrepancies between these (e.g., DAST Scans description; different ordering of scanners) #322624 (comment 521299683)
TOPIC
⡆
⣸
Using scanner-specific rows #322624 (comment 521280283)
OUTCOME: Don't do this #322624 (comment 521402925)- Requires using plain HTML table markup #322624 (comment 521303460)
- PRO Co-locates all the scanner-specific behaviour into one component, making it easier to understand/modify #322624 (comment 521303460)
- CON Requires a total rewrite of the entire table #322624 (comment 521303460)
- CON We lose some benefits of `GlTable`, e.g., stacking behaviour (`stacked="md"`) for display on smaller screens, and also some a11y #322624 (comment 521303460)
- CON Cannot so easily add/remove columns #322624 (comment 521303460)
TOPIC
⡆
⣾
Using GraphQL as data source for scanner status #322624 (comment 521291915)
OUTCOME: Do this! (Though blocked from doing this for CE by #322627) #322624 (comment 521402769)- `Project.securityScanners` already exists in our GraphQL API, which is used for the Vulnerability Report page, as it indicates which scanners are enabled #322624 (comment 521318692)
- Planned to make this API available in CE: #322627 #322624 (comment 521318692)
- CON Does not include License Compliance status #322624 (comment 521318692)
- CON Does not include information about _which_ pipeline the scan last ran on (though does have a `pipelineRun` field) #322624 (comment 521318692)
- PRO Would make for a consistent way to get scanner status across CE/EE #322624 (comment 521318692)
- Includes `available` field, although that appears not to work as expected. In theory, this _could_ be used to determine whether or not to display the upgrade CTA in the Manage column. #322624 (comment 521318692)
- Does not (_should_ not) provide all information necessary to drive the UI - e.g., scanner-specific configuration page paths (a Rails concern rather than an API one) #322624 (comment 521318692)
TOPIC
⡆
⢸
Using scanner-specific cells #322624 (comment 521315486)
OUTCOME: Do this! #322624 (comment 521401973)- Basically the strategy already used in the CE implementation #322624 (comment 521364940)
- PRO Makes for simpler, self-contained single-responsibility components (e.g., a button to fire a GraphQL mutation) #322624 (comment 521364940)
- CON Spreads the behaviour of how a particular scanner row renders across more components, potentially making it harder to understand #322624 (comment 521364940)
- PRO This is much closer to the current implementation, so is less work to adopt #322624 (comment 521364940)
TOPIC
⡆
⢀
Sharing code between CE/EE #322624 (comment 521391691)
OUTCOME: We can improve this incrementally. Doing this optimally requires a lot of work, though (e.g., #322627, #248105 (closed), &4894 (closed)) #322624 (comment 521405592)- It _should_ be easier to share code between CE/EE following any of the proposals here, but it's not entirely clear what the best strategy is yet. Doing better than this is outside the scope of this issue. In other words: we'll iterate on this as and when we need to! #322624 (comment 521393516)
TOPIC
⡆
⢠
Moving scanners to lower tiers #322624 (comment 521393909)
OUTCOME: Figuring out how to handle this better is out of scope of this issue, and not necessary to solve right now. #322624 (comment 521403528)- We currently only have two different behaviours: GitLab Ultimate and non-Ultimate. We rely on this dichotomy and conflate it with CE/EE, which is not a long term or "correct" solution. #322624 (comment 521401323)
- To handle this better, the backend will need to provide more granular information for each scanner, e.g., whether it's available for the project; whether it can be enabled via MR, or whether it has a configuration page/UI, or some other thing specific to the scanner. #248105 (closed) and &4894 (closed) are relevant for this. #322624 (comment 521401323)