Hide Level column and system admin app identities from non-system-admin users#1271
Merged
Hide Level column and system admin app identities from non-system-admin users#1271
Conversation
…in users Motivation: The App Identities settings page exposed the "Level" column (showing System Admin / User role) to all users, including non-system-admins who do not need visibility into that administrative distinction. Modifications: - Filter out system admin app identities from the list returned to non-system-admin callers. - webapp/src/pages/app/settings/app-identities/index.tsx: - Moved the "Type" column after the "Application ID" column for better visual ordering. - Conditionally render the "Level" column only when the current user is a system admin. Result: Non-system-admin users will no longer see the "Level" column in the App Identities table, and system admin app identities will be excluded from the list served to them.
minwoox
changed the title
📝 WalkthroughWalkthroughBackend now filters out system-admin app identities for non-admin callers; frontend conditionally shows an admin-only "Level" column based on user systemAdmin state and updates helper texts; tests added to verify conditional column rendering and row contents. Changes
Sequence DiagramsequenceDiagram
participant User as User (Non-Admin)
participant Frontend as Frontend UI
participant Backend as Backend API
User->>Frontend: Navigate to App Identities page
Frontend->>Backend: GET /app-identities
Backend->>Backend: Filter out system-admin identities for non-admin callers
Backend-->>Frontend: Return filtered identities
Frontend->>Frontend: Read systemAdmin from store (false)
Frontend-->>User: Render table without Level column
Note over Frontend,Backend: For system-admin users the backend returns non-secret identities including admins and frontend renders Level column
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
🧹 Nitpick comments (2)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java (1)
305-323: Consider applying the same filtering to the deprecatedlistTokensmethod for consistency.The new
listAppIdentitiesmethod filters out system-admin identities for non-admin callers, but the deprecatedlistTokensmethod (lines 314-322) only strips secrets without filtering out system-admin tokens. This inconsistency could allow non-admin users to see system-admin tokens via the deprecated endpoint.If the deprecated endpoint is still accessible, consider applying the same filter:
🔧 Suggested fix
} else { return mds.getAppIdentityRegistry() - .appIds() + .withoutSecret() + .appIds() .values() .stream() .filter(appIdentity -> appIdentity.type() == AppIdentityType.TOKEN) + .filter(appIdentity -> !appIdentity.isSystemAdmin()) .map(appIdentity -> (Token) appIdentity) - .map(Token::withoutSecret) .collect(toImmutableList()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java` around lines 305 - 323, The deprecated method listTokens currently returns all TOKEN app identities with secrets stripped for non-admins but does not filter out system-admin identities; update listTokens to apply the same non-admin filtering used in listAppIdentities by filtering appIdentity -> appIdentity.type() == AppIdentityType.TOKEN and excluding entries where appIdentity.isSystemAdmin() (or equivalent check) for non-admin callers before mapping to (Token) appIdentity and Token::withoutSecret, preserving the existing behavior for system admins.webapp/tests/pages/app/settings/app-identities/index.test.tsx (1)
55-91: Good test coverage for the Level column visibility feature.The tests correctly verify:
- Level column is hidden for non-system-admin users
- Level column is shown for system-admin users
- Badges render correctly within the Level column
- Identity rows are displayed in the table
Consider adding a test for the new Type column to ensure Token/Certificate badges render correctly:
💡 Optional: Test for Type column badges
it('renders Type badges for identities', () => { const { getAllByText } = renderWithProviders(<AppIdentityPage />, { preloadedState: { auth: baseAuthState }, }); // Both mock identities are TOKEN type expect(getAllByText('Token')).toHaveLength(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/tests/pages/app/settings/app-identities/index.test.tsx` around lines 55 - 91, Add a new test in the same suite that renders AppIdentityPage with renderWithProviders and preloadedState: { auth: baseAuthState } to assert the Type column shows correct badges; specifically create a test titled like "renders Type badges for identities" that calls getAllByText('Token') (or the localized label used by the Type badge) and expects the length to match the two mock identities (both TOKEN type) so the Token badges are rendered for each identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/src/pages/app/settings/app-identities/index.tsx`:
- Line 20: The code reads state.auth.user.systemAdmin directly in the
useAppSelector call (assigning to systemAdmin), which can throw if user is null;
change the selector to safely access state.auth.user (e.g., use optional
chaining and a default) so it returns a boolean when user is null (update the
useAppSelector selector that computes systemAdmin to use
state.auth.user?.systemAdmin ?? false or similar).
---
Nitpick comments:
In
`@server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.java`:
- Around line 305-323: The deprecated method listTokens currently returns all
TOKEN app identities with secrets stripped for non-admins but does not filter
out system-admin identities; update listTokens to apply the same non-admin
filtering used in listAppIdentities by filtering appIdentity ->
appIdentity.type() == AppIdentityType.TOKEN and excluding entries where
appIdentity.isSystemAdmin() (or equivalent check) for non-admin callers before
mapping to (Token) appIdentity and Token::withoutSecret, preserving the existing
behavior for system admins.
In `@webapp/tests/pages/app/settings/app-identities/index.test.tsx`:
- Around line 55-91: Add a new test in the same suite that renders
AppIdentityPage with renderWithProviders and preloadedState: { auth:
baseAuthState } to assert the Type column shows correct badges; specifically
create a test titled like "renders Type badges for identities" that calls
getAllByText('Token') (or the localized label used by the Type badge) and
expects the length to match the two mock identities (both TOKEN type) so the
Token badges are rendered for each identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef17aa22-6cd3-4098-9367-b739d104cd35
📒 Files selected for processing (4)
server/src/main/java/com/linecorp/centraldogma/server/internal/api/sysadmin/AppIdentityRegistryService.javawebapp/src/dogma/features/app-identity/NewAppIdentity.tsxwebapp/src/pages/app/settings/app-identities/index.tsxwebapp/tests/pages/app/settings/app-identities/index.test.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/src/pages/app/settings/app-identities/index.tsx (1)
21-22: Consider movingcolumnHelperoutside the component for effective memoization.
columnHelperis created inside the component (line 21), causing its reference to change on every render. Since it's included in theuseMemodependency array, the memoization becomes ineffective.This is pre-existing behavior, but now that
systemAdminis properly tracked, the memoization could be made effective by movingcolumnHelperoutside the component.♻️ Suggested refactor
+const columnHelper = createColumnHelper<AppIdentityDto>(); + const AppIdentityPage = () => { const systemAdmin = useAppSelector((state) => state.auth.user?.systemAdmin ?? false); - const columnHelper = createColumnHelper<AppIdentityDto>(); const columns = useMemo( () => [ // ... columns definition ], - [columnHelper, systemAdmin], + [systemAdmin], );Also applies to: 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webapp/src/pages/app/settings/app-identities/index.tsx` around lines 21 - 22, The columnHelper is recreated each render because createColumnHelper<AppIdentityDto>() is called inside the component, invalidating the useMemo for columns; move the declaration of columnHelper (the result of createColumnHelper<AppIdentityDto>()) to module scope (outside the component) so its reference is stable, then update the useMemo that builds columns (the columns variable) to remove columnHelper from its dependencies and keep only truly reactive deps (e.g., systemAdmin), ensuring effective memoization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@webapp/src/pages/app/settings/app-identities/index.tsx`:
- Around line 21-22: The columnHelper is recreated each render because
createColumnHelper<AppIdentityDto>() is called inside the component,
invalidating the useMemo for columns; move the declaration of columnHelper (the
result of createColumnHelper<AppIdentityDto>()) to module scope (outside the
component) so its reference is stable, then update the useMemo that builds
columns (the columns variable) to remove columnHelper from its dependencies and
keep only truly reactive deps (e.g., systemAdmin), ensuring effective
memoization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 918e42d3-8abb-422c-b7ea-44946684b50d
📒 Files selected for processing (1)
webapp/src/pages/app/settings/app-identities/index.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
The App Identities settings page exposed the "Level" column (showing System Admin / User role) to all users, including non-system-admins who do not need visibility into that administrative distinction.
Modifications:
Result:
Non-system-admin users will no longer see the "Level" column in the App Identities table, and system admin app identities will be excluded from the list served to them.