Skip to content

feat: allow master key to bypass email verification #8231

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

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

Conversation

mstniy
Copy link
Contributor

@mstniy mstniy commented Oct 13, 2022

New Pull Request Checklist

Issue Description

This PR allows the master key to bypass email verification.

Related issue: #8230

Approach

Modified RestWrite to avoid triggering the verification sequence and create a session token if the request was initiated by the master key and the request sets emailVerified to true.

TODOs before merging

  • Add tests
  • Add changes to documentation (guides, repository pages, in-code descriptions)
  • Add security check
  • Add new Parse Error codes to Parse JS SDK
  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 13, 2022

Thanks for opening this pull request!

  • 🎉 We are excited about your hands-on contribution!

@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Base: 94.11% // Head: 93.96% // Decreases project coverage by -0.15% ⚠️

Coverage data is based on head (10fd88a) compared to base (8863ad2).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 10fd88a differs from pull request most recent head 34dcd5d. Consider uploading reports for the commit 34dcd5d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8231      +/-   ##
==========================================
- Coverage   94.11%   93.96%   -0.16%     
==========================================
  Files         182      182              
  Lines       13770    13754      -16     
==========================================
- Hits        12960    12924      -36     
- Misses        810      830      +20     
Impacted Files Coverage Δ
src/RestWrite.js 94.65% <100.00%> (-0.14%) ⬇️
src/Adapters/Files/GridFSBucketAdapter.js 80.32% <0.00%> (-13.11%) ⬇️
src/Routers/FilesRouter.js 88.97% <0.00%> (-3.12%) ⬇️
src/Controllers/FilesController.js 94.00% <0.00%> (-2.00%) ⬇️
src/Controllers/DatabaseController.js 93.76% <0.00%> (-0.15%) ⬇️
src/Routers/ClassesRouter.js 97.95% <0.00%> (+1.02%) ⬆️
src/batch.js 94.73% <0.00%> (+1.75%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza mtrezza changed the title feat: allow the master key to bypass email verification (#8230) feat: allow the master key to bypass email verification Oct 14, 2022
@mtrezza mtrezza changed the title feat: allow the master key to bypass email verification feat: allow master key to bypass email verification Oct 14, 2022
@mtrezza
Copy link
Member

mtrezza commented Oct 14, 2022

Kindly comment when this is ready for review

@mstniy mstniy force-pushed the issue_8230 branch 2 times, most recently from 588b0f3 to 630d464 Compare October 14, 2022 12:10
@mstniy
Copy link
Contributor Author

mstniy commented Oct 14, 2022

Ready for review @mtrezza

@mtrezza mtrezza requested a review from a team October 15, 2022 22:32
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Could you please bring this up-to-date with the base branch?

Note for reviewers: this feature approach should be reviewed in context of #7144

@mstniy
Copy link
Contributor Author

mstniy commented Oct 16, 2022

@mtrezza Done
I hadn't noticed the previous issue and the discussion around it. The PR is basically the changes we did to our fork. Let me know if you consider the approach inferior to what has been suggested already.

@mtrezza
Copy link
Member

mtrezza commented Oct 16, 2022

How do you see #7144 in relation to your use case? Would that fully / partially / not at all address your requirements?

@mstniy
Copy link
Contributor Author

mstniy commented Oct 17, 2022

@mtrezza There are multiple suggestions at #7144 .
Our approach is pretty similar to what is suggested by @Samigos.
Conditioning email verification on user roles is not really applicable, as the users do not belong to any group when they first sign up.
The approach about extending the emailVerified field does not sound backwards-compatible with the existing server deployments.

Let me know what you think.

@dblythy
Copy link
Member

dblythy commented Oct 17, 2022

Thanks for this PR!

Perhaps this would be more along the lines of #8218, a maintenanceKey for the dashboard would allow override of emailVerified, tokens, etc, rather than defining a single use case for one of these restricted keys

@mtrezza
Copy link
Member

mtrezza commented Oct 17, 2022

That would be possible as a temp. workaround, but:

give maintenanceKey to a DevOp to be able to manipulate internal Parse Server fields --> these internal fields are not supposed to be manipulated for normal operation, they may need to to be manipulated for one-time migration or data correction tasks

The maintenanceKey would be even more powerful than the master key (and may become even more so in the future), so we would strongly advice against using it in routine operation.

I think email verification needs some refinements. It's either on or off for all, not very practical and probably forces some developers to not use it even though it's an important spam block feature.

Let's discuss in #7144 how a solution could look like. If we can find one that incorporates #7144 and #8230, we'd close 2 issues at once. And it could be a solution where the master key isn't necessary.

@mtrezza mtrezza linked an issue Oct 17, 2022 that may be closed by this pull request
3 tasks
@mstniy mstniy mentioned this pull request Oct 17, 2022
3 tasks
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.

Allow the master key to bypass email verification
3 participants