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

[Enhancement]: invert Authentication.UserCreate RPC's UserCreateRequest.verify #1129

Open
2 tasks done
kon14 opened this issue Sep 5, 2024 · 2 comments
Open
2 tasks done
Labels
enhancement New feature or request
Milestone

Comments

@kon14
Copy link
Contributor

kon14 commented Sep 5, 2024

Checklist

  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the Issue Tracker for a feature request that matches the one I want to file, without success.

Suggestion

The Authentication.UserCreate RPC accepts a UserCreateRequest message type, which includes a bool field named verify.
The current behavior of this field is highly misleading in regards to how user creation and verification are handled.

At a glance, the field's name clearly suggests that setting it to true would immediately verify the user.
However, in practice, setting verify to true does not result in verification, but instead implies that the user must be verified later.

In contrast, setting it to false doesn't merely not enforce initiating the verification process, but actually verifies the user on the spot:

user = await models.User.getInstance().create({
          email,
          hashedPassword,
          isVerified: !verify,
        });

This behavior is not intuitive and can easily lead to mistakes in user management logic.

To improve clarity and align the behavior with user expectations, I'd consider introducing the following breaking changes during the next major version bump:

  • rename the proto field to isVerified or verifyUser (so as to prevent implicitly breaking existing consumers)
  • likewise, update the RPC wrapper method in grpc-sdk
@kon14 kon14 added the enhancement New feature or request label Sep 5, 2024
@kkopanidis
Copy link
Contributor

It can be confusing sure, but a rename would be something like "requireVerification" in order to match the current behaviour. isVerified isn't the whole picture since it will continue to hide the fact that if it is false and email would be sent

@kon14
Copy link
Contributor Author

kon14 commented Sep 6, 2024

An email would only ever be sent if glogal cfg's verification is required, with email sends enabled.
That is not any singular RPC's specific behavior (if it is, it's most likely a bug).

If isVerified is true, it shouldn't initiate any verification flows no matter what, as it's assumed to be verified by somebody else.,
If it's not, it should stay in-line with the rest of the APIs, sending emails or not, depending on project/module cfgs.

@kkopanidis kkopanidis added this to v0.17 Sep 25, 2024
@kkopanidis kkopanidis moved this to Todo in v0.17 Sep 25, 2024
@kkopanidis kkopanidis added this to the v0.17 milestone Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

2 participants