feat: remove users 😵 #1344
feat: remove users 😵 #1344
Conversation
Signed-off-by: Luke McFarlane <luke.mcfarlane@rocketlab.com.au>
Signed-off-by: Luke McFarlane <luke.mcfarlane@rocketlab.com.au>
Signed-off-by: Luke McFarlane <luke.mcfarlane@rocketlab.com.au>
Signed-off-by: Luke McFarlane <luke.mcfarlane@rocketlab.com.au>
|
@luke-mcfarlane-rocketlab please resolve the conflicts. Thanks. :-) |
| 'Only cluster admins can remove users.' | ||
| ); | ||
|
|
||
| if (!id) throw new Exceptions.ValidationException('User ID not specified'); |
There was a problem hiding this comment.
This is a good validation , however can you please also add checks for invalid user?
There was a problem hiding this comment.
not sure I understand what you mean by invalid user? The user calling the API or the user being removed?
web/src/routes/_protected.tsx
Outdated
| function RouteComponent() { | ||
| const auth = useAuth(); | ||
|
|
||
| if (!auth.user) return <></>; |
There was a problem hiding this comment.
with the empty fragment, it causes the UI flickering, can you show a loading spinner instead?
| * @param {string} userId - The ID of the user to remove. | ||
| * @returns {JSX.Element} The rendered RemoveUserDialog component. | ||
| */ | ||
| export const RemoveUserDialog = ({userId}: {userId: string}) => { |
There was a problem hiding this comment.
if userId is missing or invalid , can we disable the delete button, it might leadd to a bad request.
| <Button | ||
| className="w-full" | ||
| onClick={async () => { | ||
| const response = await fetch( |
There was a problem hiding this comment.
directly fetching responses here is risky I believe,can you please wrap this in a try-catch block to handle network failures?
| return; | ||
| } | ||
|
|
||
| QueryClient.invalidateQueries({queryKey: ['users', undefined]}); |
There was a problem hiding this comment.
shouldn't be the query key = 'users' instead of 'users', undefined? wouldn't it cause query caching issues? thinking out loud let me know what you think. :-)
api/src/api/users.ts
Outdated
| ); | ||
|
|
||
| removeUser(userToRemove); | ||
|
|
There was a problem hiding this comment.
would it be possible to wrap this in try-catch to handle possible database failures?
|
|
||
| if (!users_db) throw new Error('Failed to connect to user database'); | ||
|
|
||
| if (!user._id || !user._rev) throw new Error('User not found'); |
There was a problem hiding this comment.
instead og generic error here, can we return a HTTP response code ? like 404 not found using exception handler?
we can say - throw new Exception.ItemNotFoundException('Usernot found in database.', 404);
let me know your thoughts. Thanks!
There was a problem hiding this comment.
This is not an endpoint - its just a function, but yes this is thrown from the endpoint
|
|
||
| const {status} = await getUserDetails(token, refreshToken); | ||
|
|
||
| if (status === 'success') return; |
There was a problem hiding this comment.
can we wrap this in try-catch please?
something like. if (status === 'success') return;
} catch (error) {
console.error('Failed to fetch user details:', error);
There was a problem hiding this comment.
resolved at the provider
|
Please also resolve the conflicts. :-) @luke-mcfarlane-rocketlab |
Signed-off-by: Luke McFarlane <luke.mcfarlane@rocketlab.com.au>
Signed-off-by: Luke McFarlane <luke.mcfarlane@rocketlab.com.au>
|
@ranisa-gupta16 all resolved |
JIRA Ticket
BSS-670
Description
Added the ability for cluster admins to remove users from the platform.
Proposed Changes
_protectedroute to include all pages which require auth so that we can have pages which do not require authHow to Test
Additional Information
I think we should consider removing the "reset password" functionality from the new conductor and replace it with an email flow in old conductor. The current approach would allow cluster-admins to impersonate users.
Checklist