feat(committees): add access-denied permission check on mailing list CTAs#1016
feat(committees): add access-denied permission check on mailing list CTAs#1016MRashad26 wants to merge 7 commits into
Conversation
On '+ Add List' click in committee settings, perform a fresh committee fetch before opening the picker dialog; redirect to overview with _notice=mailing-lists if the member no longer has writer access. On '+ Create New List' click inside the picker dialog, perform the same fresh committee check; close the dialog and redirect on denial, or navigate to /mailing-lists/manage on success. Both handlers use lens-aware redirect (foundation vs project overview) consistent with the votes and surveys permission-check pattern. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
…V2-2252 Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe mailing list picker and create-new-list actions in committee settings now check committee writer permission before proceeding. Non-writers are redirected to the appropriate overview route with a mailing-lists notice. ChangesWriter-gated mailing list flows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-1016.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
There was a problem hiding this comment.
Pull request overview
Adds a “fresh permission check on click” flow for committee mailing list CTAs to avoid stale committee.writer state, aligning committees → mailing-lists behavior with the existing access-denied redirect + _notice toast pattern used elsewhere.
Changes:
- Add a fresh
getCommittee()permission check before opening the mailing list picker dialog (“+ Add List”) and redirect to the lens-aware overview on denial. - Replace the picker dialog’s “Create New List” routerLink with a click handler that performs the same fresh permission check and redirects (or navigates to
/mailing-lists/manageon success). - Pass
committeeUidinto the dialog viaDynamicDialogConfig.dataso the dialog can re-check permissions independently.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/lfx-one/src/app/modules/committees/components/mailing-list-picker-dialog/mailing-list-picker-dialog.component.ts | Adds onCreateNewList() with a fresh committee permission check and redirect/navigation behavior. |
| apps/lfx-one/src/app/modules/committees/components/mailing-list-picker-dialog/mailing-list-picker-dialog.component.html | Switches “Create New List” to a click handler (previously routerLink-based). |
| apps/lfx-one/src/app/modules/committees/components/committee-settings-tab/committee-settings-tab.component.ts | Adds a fresh permission check before opening the picker dialog and passes committeeUid into dialog data. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review comments from @copilot-pull-request-reviewer: - mailing-list-picker-dialog.component.html: replace <a> with <button type="button"> for keyboard accessibility (per @copilot) - mailing-list-picker-dialog.component.ts: build denyParams inside next() so fresh.project_slug can be included in the redirect query params, matching the pattern in committee-settings-tab (per @copilot) Resolves 2 review threads. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/lfx-one/src/app/modules/committees/components/committee-settings-tab/committee-settings-tab.component.ts`:
- Around line 117-135: The async getCommittee() callback can execute after
component state changes, causing mixed snapshot data to be used when opening the
dialog or redirecting. Capture all necessary state values (committee properties,
lens, and navigation context) into local const variables at the start of the
function before the async operation begins, and ensure both the deny() callback
and openPickerDialog(committee.uid) call use only these captured local variables
rather than fresh signal reads that might return different values due to state
mutations during the async operation.
In
`@apps/lfx-one/src/app/modules/committees/components/mailing-list-picker-dialog/mailing-list-picker-dialog.component.html`:
- Around line 21-22: The anchor element with the onCreateNewList() click handler
lacks proper button semantics and is not keyboard-accessible. Replace the anchor
element with a button element using type="button", keeping the existing
(click)="onCreateNewList()" handler and the styling classes intact. This ensures
the Create New List control is properly focusable and activatable via keyboard
navigation, meeting the accessibility requirements for interactive controls in
the component.
In
`@apps/lfx-one/src/app/modules/committees/components/mailing-list-picker-dialog/mailing-list-picker-dialog.component.ts`:
- Around line 85-90: The deny flow in the mailing-list-picker-dialog component
does not preserve project context when navigating to the overview page. Modify
the deny function to extract the project information from the fetched committee
payload and add it as a query parameter to denyParams when the active lens is
'project' (not 'foundation'). This ensures project context is maintained in the
navigation URL, consistent with the settings-tab deny flow. Apply the same fix
to both the deny callback function and the confirm flow callback function to
ensure consistency across all navigation paths in this component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c2ac82a-e8aa-4584-8218-daa8cb5f4030
📒 Files selected for processing (3)
apps/lfx-one/src/app/modules/committees/components/committee-settings-tab/committee-settings-tab.component.tsapps/lfx-one/src/app/modules/committees/components/mailing-list-picker-dialog/mailing-list-picker-dialog.component.htmlapps/lfx-one/src/app/modules/committees/components/mailing-list-picker-dialog/mailing-list-picker-dialog.component.ts
Review Feedback AddressedCommit: 84bdf58 Changes Made
Threads Resolved2 of 2 unresolved threads addressed in this iteration. |
Address review comments from @coderabbitai: - committee-settings-tab.component.ts: capture committeeUid and projectUid as local consts before the async getCommittee() call so the callback always operates on click-time snapshot values rather than potentially-stale signal reads (per @coderabbitai) - committee-settings-tab.component.ts: add takeUntilDestroyed() to the getCommittee() pipe so the callback is cancelled if the component is destroyed mid-request (per @coderabbitai) - committee-settings-tab.component.ts: pass projectUid into openPickerDialog() instead of re-reading this.committee().project_uid inside the method (per @coderabbitai) Resolves 1 review thread. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: 3cc6f1c Changes Made
Threads Resolved1 of 1 unresolved threads addressed in this iteration. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
Address review comments from @copilot-pull-request-reviewer: - mailing-list-picker-dialog.component.ts: add takeUntilDestroyed() to onCreateNewList() pipe so the navigation callback is cancelled if the dialog is destroyed before the getCommittee() request resolves (per @copilot) - committee-settings-tab.component.ts: tighten openPickerDialog() signature from string|undefined to string — Committee.project_uid is required in the shared interface, so the undefined arm and ?? '' fallback were unnecessary (per @copilot) Resolves 2 review threads. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: e432494 Changes Made
Threads Resolved2 of 2 unresolved threads addressed in this iteration. |
Audit: PR #1016 — feat(committees): add access-denied permission check on mailing list CTAsMRashad26 · Scope: Fresh Lanes run: 1. Why this change
2. How it was done
3. Is it correctYes — merge-ready. Pattern aligns with #997/#1000. Prior review: Async state capture → fixed (3cc6f1c). Button a11y → fixed. Success-path project slug → fixed. Error-path project slug → still open (see inline). Save/remove while dialog open → no second check (same scope as sibling PRs; server enforces on PUT). 4. Missing / gaps
5. Q&A this PR resolves
6. Business rules & ADRs
7. Open questions & confirmations
Verdict: 🟡 Minor comments |
luismoriguerra
left a comment
luismoriguerra
left a comment
There was a problem hiding this comment.
Audit follow-up: one remaining non-blocking consistency nit on the error-path deny params (inline). Top-level audit report posted separately.
ahmedomosanya
left a comment
ahmedomosanya
left a comment
There was a problem hiding this comment.
Audit result: 🔴 Required changes
The PR references LFXV2-2252, but that is the parent Org Lens parity epic. The specific ticket appears to be LFXV2-2469 (“Allow committee writers to create/manage mailing lists for their committee”). LFXV2-2484 may also be relevant if this is intended to address the reported mailing-list 403 bug, but LFXV2-2469 is the closest feature ticket.
Blocking issues:
onCreateNewList()navigates to/mailing-lists/manage, but the router defines/mailing-lists/createand/:id. As written,manageis treated as a mailing-list id rather than the create page.- The create navigation passes
project_uid, butwriterGuardresolves write access from?project=<slug>or active context. This should follow the same slug-based query-param contract used by the votes/surveys helper so guarded navigation works reliably.
Additional note:
openMailingListPicker()treats everygetCommittee()failure as access denied, so transient 5xx/network failures can redirect valid writers with a misleading_notice=mailing-liststoast.
Local checks passed: yarn lint:check, yarn check-types, and yarn format:check.
Address @luismoriguerra review on PR #1016: the error() callback in onCreateNewList() was redirecting with only { _notice: 'mailing-lists' }, while the success-path deny correctly included project_slug. On a transient fetchCommittee failure, the toast would lose project context. Fix: pass projectSlug through openPickerDialog() into the dialog config, then construct the same denyParams shape in the error handler. Resolves 1 review thread. Signed-off-by: Rashad <mrashad@contractor.linuxfoundation.org>
Review Feedback AddressedCommit: e0dade3 Changes Made
Threads Resolved1 of 1 unresolved threads resolved. |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
@MRashad26 — clean implementation that closes out a well-iterated review cycle.
This follows the permission-check pattern established by meetings (#992), votes (#997), and surveys (#1000), and actually improves on the original meetings implementation by adding takeUntilDestroyed and lens-aware redirects — both of which committee-meetings is still missing.
All prior findings from Copilot (×2), CodeRabbit, and @luismoriguerra have been addressed across the five iteration commits. The snapshot-before-async pattern, tightened projectUid: string signature, proper <button type="button"> semantics, and project_slug in both deny paths are all confirmed correct in the current code.
Security review confirms no new risks: overviewPath is derived from an internal signal (no open-redirect risk), query params come from server-returned data (no injection risk), and the upstream microservice enforces the real authorization boundary via the user's bearer token.
🟡 Minor: 1 — test coverage gap for async permission-check paths
⚪ Nit: 1 — duplicate denyParams construction in two error() paths
✅ Approved with minor comments
| @@ -10,6 +10,7 @@ import { TagComponent } from '@components/tag/tag.component'; | |||
| import { Committee, CreateMailingListRequest, GroupsIOMailingList, JoinMode, MailingListPickerDialogResult } from '@lfx-one/shared/interfaces'; | |||
There was a problem hiding this comment.
[minor] No automated test coverage for the new async permission-check paths. The denial flow (writer !== true) and error fallback in both openMailingListPicker() and onCreateNewList() are only covered by the manual test plan.
The sibling implementations (meetings, votes, surveys) also lack these tests — so this is consistent with the current repo pattern. A Jasmine unit test mocking CommitteeService.getCommittee() to return a non-writer response would be the minimum addition if test coverage is required here; otherwise, document this as a known gap consistent with peer implementations.
| private readonly projectSlug: string | undefined = this.config.data.projectSlug; | ||
|
|
||
| public selectedMailingListUids = signal<Set<string>>(new Set(this.config.data.associatedUids)); | ||
| public mlSearchQuery = signal(''); |
There was a problem hiding this comment.
[nit] The error() handler builds its own errorDenyParams object rather than reusing the same construction as the next() deny path. The two blocks are nearly identical — the only difference is the source of the slug (this.projectSlug vs fresh.project_slug). A one-line pre-capture of the slug before subscribe (as done in the settings-tab) would unify both paths:
const projectSlug = this.projectSlug; // captured once
// ... then in both next-deny and error:
const denyParams: Record<string, string> = { _notice: 'mailing-lists' };
if (projectSlug) denyParams['project'] = projectSlug;Not a functional issue — just a consistency note.
Summary
+ Add Listbutton (committee settings): on click, performs a freshgetCommittee()call; ifwriter !== true, redirects to the lens-aware overview (/foundation/overviewor/project/overview) with_notice=mailing-liststriggering the "Access Denied" toast — consistent with the meetings, votes, and surveys pattern from LFXV2-2252+ Create New Listlink (picker dialog): replaced[routerLink]navigation with anonCreateNewList()click handler that performs the same fresh committee check; closes the dialog and redirects on denial, or navigates to/mailing-lists/manageon successcommitteeUidis now passed through the dialog'sdataobject so the dialog can perform the check independently without relying on stale parent-component stateRelated PRs
Test plan
+ Add List→ picker opens normally+ Create New Listinside the picker → navigates to/mailing-lists/manage+ Add List→ redirected to project overview with "Access Denied" toast+ Create New List→ dialog closes, redirected to project overview with "Access Denied" toast/foundation/overviewnot/project/overview🤖 Generated with Claude Code