New Vulnerabilities list tab and page - #1031
Conversation
Reviewer's GuideThis PR replaces placeholder CVEs and Vulnerability pages with Scalprum-based microfrontend integration by wrapping them in a custom ScalprumContextWrapper, updating component exports and plugin registrations, and adding the necessary Scalprum dependencies. Class diagram for ScalprumContextWrapper and related componentsclassDiagram
class ScalprumContextWrapper {
+children: node
+config: object
+mockUser: object
}
class CVEsHostDetailsTabWrapper {
+Renders: ScalprumContextWrapper
}
class CVEsHostDetailsTab {
+Renders: ScalprumComponent
+Renders: PageLayout
}
class InsightsVulnerabilityWrap {
+Renders: ScalprumContextWrapper
}
class InsightsVulnerability {
+Renders: ScalprumComponent
+Renders: PageLayout
}
ScalprumContextWrapper <|-- CVEsHostDetailsTabWrapper
ScalprumContextWrapper <|-- InsightsVulnerabilityWrap
CVEsHostDetailsTabWrapper *-- CVEsHostDetailsTab
InsightsVulnerabilityWrap *-- InsightsVulnerability
CVEsHostDetailsTab *-- ScalprumComponent
InsightsVulnerability *-- ScalprumComponent
ScalprumContextWrapper *-- ScalprumProvider
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ShimShtein - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Using a hardcoded mockUser may cause issues in production. (link)
General comments:
- The wrapper components (CVEsHostDetailsTabWrapper and InsightsVulnerabilityWrap) don’t forward incoming props (e.g. hostName) to their inner components—ensure you spread or pass those props through if they’re needed downstream.
- You’re wrapping each page component in its own ScalprumContextWrapper, which leads to repeated context providers—consider lifting the provider to a higher level (e.g. in your routes setup) to reduce duplication.
- ScalprumContextWrapper hardcodes manifestLocation and cdnPath using window.location—extract these into configuration or environment variables to make deployments and testing more flexible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The wrapper components (CVEsHostDetailsTabWrapper and InsightsVulnerabilityWrap) don’t forward incoming props (e.g. hostName) to their inner components—ensure you spread or pass those props through if they’re needed downstream.
- You’re wrapping each page component in its own ScalprumContextWrapper, which leads to repeated context providers—consider lifting the provider to a higher level (e.g. in your routes setup) to reduce duplication.
- ScalprumContextWrapper hardcodes manifestLocation and cdnPath using window.location—extract these into configuration or environment variables to make deployments and testing more flexible.
## Individual Comments
### Comment 1
<location> `webpack/common/ScalprumModule/ScalprumContext.js:14` </location>
<code_context>
+ },
+ };
+
+ const mockUser = {
+ entitlements: {},
+ identity: {
+ account_number: 'string',
+ org_id: 'string',
+ internal: {
+ org_id: 'string',
+ account_id: 'string',
+ },
+ type: 'string',
+ user: {
+ username: 'string',
+ email: 'string',
+ first_name: 'string',
+ last_name: 'string',
+ is_active: 'boolean',
+ is_internal: 'boolean',
+ is_org_admin: 'boolean',
+ locale: 'string',
+ },
+ },
+ };
+ return (
+ <ScalprumProvider
</code_context>
<issue_to_address>
Using a hardcoded mockUser may cause issues in production.
Returning a hardcoded mockUser from chrome.auth.getUser can result in incorrect user context or security vulnerabilities in production. Make this behavior conditional on the environment or configurable.
</issue_to_address>
### Comment 2
<location> `webpack/common/ScalprumModule/ScalprumContext.js:58` </location>
<code_context>
+ api={{
+ chrome: {
+ isBeta: () => false,
+ on: () => {},
+ auth: {
+ getUser: () => Promise.resolve(mockUser),
</code_context>
<issue_to_address>
The chrome.on stub may not be sufficient for all event listeners.
Modules relying on event subscription or unsubscription may fail silently. Consider adding minimal event emitter functionality or a warning.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
chrome: {
isBeta: () => false,
on: () => {},
auth: {
getUser: () => Promise.resolve(mockUser),
},
},
=======
chrome: (() => {
// Minimal event emitter for mocking
const listeners = {};
return {
isBeta: () => false,
on: (event, handler) => {
if (!listeners[event]) {
listeners[event] = [];
}
listeners[event].push(handler);
// Optionally, log a warning to indicate this is a mock
// eslint-disable-next-line no-console
console.warn(`[ScalprumContext] chrome.on('${event}') registered in mock context.`);
},
off: (event, handler) => {
if (listeners[event]) {
listeners[event] = listeners[event].filter(h => h !== handler);
// eslint-disable-next-line no-console
console.warn(`[ScalprumContext] chrome.off('${event}') unregistered in mock context.`);
}
},
// Optionally, a method to trigger events in tests
__emit: (event, ...args) => {
if (listeners[event]) {
listeners[event].forEach(handler => handler(...args));
}
},
auth: {
getUser: () => Promise.resolve(mockUser),
},
};
})(),
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
As for @MariaAga 's request got rid of the |
|
@ShimShtein can you please update the title and description to reflect that this adds CVE list as well? PR #1025 was closed in favor of this one. |
vkrizan
left a comment
vkrizan
left a comment
There was a problem hiding this comment.
Thank you. I've left a couple of comments.
| component: props => <CVEsHostDetailsTabWrapper {...props} />, | ||
| weight: 300, | ||
| metadata: { | ||
| hideTab: vulnerabilityDisabled, |
There was a problem hiding this comment.
The tab is not showing for me. How do I debug it?
There was a problem hiding this comment.
- Are there any errors in web console?
- Take a look at the
/hosts/:idAPI call, there is a flag that is passed there under thevulnerability: enabledpath.If it is passed as false, it means you need to set the advisor_local setting
There was a problem hiding this comment.
There is no such key vulnerability on the API response. Probably I've registered the system w/o Insights enabled. How to enable it?
There was a problem hiding this comment.
Looks like the vulnerability node is only sent with index and not show:
foreman_rh_cloud/lib/foreman_rh_cloud/engine.rb
Lines 131 to 132 in da91f30
Need to add it to app/views/api/v2/hosts/insights/base.rabl.
There was a problem hiding this comment.
Another option is to have vulnerabilityDisabled look at the :use_local_advisor_engine node instead, which is already in the base rabl.
There was a problem hiding this comment.
From Shim: "We are adding it to API, specifically to a single rabl. The only thing that needs to exist, is the insights facet. If the facet is not there, we won't add the node. Now the facet should be created on each package upload. Maybe in Viliam's case the host was there before my package upload pr got in, so he doesn't have the insights facet on the host. The easiest way would be to sync hosts with a button. I think I have enabled this use case, and it will properly sync the ids"
There was a problem hiding this comment.
I did a resycn with "Generate and upload report" but some hosts still don't show it. Interesting fact is that navigating from CVE counts form the hosts list table gets the tab shown, but it doesn't provide it a subscription/system id.
|
Ready for another round! |
| systemId: PropTypes.string.isRequired, | ||
| }; | ||
|
|
||
| const CVEsHostDetailsTabWrapper = ({ response }) => ( |
There was a problem hiding this comment.
The tab content is missing a bounding box. Its contents are rendered very close to the edges.
vkrizan
left a comment
vkrizan
left a comment
There was a problem hiding this comment.
Feel free to merge. The remaining issues can be done in a followup.
jeremylenz
changed the title
5 participants
It's based on #1025
It introduces CVEs list to host details as a new "Vulnerabiliy" tab.
It also introduces the CVE List page from #1019.
Summary by Sourcery
Introduce a new Vulnerability tab on host details and a standalone Vulnerability page by integrating remote modules via Scalprum and provide a context wrapper for plugin configuration.
New Features:
Enhancements:
Build: