-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(auth): NASS-1534: Add new auth package and move central code over #7195
base: main
Are you sure you want to change the base?
Conversation
…b.com/beyondessential/tamanu into nass-1534-dry-up-auth-between-servers
deviceId, | ||
}, | ||
{ | ||
where: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was throwing type error and I think (still to test) should work without
// Send some additional data with login to tell the user about | ||
// the context they've just logged in to. | ||
res.send({ | ||
token, | ||
refreshToken, | ||
user: convertFromDbRecord(stripUser(user.get({ plain: true }))).data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stripuser was stripping password which covertFromDbRecord does anyway
packages/auth/src/login/refresh.ts
Outdated
asyncHandler(async (req, res) => { | ||
export const refresh = ({ secret, refreshSecret }: { secret: string; refreshSecret: string }) => | ||
asyncHandler(async (req: any, res): Promise<void> => { | ||
const { body, store } = req; | ||
const { refreshToken, deviceId } = body; | ||
|
||
const { canonicalHostName, auth } = config; | ||
const { canonicalHostName, auth } = config as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both req
and config
will need to get their types properly handled but seems out of scope of this card
38133bc
to
285ce2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
packages/auth/src/login/login.ts
Outdated
secret: string; | ||
refreshSecret: string; | ||
getLocalisation: Function; | ||
}) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the pattern of having an emport interface LoginArgs {...}
that's then used where needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like you could do
export interface AuthArgs {
secret: string;
refreshSecret: string;
}
export interface LoginArgs extends AuthArgs {
getLocalisation: Function<foo>;
}
and the AuthArgs can be used directly by the refresh function as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yep i got you 🙏
@@ -0,0 +1,47 @@ | |||
type DbRecord = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think interfaces are prefered over typedefs in ts style
"target": "es2020" | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOS but we should really consider moving a bunch of those settings to be defaults in the common file.
Changes
Add a brief description of the changes in this PR to help give the reviewer context.
Deploys
Remember to...