Create, list, delete Android certificates from the UI - #37314
Conversation
…certificates' code
GitHub Actions
Error
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37314 +/- ##
========================================
Coverage 65.95% 65.96%
========================================
Files 2336 2344 +8
Lines 185738 185870 +132
Branches 7854 7883 +29
========================================
+ Hits 122509 122608 +99
- Misses 52036 52069 +33
Partials 11193 11193
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughA new UI feature for managing Android certificates within OS Settings is introduced, including create, list, and delete operations. The implementation adds type definitions for host certificates, creates dedicated UI components (Certificates card, Add/Delete modals), refactors ProfileListHeading into a generic UploadListHeading component, updates navigation and routing, and extends API services with certificate endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Certificates Card
participant Modal as Add Certificate Modal
participant API as Certificate API
participant Notify as Notification
User->>UI: Click Add Certificate
UI->>Modal: Open modal
Modal->>Modal: Fetch certificate authorities
User->>Modal: Enter certificate details
Modal->>Modal: Validate form (name, CA, subject)
User->>Modal: Click Create
Modal->>API: POST certificate
API-->>Modal: Success/Error
alt Success
Modal->>Notify: Show success toast
Notify-->>User: Confirmation
Modal->>UI: Close & refresh list
else Error
Modal->>Notify: Show error toast
Notify-->>User: Error message
end
sequenceDiagram
participant User
participant UI as Certificates Card
participant Modal as Delete Certificate Modal
participant API as Certificate API
participant Notify as Notification
User->>UI: Click delete on certificate
UI->>Modal: Open confirmation modal
User->>Modal: Click Delete
Modal->>API: DELETE /certificates/:id
API-->>Modal: Success/Error
alt Success
Modal->>Notify: Show success flash
Notify-->>User: Confirmation
Modal->>UI: Close & refresh list
else Error
Modal->>Notify: Show error flash
Notify-->>User: Error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/_styles.scss (1)
1-2: Consider removing the empty style rule or adding necessary styles.The
.delete-certificate-modalselector contains no style declarations. If no custom styling is needed for this modal, consider removing this file. Otherwise, add the required styles.frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/DeleteCertificateModal.tsx (1)
38-41: Consider clarifying the team context in the modal message.The message states "from all hosts assigned to this team," but the modal doesn't receive or display team information. This could be confusing if:
- The feature works at the global level (no teams)
- The team context is not clear to the user
Consider one of these approaches:
- If team ID is relevant, pass it as a prop and display the team name
- If this always operates at the team level, ensure the parent context makes this clear
- If teams are optional, adjust the message to handle both scenarios
Example for approach 3:
<p> - This action will remove the <b>{name}</b> certificate from all hosts - assigned to this team. + This action will remove the <b>{name}</b> certificate from all assigned hosts. </p>frontend/services/entities/certificates.ts (2)
109-122: Misleading variable name and inconsistent query string handling.Line 114 assigns
CERTIFICATEStoCERT_TEMPLATES, which is confusing since this endpoint manages certificates, not certificate templates. Additionally, line 116 usesbuildQueryStringFromParamscorrectly, but this pattern is not followed increateCert(line 132).Apply this diff to improve clarity and consistency:
- getCerts: ({ - team_id, - page, - per_page, - }: IGetCertsParams): Promise<IGetCertsResponse> => { - const { CERTIFICATES: CERT_TEMPLATES } = endpoints; - - const queryString = buildQueryStringFromParams({ team_id, page, per_page }); - - return sendRequest( - "GET", - queryString ? CERT_TEMPLATES.concat(`?${queryString}`) : CERT_TEMPLATES - ); - }, + getCerts: ({ + team_id, + page, + per_page, + }: IGetCertsParams): Promise<IGetCertsResponse> => { + const { CERTIFICATES } = endpoints; + + const queryString = buildQueryStringFromParams({ team_id, page, per_page }); + + return sendRequest( + "GET", + queryString ? CERTIFICATES.concat(`?${queryString}`) : CERTIFICATES + ); + },
123-138: UsebuildQueryStringFromParamsfor consistency and consider endpoint helper pattern.Line 132 manually concatenates the
team_idquery parameter, whilegetCertsusesbuildQueryStringFromParams. Line 137 manually builds the delete path with template literals.For consistency, consider using
buildQueryStringFromParamsincreateCert:createCert: ({ name, certAuthorityId, subjectName, teamId }: ICreateCert) => { const { CERTIFICATES } = endpoints; const requestBody = { name, certificate_authority_id: certAuthorityId, subject_name: subjectName, }; + const queryString = buildQueryStringFromParams({ team_id: teamId }); return sendRequest( "POST", - teamId ? CERTIFICATES.concat(`?team_id=${teamId}`) : CERTIFICATES, + queryString ? CERTIFICATES.concat(`?${queryString}`) : CERTIFICATES, requestBody ); },Additionally, for the
deleteCertendpoint, consider following the pattern used elsewhere in this file (e.g.,CERTIFICATE_AUTHORITY(id)on line 82) by adding aCERTIFICATE(id)helper inendpoints.ts:deleteCert: (id: number) => { - return sendRequest("DELETE", endpoints.CERTIFICATES.concat(`/${id}`)); + return sendRequest("DELETE", endpoints.CERTIFICATE(id)); },frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx (2)
11-14: Address@ts-ignoresuppressions.The
@ts-ignoredirectives suppress type checking forInputFieldandDropdown. This technical debt should be tracked and resolved to maintain type safety.Consider either:
- Adding proper TypeScript definitions for these components, or
- Tracking this as a TODO with a linked issue to address the missing types
If these components are widely used without types, consider creating a shared effort to add type definitions across the codebase.
67-80: Consider a more specific query key for Certificate Authorities.The query key is a simple string
"certAuthorities", which may cause cache collision if other components fetch CA data with different parameters or contexts. While this works for the current use case, a more structured key would improve cache predictability.Consider using a structured query key:
const { data: cAResp, isLoading: isLoadingCAs, isError: isErrorCAs, } = useQuery( - "certAuthorities", + ["certAuthorities", { scope: "dropdown" }], () => { return certificatesAPI.getCertificateAuthoritiesList(); }, { ...DEFAULT_USE_QUERY_OPTIONS, select: (data) => data.certificate_authorities, } );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
changes/36689-update-android-certs-from-ui(1 hunks)frontend/__mocks__/certificatesMock.ts(2 hunks)frontend/__mocks__/hostMock.ts(1 hunks)frontend/__mocks__/mdmMock.ts(1 hunks)frontend/interfaces/host.ts(1 hunks)frontend/pages/ManageControlsPage/OSSettings/OSSettingsNavItems.tsx(2 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/Certificates.tsx(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/_styles.scss(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateCard/AddCertificateCard.tsx(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateCard/_styles.scss(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/_styles.scss(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/index.ts(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/DeleteCertificateModal.tsx(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/_styles.scss(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/index.ts(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/index.ts(1 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/CustomSettings.tsx(2 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ProfileListHeading/index.ts(0 hunks)frontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ProfileListItem/ProfileListItem.tsx(1 hunks)frontend/pages/ManageControlsPage/components/UploadListHeading/UploadListHeading.tsx(3 hunks)frontend/pages/ManageControlsPage/components/UploadListHeading/_styles.scss(1 hunks)frontend/pages/ManageControlsPage/components/UploadListHeading/index.ts(1 hunks)frontend/pages/admin/IntegrationsPage/IntegrationNavItems.tsx(1 hunks)frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/CertificateAuthorities.tsx(1 hunks)frontend/router/paths.ts(2 hunks)frontend/services/entities/certificates.ts(4 hunks)frontend/services/entities/mdm.ts(0 hunks)frontend/utilities/endpoints.ts(1 hunks)
💤 Files with no reviewable changes (2)
- frontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ProfileListHeading/index.ts
- frontend/services/entities/mdm.ts
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/interfaces/host.ts (1)
server/fleet/hosts.go (1)
DiskEncryptionStatus(598-598)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts (2)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx (1)
IAddCertFormData(30-34)frontend/services/entities/certificates.ts (1)
ICertificate(55-61)
frontend/__mocks__/certificatesMock.ts (1)
frontend/services/entities/certificates.ts (1)
ICertificate(55-61)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/DeleteCertificateModal.tsx (1)
frontend/services/entities/certificates.ts (1)
ICertificate(55-61)
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx (3)
frontend/services/entities/certificates.ts (1)
ICertificate(55-61)frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts (3)
generateFormValidations(31-95)IAddCertFormValidation(4-9)validateFormData(107-135)frontend/utilities/constants.tsx (1)
DEFAULT_USE_QUERY_OPTIONS(451-461)
frontend/services/entities/certificates.ts (2)
frontend/services/entities/common.ts (2)
PaginationParams(14-17)ListEntitiesResponsePaginationCommon(2-5)frontend/utilities/url/index.ts (1)
buildQueryStringFromParams(101-113)
🔇 Additional comments (30)
frontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ProfileListItem/ProfileListItem.tsx (1)
2-2: LGTM!The blank line improves import organization by separating the React import from third-party libraries.
frontend/__mocks__/mdmMock.ts (1)
64-64: LGTM!The addition of
certificates: []to the mock data structure correctly aligns with the new certificate management feature and theIHostCert[]type definition.frontend/interfaces/host.ts (2)
100-116: LGTM!The new type definitions for host certificates are well-structured and align with the certificate management feature. The
HostCertStatusunion type appropriately covers all certificate states, and the comment clarifying UI display behavior for pending states is helpful.
123-123: LGTM!The addition of
certificates: IHostCert[]to theIOSSettingsinterface correctly extends the OS settings structure to support certificate data.frontend/__mocks__/hostMock.ts (1)
79-79: LGTM!The addition of
certificates: []to the host mock correctly aligns with the newIOSSettings.certificatesfield and matches the corresponding change inmdmMock.ts.changes/36689-update-android-certs-from-ui (1)
1-1: LGTM!The changelog entry clearly and concisely describes the implemented feature.
frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/CertificateAuthorities.tsx (1)
141-141: LGTM!The title change from "Certificates" to "Certificate authorities" improves clarity and helps distinguish this section from the new Android certificates feature introduced in this PR.
frontend/utilities/endpoints.ts (1)
313-313: No action needed. Verification confirms there is only one CERTIFICATES endpoint declaration at line 313. No duplicate exists.Likely an incorrect or invalid review comment.
frontend/pages/ManageControlsPage/components/UploadListHeading/_styles.scss (1)
1-18: The UploadListHeading component correctly uses all updated class names.All CSS class names have been properly updated in the component. The component uses
baseClass = "upload-list-heading"and references all nested classes (__upload-name-heading,__actions-heading,__add-button) correctly. No oldprofile-list-headingreferences remain.frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/index.ts (1)
1-1: LGTM!The barrel export follows the established pattern used consistently across the codebase.
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/_styles.scss (1)
1-14: LGTM!The styling is well-structured. The monospace font for the textarea is particularly appropriate for certificate content (PEM format).
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/_styles.scss (1)
1-10: LGTM!The flexbox layout with gap is clean and modern. The fixed height appears consistent with other OS Settings cards.
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/index.ts (1)
1-1: LGTM!The barrel export follows the established pattern.
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateCard/_styles.scss (1)
1-20: LGTM!The styling effectively centers the content and follows BEM naming conventions. The use of flexbox for centering is clean and appropriate.
frontend/pages/ManageControlsPage/OSSettings/OSSettingsNavItems.tsx (1)
8-8: No action required. The Certificates component correctly defines its props as extendingIOSSettingsCommonPropswith an optionalcurrentPageproperty, confirming full compatibility with the navigation integration in OSSettingsNavItems.tsx.frontend/pages/admin/IntegrationsPage/IntegrationNavItems.tsx (1)
47-48: Path definition is correctly aligned with the new URL structure.The rename from "Certificates" to "Certificate authorities" is fully implemented and intentional. The
ADMIN_INTEGRATIONS_CERTIFICATE_AUTHORITIESpath constant infrontend/router/paths.ts(line 62) is correctly set to${INTEGRATIONS_PREFIX}/certificate-authorities, matching theurlSectionvalue inIntegrationNavItems.tsx. All components—title, urlSection, path constant, and Card component—are properly aligned.frontend/pages/ManageControlsPage/components/UploadListHeading/index.ts (1)
1-1: Refactor from ProfileListHeading to UploadListHeading is complete.The barrel export is correct. All references have been properly updated—no remaining ProfileListHeading references exist, and UploadListHeading is correctly imported and used in Certificates.tsx and CustomSettings.tsx.
frontend/__mocks__/certificatesMock.ts (1)
68-80: LGTM!The Android certificate mock follows the established pattern in this file and correctly implements the
ICertificateinterface. The factory function provides good flexibility for test customization.frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/index.ts (1)
1-1: LGTM!Standard barrel export pattern for cleaner imports.
frontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/CustomSettings.tsx (1)
29-29: LGTM!The refactoring to use the shared
UploadListHeadingcomponent is well-executed. The new props (entityName,createEntityText,onClickAdd) make the component more flexible and reusable.Also applies to: 175-179
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tests.tsx (1)
51-332: Excellent test coverage!The test suite comprehensively covers:
- Modal rendering with all form fields
- Validation for empty, invalid, duplicate, and too-long names
- Required field validation for CA and subject name
- Full happy path flow
- Cancel functionality
The tests are well-structured and use appropriate testing patterns with MSW for API mocking.
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/DeleteCertificateModal.tsx (1)
22-33: LGTM!The delete handler properly manages loading states and provides clear user feedback through flash messages.
frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateCard/AddCertificateCard.tsx (1)
13-35: LGTM!The component is clean and follows established patterns. The GitOps mode integration ensures the Add button is properly disabled when GitOps is enabled.
frontend/pages/ManageControlsPage/components/UploadListHeading/UploadListHeading.tsx (2)
10-20: Excellent refactoring for reusability!The updated props interface makes this component generic and reusable across different contexts. The new prop names (
entityName,createEntityText,onClickAdd) are more descriptive and self-documenting than the previousonClickAddProfile.
8-8: LGTM!The
baseClassand related CSS class references are consistently updated to match the new component name.Also applies to: 23-23
frontend/router/paths.ts (1)
62-62: The path rename is safe and complete—all references have been properly updated.No hardcoded references to the old
/integrations/certificatespath remain in the codebase. The new constantADMIN_INTEGRATIONS_CERTIFICATE_AUTHORITIESis actively used in navigation (IntegrationNavItems.tsx) and the certificate modal (AddCertificateModal.tsx), with all consumers importing from the paths definition. This is a clean migration with no breaking changes.frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx (1)
100-119: Form submission logic looks correct.The form validates before enabling submission (line 182), ensuring
certAuthorityIdis non-empty beforeparseIntis called (line 107). Error handling provides user feedback, and the success flow properly triggers callbacks.frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/helpers.ts (2)
31-95: Well-structured validation framework.The validation logic correctly implements:
- Sequential validation with early exit on first failure
- Case-insensitive uniqueness checking (line 56)
- Appropriate character restrictions for certificate names
- Clear error messages for each validation rule
The ordering of validations ensures users see the most relevant error first (required → character validation → uniqueness → length).
107-135: Validation execution logic is sound.The function correctly aggregates field-level validation results and maintains an overall
isValidflag. The use ofObject.keyswith type assertion (line 116) safely iterates over the validation configuration.frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/Certificates.tsx (1)
59-83: Query configuration and gating logic are correct.The certificates query is properly gated behind premium tier and Android MDM checks (line 81), uses structured query keys for cache management, and follows the established react-query patterns in the codebase.
GitHub Actions
Inactive
| export type HostCertStatus = | ||
| | "pending_removal" | ||
| | "removing" | ||
| | "removed" |
There was a problem hiding this comment.
We will not have these 3. We will use operation_type=remove and pending/delivering/delivered instead.
There was a problem hiding this comment.
so "verified" | "failed" | "pending" | "delivering" | "delivered"?
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
My review is in progress. Going slow since I'm not that familiar with React/frontend.
|
|
||
| return ( | ||
| <SettingsSection title="Certificates"> | ||
| <SettingsSection title="Certificate authorities"> |
| disabled={disableChildren} | ||
| className={`${baseClass}__card--add-button`} | ||
| type="button" | ||
| onClick={() => setShowModal(true)} |
There was a problem hiding this comment.
Question. It seems this component knows more about the parent than it should. At the very least, wouldn't it be better for this line to be like onClick={onClick}?
Minor issue, but maybe you've discussed it in Frontend sync.
A random reference: https://matanbobi.dev/posts/stop-passing-setter-functions-to-components
There was a problem hiding this comment.
Yea we often will define a toggleModal function like const toggleModal = () => {setShowModal(!showModal)} but I feel it's excessive and writing it this way is very clear and concise
There was a problem hiding this comment.
I just looked at the article you linked. The issue it's flagging would be if here the Button component expected a setShowModal function passed to it that it would then call. This is not what's happening - we're already separating the abstractions by passing the callbackfunction () => setShowModal(true) to the onClick prop, which as you can see here makes very minimal assumptions about how that prop's value should look.
There was a problem hiding this comment.
i.e., Button calls its onClick without needing to know anything about what it is.
There was a problem hiding this comment.
That typing IS a little messy now that I look at it, seems people were attempting to make it useful for both click events and keydown events, but that's a different issue
| const nameInput = screen.getByPlaceholderText("VPN certificate"); | ||
|
|
||
| const longName = "a".repeat(256); | ||
| await user.type(nameInput, longName); |
There was a problem hiding this comment.
Nit. Typing 256 characters one by one feels slow. How about:
fireEvent.change(nameInput, { target: { value: longName } });
There was a problem hiding this comment.
We want to test as close to the actual user interaction as possible, and in this case it would actually be typing in the name, not programatically changing the field's value
There was a problem hiding this comment.
The alternative would be click on the field and paste a long value. That would speed up the test and be something that an actual user could do.
GitHub Actions
Inactive
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Did a partial review. Approving since we need these changes for customer.
2 participants
Related issue: Resolves #36689
If some of the following don't apply, delete the relevant line.
changes/Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.