Skip to content
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

Verify Email middleware #13

Merged
merged 12 commits into from
Aug 12, 2024
Merged

Verify Email middleware #13

merged 12 commits into from
Aug 12, 2024

Conversation

PizieDust
Copy link
Collaborator

@PizieDust PizieDust commented Jul 26, 2024

This PR adds a new middleware for checking if a user's email has been verified.
There's also addition of smaller utility function such as checking if the cookie in the header is valid and hasn't expired.

@PizieDust PizieDust added the enhancement New feature or request label Jul 26, 2024
@PizieDust PizieDust self-assigned this Jul 26, 2024
middleware.ml Outdated Show resolved Hide resolved
middleware.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Contributor

hannesm commented Jul 27, 2024

Looks good on general, minir nits above.

From the bird perspective, I lack some understanding why cookie mahic is done in middleware and also in the unikernel. Maybe we need to draw some diagrams on what a http request should have for validation? I'm still a big fan of e.g. webmachine diagram https://raw.githubusercontent.com/wiki/webmachine/webmachine/images/http-headers-status-v3.png

(also there's http://github.com/inhabitedtype/ocaml-webmachine/ but rather dated an stuck with cohttp) -- so let's have a simple version thereof for mollymawk :)

for functions with only a single argument, I'd not use labels. Usually I use them if I pass two arguments of the same type, or if I pass boolean values.

@PizieDust
Copy link
Collaborator Author

Looks good on general, minir nits above.

From the bird perspective, I lack some understanding why cookie mahic is done in middleware and also in the unikernel. Maybe we need to draw some diagrams on what a http request should have for validation? I'm still a big fan of e.g. webmachine diagram https://raw.githubusercontent.com/wiki/webmachine/webmachine/images/http-headers-status-v3.png

(also there's http://github.com/inhabitedtype/ocaml-webmachine/ but rather dated an stuck with cohttp) -- so let's have a simple version thereof for mollymawk :)

for functions with only a single argument, I'd not use labels. Usually I use them if I pass two arguments of the same type, or if I pass boolean values.

Thank you for the review and this diagram. It is really helpful. I will at some point create a dedicated error page that we can call to handle all the different types of errors, and also be sure to include the appropriate status codes.

@PizieDust PizieDust requested a review from hannesm July 29, 2024 07:08
user_model.ml Outdated Show resolved Hide resolved
user_model.ml Outdated Show resolved Hide resolved
user_model.ml Outdated Show resolved Hide resolved
user_model.ml Outdated Show resolved Hide resolved
utils.ml Outdated Show resolved Hide resolved
];
];
Footer_layout.footer;
Tyxml.Html.Unsafe.data "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand, why do we have a Unsafe.data "" here? What is its purpose?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be corrected with a function to call our backend when the user wants to resend a verification email, in case the one they received already expired. :)

utils.ml Outdated
Sendmail.mechanism = PLAIN;
username = config_data.username;
password = config_data.password;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be part of the configuration -- either passed via command line arguments or stored in the persistent storage...

utils.ml Outdated
let generate_verification_link uuid timestamp =
let signature = generate_signature uuid timestamp in
let encoded_uuid = Base64.encode_string uuid in
"/auth/verify/token=" ^ encoded_uuid ^ "/" ^ signature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue with this is: consider we know the user uuid (passed in here) -- and I'm at the moment not sure whether it is exposed (accidentally or as part of an API), we can do the entire token computation.

this means that a person who want to verify an email address they don't have access to, can do so - without receiving the email.

store email verification uuid in user model, use a random uuid
@hannesm hannesm merged commit 2cd04e7 into main Aug 12, 2024
0 of 2 checks passed
@hannesm hannesm deleted the valid_cookies branch August 12, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants