-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Mutations for forgot-password operation #117
Conversation
{ req, res }: ContextParams, | ||
redis: Redis | ||
): Context { | ||
export function buildContext({ req, res, redis }: ContextParams): Context { |
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 couldn't find any way to pass this redis client to buildcontext function to access it inside resolvers. So For the time being I hard-coded in the node-module. This function is from graphql-passport (node_modules/graphql-passport/lib/buildContext.d.ts). I think that is not the correct way to do this.
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 it should work similar to the prisma integration:
- Register it globally with tsryinge:
JabRefOnline/api/tsyringe.config.ts
Lines 6 to 8 in 0d608ba
container.register<PrismaClient>(PrismaClient, { useFactory: instanceCachingFactory<PrismaClient>(() => new PrismaClient()), }) - Declare it as constructor dependency in services
JabRefOnline/api/user/auth.service.ts
Line 16 in 0d608ba
constructor(private prisma: PrismaClient) {} - Use it
JabRefOnline/api/user/auth.service.ts
Line 19 in 0d608ba
const user = await this.prisma.user.findUnique({
I think only the service (and not the resolvers) should know about redis since where the data is stored is somewhat of an implementation detail.
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.
Thanks a lot for your contribution! Overall this looks very good already.
First a general remark: It would be good to split this PR into multiple smaller PRs that address only one thing, for example forgot password, redis store, error handling for authentication. Makes reviewing easier and also discussing things, since one doesn't have a few parallel strings of discussion in the same PR.
Concerning the concrete implementation:
- Contrary to what I said earlier, I agree we should just use redis as a session store. It is clear that this is used quite often, and it's the perfect use case for redis. So no need to change it to prisma-store.
- Yesterday, I was looking at the error handling in graphql and found this very good article outlining different strategies: https://productionreadygraphql.com/2020-08-01-guide-to-graphql-errors If I'm not mistaken, your proposal corresponds to "Stage 2". As outlined in the article, this has a few drawbacks. Personally, I prefer "Stage 5" where you return the actual result and the possible errors as union types. This can also be combined with the idea of using a general interface for errors, as described in Stage 6a. The final result would be something like
type Mutation {
createUser(input: CreateUserInput): CreateUserResult
}
union CreateUserResult = User | UserNameTaken | PasswordTooShort
type UserNameTaken implements UserError {
message: String!
path: String!
suggestion: String!
}
interface UserError {
message: String!
path: String!
}
If the need arises and one really needs to return multiple errors, one can still go the route via error lists as in the article (but for now it feels a bit overkill). What do you think?
Moreover, in https://engineering.zalando.com/posts/2021/04/modeling-errors-in-graphql.html, they proposed to use the notion "Problem" instead of "Error", the latter then being reserved for unexpected or internal errors. That might also be a worth a thought.
{ req, res }: ContextParams, | ||
redis: Redis | ||
): Context { | ||
export function buildContext({ req, res, redis }: ContextParams): Context { |
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 it should work similar to the prisma integration:
- Register it globally with tsryinge:
JabRefOnline/api/tsyringe.config.ts
Lines 6 to 8 in 0d608ba
container.register<PrismaClient>(PrismaClient, { useFactory: instanceCachingFactory<PrismaClient>(() => new PrismaClient()), }) - Declare it as constructor dependency in services
JabRefOnline/api/user/auth.service.ts
Line 16 in 0d608ba
constructor(private prisma: PrismaClient) {} - Use it
JabRefOnline/api/user/auth.service.ts
Line 19 in 0d608ba
const user = await this.prisma.user.findUnique({
I think only the service (and not the resolvers) should know about redis since where the data is stored is somewhat of an implementation detail.
Copy over ideas list for BlueHat
This PR adds mutations for forget-password.
Implementation: The idea is to generate a token, save it in Redis, and send it in a link to the user's email. Then that link will land on the new password page. On this page, we will retrieve the token from the URL and call the change-password mutation
endpoint with the token and newly selected password. Then this mutation checks whether the token is valid or not. A token can be used only once to change the password and expires in 24 hrs.
Package:
UUID: To generate a token
Nodemailer: To send an email:
and Redis to store tokens
Ps: I will later switch the store to Prisma.