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

Hashing based Verification Token Handling #43

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

anshulg954
Copy link
Collaborator

Change Description

Try to be precise. You can additionally add comments to your PR, this might help the reviewer a lot.
It is an enhancement functionality where we ask user to input the verification token instead of verifying it with a link.

If you used new dependencies: Did you add them to requirements.txt?

Who did you ping on Mattermost to review your PR? Please ping that person again whenever you are ready for another review.
@noahho

Breaking changes

If you made any breaking changes, please update the version number.
Breaking changes are totally fine, we just need to make sure to keep the users informed and the server in sync.

Does this PR break the API? If so, what is the corresponding server commit?
It did not break anything but a few users complained that our emails landed in spam. It is an update where we try to minimize the landing of emails in spam. Corresponding server PR: https://github.com/automl/tabpfn-server/pull/76

Does this PR break the user interface? If so, why?
No, it enhances the user interface.

Please do not mark comments/conversations as resolved unless you are the assigned reviewer. This helps maintain clarity during the review process.

@anshulg954
Copy link
Collaborator Author

This also fixes the bug in reverify email. @liam-sbhoo

tabpfn_client/config.py Outdated Show resolved Hide resolved
Copy link

@tabpfn tabpfn left a comment

Choose a reason for hiding this comment

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

See the inline comments

@anshulg954 anshulg954 force-pushed the hashed_reduced_len_token branch from 29bd2de to 684d11e Compare October 30, 2024 21:39
@anshulg954
Copy link
Collaborator Author

Hi,
I have pushed the changes and added comments wherever necessary. Please also check the corresponding PR on server side.

Thanks

@anshulg954 anshulg954 requested a review from tabpfn October 30, 2024 21:53
@anshulg954 anshulg954 force-pushed the hashed_reduced_len_token branch 2 times, most recently from 621a767 to 5636876 Compare December 19, 2024 19:47
Copy link
Collaborator

@SamuelGabriel SamuelGabriel left a comment

Choose a reason for hiding this comment

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

I have a minor change proposal. The only question I have left: what do we do if a user is not verified and forgot their password? Both in the case they have the access token for their account and if they do not.

tabpfn_client/service_wrapper.py Outdated Show resolved Hide resolved
@anshulg954
Copy link
Collaborator Author

for

Hi Sam,
In both the cases user can reset the password and then re-login where reverification will take place if the user is not verified.

@SamuelGabriel
Copy link
Collaborator

ok, i see. this sounds good :)

@anshulg954 anshulg954 force-pushed the hashed_reduced_len_token branch from 5636876 to 2658e31 Compare December 20, 2024 14:51
@anshulg954 anshulg954 force-pushed the hashed_reduced_len_token branch from 2658e31 to f6ad536 Compare December 20, 2024 14:58
@SamuelGabriel SamuelGabriel merged commit 6fe87cf into main Dec 20, 2024
2 checks passed
@SamuelGabriel SamuelGabriel deleted the hashed_reduced_len_token branch December 20, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants