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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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

@@ -44,6 +44,7 @@ def init(use_server=True):
):
print("Your email is not verified. Please verify your email to continue...")
PromptAgent.reverify_email(is_valid_token_set[1], user_auth_handler)
user_auth_handler.set_token(is_valid_token_set[1])
Copy link

Choose a reason for hiding this comment

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

Can you adapt line 38 and replace the array / tuple by two separate variables

A, B = user_auth_handler.try_reuse_existing_token() # with A, B replace by what that means

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. change accomodated, I did this initially to have minimal changes in the inner function try_reuse_existing_token.

)
if not is_created:
raise RuntimeError("User registration failed: " + str(message) + "\n")

print(
cls.indent(
"Account created successfully! To start using TabPFN please click on the link in the verification email we sent you."
"Account created successfully! To start using TabPFN please enter the secret key in the verification email we sent you."
Copy link

Choose a reason for hiding this comment

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

To start using TabPFN please enter the verification code we sent to you by mail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

cls.indent(
"Thank you for verifying your email successfully! Your access token is: "
)
+ access_token
Copy link

Choose a reason for hiding this comment

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

Don't show the access token here. Only write:

cls.indent(
"Thank you for verifying your email successfully!

Copy link

Choose a reason for hiding this comment

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

To access the token there should be a function get_token and set_token

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Access token thing is in another PR. should we do all in one instead or keep it separate. usually separate notion boards have separate PRs

if status_code == 403:
# Verify email
verified = False
while not verified:
Copy link

Choose a reason for hiding this comment

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

Somehow verification is duplicated? Why here and above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For registration, Login, and reverification. for login we need to set_token using email and password.
However, i have added a separate function now for verification functionality to avoid this - how about this?

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
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

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.

3 participants