Conversation
Implement basic version information display in the UI About modal. This provides users, QE, and Support with visibility into which release is deployed. Changes: - Add /api/v1/version REST endpoint (VersionResource.java) - Reads EXPLOIT_IQ_VERSION environment variable - Returns JSON with version info - Falls back to "unknown" for local dev - Add About modal component (AboutModal.tsx) - PatternFly AboutModal with Red Hat branding - Displays product description and version - Supports future component details from TC-5943 - Add version data hook (useVersion.ts) - Fetches version from API with caching - Handles loading and error states - Update PageHeader component - Add help icon (?) in toolbar - Trigger About modal on click - Update UserAvatarDropdown - Add "About" menu item before "Logout" - Alternative way to open About modal The implementation is designed to be extended by TC-5943 which will add component-specific versions, build metadata, and deployment info. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
tmihalac
left a comment
There was a problem hiding this comment.
PR Review — Add version display to Exploit Intelligence UI (TC-5554)
Clean feature overall — nice composition (shared modal state in PageHeader, callback threaded into UserAvatarDropdown), solid error handling in useVersion (log + fallback, no silent failure), correct routing (quarkus.rest.path=/api/v1 + @Path("/version")), and deployment-friendly env-var-with-fallback. A few things worth addressing before merge.
Important
-
@PermitAllis misleading — it does not make/api/v1/versionpublic. The config has a catch-all HTTP security policy:quarkus.http.auth.permission.default.paths=/*withdefault.policy=role-policy(requiresexploit-iq-view/prodsec/admin). Quarkus evaluatesquarkus.http.auth.permission.*at the HTTP layer before JAX-RS annotations, and there is no more-specific permit set for/api/v1/version(unlikelogout/management, which are explicitly listed). So this endpoint is governed by thedefaultrole-policy and requires an authenticated user with a role —@PermitAllhas no effect. It works because the UI calls it post-login, but the annotation misrepresents the endpoint's security posture. Decide intent: remove@PermitAllif it should stay authenticated, or addquarkus.http.auth.permission.version.paths=/api/v1/version+policy=permitif it should truly be public (note: a public version endpoint is a minor version-fingerprinting info-leak — a conscious decision, not an accident). See inline comment. -
No tests added for any of the new code. The PR adds a REST endpoint, a data hook, and a modal with zero tests, against the repo's testing discipline. At minimum: a
@QuarkusTestforVersionResourceasserting200+ JSONversionfield and the"unknown"fallback whenEXPLOIT_IQ_VERSIONis unset; and auseVersiontest covering the success path and the error→{version:"unknown"}fallback. -
Inaccurate "caching" claim — the hook doesn't cache. The JSDoc says "Caches result to avoid repeated calls" and the PR body says "with caching", but
useVersionis a plain per-mountuseState+useEffectfetch with no cache. It fetches once today only becauseAboutModalis mounted once inPageHeader. This is comment rot that will mislead: if the modal is later mounted conditionally (see below), every open re-fetches, silently contradicting the comment. Either implement real caching or correct the comment. See inline comment.
Suggestions
AboutModalis always mounted → version is fetched eagerly on page load, even if the user never opens it. Consider{isAboutModalOpen && <AboutModal .../>}— but reconcile with the caching point, since without a cache that means one fetch per open.- Speculative
componentshandling is dead code with an unsafe cast.VersionInfohas nocomponentsfield (commented out for TC-5943), so the block is unreachable today and the large inlineas VersionInfo & {...}cast is a type-safety smell. Prefer deferring to TC-5943, or addcomponents?to the interface as optional and drop the cast. See inline comment. - Product naming inconsistency:
productName="Red Hat Trusted Profile Analyzer"but the modal<h2>says "Exploit Intelligence" and the description links to "Red Hat Exploit Intelligence documentation". Verify which name should headline the About modal. - a11y: the toolbar help button uses
aria-label="Help"but opens the About modal — consideraria-label="About"for accuracy. See inline comment.
| * @return JSON response with version information | ||
| */ | ||
| @GET | ||
| @PermitAll |
There was a problem hiding this comment.
@PermitAll does not make this endpoint public. The config has a catch-all quarkus.http.auth.permission.default.paths=/* with default.policy=role-policy (requires exploit-iq-view/prodsec/admin), and Quarkus enforces quarkus.http.auth.permission.* at the HTTP layer before JAX-RS annotations. There's no more-specific permit set for /api/v1/version (unlike logout/management), so the default role-policy governs it and the endpoint actually requires an authenticated user with a role. It works today only because the UI calls it post-login.
Decide the intent and make it explicit:
- Should stay authenticated → remove
@PermitAll(currently dead/misleading). - Should be public → add
quarkus.http.auth.permission.version.paths=/api/v1/version+policy=permit, matching thelogout/managementpattern (and accept the minor version-fingerprinting info-leak consciously).
|
|
||
| /** | ||
| * Custom hook to fetch version information from the API | ||
| * Caches result to avoid repeated calls |
There was a problem hiding this comment.
This says "Caches result to avoid repeated calls", but the hook has no cache — it's a plain per-mount useState+useEffect fetch (also echoed in the PR description as "with caching"). It fetches once today only because AboutModal is mounted once in PageHeader. If the modal is later mounted conditionally (a natural optimization), every open would re-fetch, silently contradicting this comment. Either implement real caching (module-level cache / React Query) or correct the comment.
|
|
||
| // Check if components exist with proper type checking | ||
| const hasComponents = versionInfo && 'components' in versionInfo; | ||
| const components = hasComponents ? (versionInfo as VersionInfo & { components: { webapp?: { version: string }, engine?: { version: string }, cache?: { version: string } } }).components : undefined; |
There was a problem hiding this comment.
VersionInfo has no components field (it's commented out for TC-5943), so hasComponents is always false and this whole block is unreachable dead code today. The inline as VersionInfo & {...} cast is also a type-safety smell. Suggest deferring the components rendering to TC-5943, or — if you want the scaffold now — add components? as an optional field on the VersionInfo interface and drop the cast.
| <UserAvatarDropdown /> | ||
| <Button | ||
| variant="plain" | ||
| aria-label="Help" |
There was a problem hiding this comment.
This button opens the About modal, but aria-label="Help" with a question-circle icon announces "Help" to screen readers — a semantic mismatch. Consider aria-label="About" so the action matches what it does.
- Use PF design tokens for spacing (spacer/2xl, font-size/2xl, etc.) - Replace PF DescriptionList with dl/dt/dd for correct single-column layout - Override AboutModal grid to remove empty brand row at top - Add CSS overrides for brand/header/close button grid areas - Add Vite proxy config for local development without Quarkus - Add mock-server.js for testing version API locally Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove misleading @permitAll from VersionResource (endpoint requires auth via Quarkus HTTP policy; annotation was dead code) - Add module-level cache to useVersion hook so re-mounts skip re-fetch - Add components as optional field on VersionInfo interface, drop unsafe type cast in AboutModal - Fix aria-label="Help" → "About" on question-circle button Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iption text - Set modal height to fit-content to remove excess bottom space - Merge description into single paragraph with line break before docs link Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ript Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…y strings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
after our brief discussion on Slack, and knowing that the idea is to expose the ExploitIQ version and not the client specific version, it might be preferable to use a solution based on good practice with a response from That would also avoid using envar which is not an ideal solution. What do you think ? |
gildub
left a comment
There was a problem hiding this comment.
As commented earlier it seems using /version a good practice avoiding the confusion with any API version.
Please see inline comments
| <DropdownItem key="logout" onClick={handleLogout}> | ||
| Logout | ||
| </DropdownItem> | ||
| </> |
There was a problem hiding this comment.
userDropdownItems is not needed because it's not re-used and because <></> wrapper is not necessary. See below.
| </MenuToggle> | ||
| )} | ||
| > | ||
| <DropdownList>{userDropdownItems}</DropdownList> |
There was a problem hiding this comment.
Remove above userDropdownItems and replace here with
| <DropdownList> | |
| <DropdownItem key="about" onClick={handleAbout}>About</DropdownItem> | |
| <DropdownItem key="logout" onClick={handleLogout}>Logout</DropdownItem> | |
| </DropdownList> |
| } | ||
| }; | ||
|
|
||
| // Show spinner while loading user info |
There was a problem hiding this comment.
Remove unnecessary comment when the code is self-explanatory.
| ); | ||
| } | ||
|
|
||
| // Show error state (user might need to login) |
There was a problem hiding this comment.
Remove unnecessary comment when the code is self-explanatory.
| return ( | ||
| <Dropdown | ||
| isOpen={isOpen} | ||
| onSelect={onSelect} |
There was a problem hiding this comment.
Replace with
| onSelect={() => setIsOpen(false)} |
| <MenuToggle | ||
| ref={toggleRef} | ||
| isExpanded={isOpen} | ||
| onClick={onToggle} |
There was a problem hiding this comment.
Replace with
| onClick={() => setIsOpen(!isOpen)} |
| const onSelect = () => { | ||
| setIsOpen(false); | ||
| }; |
| const onToggle = () => { | ||
| setIsOpen(!isOpen); | ||
| }; |
| ref={toggleRef} | ||
| isExpanded={isOpen} | ||
| onClick={onToggle} | ||
| icon={<Icon ><UserIcon/></Icon>} |
There was a problem hiding this comment.
Remove unnecessary space:
| icon={<Icon><UserIcon/></Icon>} |
| if (onAboutClick) { | ||
| onAboutClick(); | ||
| } |
There was a problem hiding this comment.
Optional chaining ?.() is exactly the language feature for "call this if it exists", so the explicit null check is just a verbose way of writing the same thing.
| if (onAboutClick) { | |
| onAboutClick(); | |
| } | |
| onAboutClick?.(); | |
| }; | |
Implement basic version information display in the UI About modal. This provides users, QE, and Support with visibility into which release is deployed.
Changes:
Add /api/v1/version REST endpoint (VersionResource.java)
Add About modal component (AboutModal.tsx)
Add version data hook (useVersion.ts)
Update PageHeader component
Update UserAvatarDropdown
The implementation is designed to be extended by TC-5943 which will add component-specific versions, build metadata, and deployment info.