feat(Clusters): authorize with meta #2988
Conversation
Raubzeug
requested review from
adameat,
artemmufazalov and
astandrik
as code owners
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/containers/App/Content.tsx
Outdated
| const authUnavailable = useClusterWithoutAuthInUI(); | ||
|
|
||
| const location = useLocation(); | ||
| const isClustersPage = location.pathname === '/clusters'; |
There was a problem hiding this comment.
logic: hardcoded pathname check won't work with basename (e.g., /monitoring/clusters). use location.pathname.endsWith(routes.clusters) or match against the route pattern
| const isClustersPage = location.pathname === '/clusters'; | |
| const isClustersPage = location.pathname.endsWith(routes.clusters); |
| const needDatabase = useLoginWithDatabase(); | ||
|
|
||
| const [authenticate, {isLoading}] = authenticationApi.useAuthenticateMutation(undefined); | ||
| const isClustersPage = location.pathname === '/clusters'; |
There was a problem hiding this comment.
logic: hardcoded pathname check won't work with basename (e.g., /monitoring/clusters). use location.pathname.endsWith(routes.clusters) or match against the route pattern
| const isClustersPage = location.pathname === '/clusters'; | |
| const isClustersPage = location.pathname.endsWith(routes.clusters); |
Raubzeug
force-pushed
the
clusters-auth
branch
from
2469ead to
6157bc5
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds authentication support for the clusters page using meta API endpoints. When meta login capability is available, the authentication flow uses /meta/login and /meta/whoami instead of the standard viewer endpoints.
Key Changes:
- Added meta authentication endpoints (
/meta/logincapability and API methods) - Modified authentication flows to conditionally use meta endpoints based on page context and capability availability
- Enhanced route detection with reusable helper functions
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/api/capabilities.ts | Added /meta/login capability type |
| src/store/reducers/capabilities/hooks.ts | Added hook to check meta login availability |
| src/store/reducers/authentication/authentication.ts | Modified authentication mutations to support meta endpoints |
| src/services/api/meta.ts | Implemented meta authentication API methods |
| src/routes.ts | Added reusable route checking helper functions |
| src/containers/Header/Header.tsx | Refactored to use new route helper functions |
| src/containers/Authentication/Authentication.tsx | Added meta authentication support in login form |
| src/containers/App/Content.tsx | Added conditional meta user authentication wrapper for clusters page |
| const {useMeta, ...rest} = params; | ||
| let data; | ||
| if (useMeta) { | ||
| data = await window.api.meta?.metaAuthenticate(rest); |
There was a problem hiding this comment.
Optional chaining on window.api.meta?.metaAuthenticate could result in data being undefined if window.api.meta is null/undefined. This would cause the subsequent dispatch(setIsAuthenticated(true)) to execute even when authentication fails. Add a null check or use non-null assertion if the condition guarantees window.api.meta exists.
| data = await window.api.meta?.metaAuthenticate(rest); | |
| if (!window.api.meta) { | |
| throw new Error('Meta API is not available for authentication'); | |
| } | |
| data = await window.api.meta.metaAuthenticate(rest); |
| const isClustersPage = checkIsClustersPage(location.pathname); | ||
| const isMetaLoginAvailable = useMetaLoginAvailable(); | ||
|
|
||
| const [authenticate, {isLoading}] = authenticationApi.useAuthenticateMutation(); |
There was a problem hiding this comment.
[nitpick] The undefined parameter was removed but the function call was changed from useAuthenticateMutation(undefined) to useAuthenticateMutation(). While functionally equivalent, the explicit change in the diff suggests this may have been intentional. Verify this change is necessary and not just a formatting modification.
| return pathname.endsWith(routes.clusters); | ||
| } | ||
|
|
||
| export function checkIsTenantPage(pathname: string) { | ||
| return pathname.endsWith(routes.tenant); |
There was a problem hiding this comment.
Using endsWith() for route matching is fragile and will produce false positives. For example, /some/other/clusters would match even though it's not the clusters page. Use exact matching or a more robust pattern matching approach: pathname === routes.clusters or use a library like matchPath from react-router.
| return pathname.endsWith(routes.clusters); | |
| } | |
| export function checkIsTenantPage(pathname: string) { | |
| return pathname.endsWith(routes.tenant); | |
| return pathname === routes.clusters; | |
| } | |
| export function checkIsTenantPage(pathname: string) { | |
| return pathname === routes.tenant; |
| metaAuthenticate(params: {user: string; password: string}) { | ||
| return this.post(this.getPath('/meta/login'), params, {}); |
There was a problem hiding this comment.
The metaAuthenticate method lacks a return type annotation. Based on the usage in authentication.ts where the result is dispatched, add an explicit return type such as Promise<TUserToken> or the appropriate type for the authentication response.
| metaAuthenticate(params: {user: string; password: string}) { | |
| return this.post(this.getPath('/meta/login'), params, {}); | |
| metaAuthenticate(params: {user: string; password: string}): Promise<TUserToken> { | |
| return this.post<TUserToken>(this.getPath('/meta/login'), params, {}); |
closes #2956
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: 🔺
Current: 45.87 MB | Main: 45.86 MB
Diff: +4.71 KB (0.01%)
ℹ️ CI Information