Move auth to ms2 #196
Move auth to ms2 #196
Conversation
| } catch (error) { | ||
| errors.push({ roleId, error: error as Error }); | ||
| } | ||
| } | ||
|
|
||
| return errors; |
There was a problem hiding this comment.
I would change this to an early return as soon as you hit the first error.
The reason is that later with a proper database, these batch operations are usually atomic. So any error would fail the whole operation and not complete some and not others. So to not have to refactor the client side handlers, we should just return one error already.
For reference, you can look at the return userError() lines in lib/data/processes.tsx
There was a problem hiding this comment.
Fixed in one of my last commits
| } catch (error) { | ||
| errors.push({ roleId, error: error as Error }); | ||
| } | ||
| } | ||
|
|
||
| return errors; |
There was a problem hiding this comment.
Fixed in one of my last commits
| console.log( | ||
| 'leftkeys', | ||
| Object.keys(roleMetaObjects).map((r) => roleMetaObjects[r].name), | ||
| ); |
There was a problem hiding this comment.
I removed it in one of my last commits
There was a problem hiding this comment.
A general comment on throwing errors in server actions:
In production mode, React doesn't emit errors or rejected promises to the client. Instead a hash is sent representing the error. This hash can be used to associate multiple of the same errors together and associate the error with server logs. React replaces the error message with its own generic one.
https://nextjs.org/blog/security-nextjs-server-components-actions#error-handling
That is why for "expected" errors (like permissions, wrong id, etc.) I return a normal object with userError(). That way our UI can show the message and the user doesn't lose the state of the forms as would be the case by showing the error.tsx.
There was a problem hiding this comment.
Fixed in one of my last commits
| const { id: roleId, name } = addRole(role, new Ability(adminRules())); | ||
|
|
||
| if (process.env.NODE_ENV === 'development') { | ||
| const roleMappings = developmentRoleMappingsMigrations | ||
| .filter((mapping) => mapping.roleName === name) | ||
| .map((mapping) => ({ | ||
| roleId, | ||
| userId: mapping.userId, | ||
| })); | ||
|
|
||
| addRoleMappings(roleMappings, new Ability(adminRules())); |
There was a problem hiding this comment.
Is it unproblematic that these are async functions?
There was a problem hiding this comment.
I forgot to remove async form addRole. I fixed it in one of my last commits
| ); | ||
|
|
||
| export const dynamic = 'force-dynamic'; |
There was a problem hiding this comment.
Not necessary as long as getCurrentUser() is called.
There was a problem hiding this comment.
Fixed in one of my last commits
| setSelectedRows([]); | ||
| router.refresh(); | ||
| } catch (e) { | ||
| messageApi.error((e as Error).message); |
There was a problem hiding this comment.
See other comment. This will always be a generic message in production.
There was a problem hiding this comment.
Fixed in one of my last commits
Details
TODO: users site still needs the old ms, since nextauth doesn't implement auth0's api