Improve email verification #3681

aontas · 2017-03-31T03:58:00Z

This should fix the issues as outlined in #3393 and #3432

facebook-github-bot · 2017-04-04T09:07:36Z

@aontas updated the pull request - view changes

acinader

this looks good to me and is a nice catch. Would like @flovilmart to take a look at too.

flovilmart · 2017-04-05T20:19:07Z

@acinader that's a hot fix, but we need something for those protected fields, declared at the class level. Also we strip authData and password from the responses, I believe this should be stripped at the same spot no?

aontas · 2017-04-12T04:42:39Z

@flovilmart So you would prefer the logic I added in UsersRouter to instead live in the cleanResultOfSensitiveUserInfo function in RestQuery.js?

And move the other logic (disallowing emailVerified update) to the UserController?

When I put these changes in, would that be enough to satisfy your comments? If not can you provide some more direction as to what you would consider appropriate?

flovilmart · 2017-04-12T18:50:48Z

@aontas I believe we should keep it in the responses, unless that causes an issue... (http://docs.parseplatform.org/rest/guide/#verifying-emails), only for the current user.

perhaps we should validate that the update don't contain the emailVerified here: https://github.com/parse-community/parse-server/blob/master/src/RestWrite.js#L346

aontas · 2017-04-13T01:09:45Z

@flovilmart There is nothing in this PR that removes the emailVerified flag from the responses. I agree that it is required to stay there. The only things that are removed are internal data members, which have been denoted by a leading underscore (ie, "_email_verify_token" which is the secret provided via email).

I will move the logic to check the incoming emailVerified data in RestWrite.

codecov · 2017-04-13T01:18:59Z

Codecov Report

Merging #3681 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3681      +/-   ##
=========================================
+ Coverage   90.22%   90.3%   +0.07%     
=========================================
  Files         114     114              
  Lines        7491    7498       +7     
=========================================
+ Hits         6759    6771      +12     
+ Misses        732     727       -5

Impacted Files	Coverage Δ
src/Routers/UsersRouter.js	`88.88% <100%> (+1.86%)`	⬆️
src/RestWrite.js	`94.54% <100%> (+0.23%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`95.44% <0%> (+0.13%)`	⬆️
src/Controllers/SchemaController.js	`97.29% <0%> (+0.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fbe354...c50b34a. Read the comment docs.

flovilmart

One small fix, otherwise we'll miss out some test :)

flovilmart · 2017-04-13T12:36:58Z

spec/ParseUser.spec.js

@@ -26,7 +26,7 @@ function verifyACL(user) {
  expect(perms['*'].write).not.toBe(true);
 }

-describe('Parse.User testing', () => {
+fdescribe('Parse.User testing', () => {


aontas · 2017-04-13T13:17:05Z

Oops. Yeah. Fixed now.

flovilmart

@aontas still a few pending questions here and there. Thanks for the work you're putting onto it!

flovilmart · 2017-04-23T21:56:54Z

spec/ParseUser.spec.js

+      verifyUserEmails: true,
+      emailAdapter: emailAdapter,
+      publicServerURL: "http://localhost:8378/1"
+    })


reconfigureServer returns a promise, can you handle that please?

const user = new Parse.User(); user.set({ username: 'hello', password: 'world' }); reconfigureServer({ appName: 'unused', verifyUserEmails: true, emailAdapter: emailAdapter, publicServerURL: "http://localhost:8378/1" }).then(() => { return user.signUp() }) ...

flovilmart · 2017-04-23T21:57:06Z

spec/ParseUser.spec.js

+      sendMail: () => Promise.resolve()
+    }
+
+    reconfigureServer({


same as above

flovilmart · 2017-04-23T21:58:22Z

src/Routers/UsersRouter.js

@@ -57,6 +57,17 @@ export class UsersRouter extends ClassesRouter {
          const user = response.results[0].user;
          // Send token back on the login, because SDKs expect that.
          user.sessionToken = sessionToken;
+
+          // Remove hidden properties.
+          for (var key in user) {


just a quick question,
Those fields get stripped only when calling /me, but not when let's say we make an unauthenticated query?

Could we add a test for that?

What sort of unauthenticated query would we want these fields to be available?

That's the point, we won't. But those fields would only be stripped here, not everywhere we need them to be stripped

There does seem to be handling somewhere that removes hidden fields (see updated test where users/ is retrieved without hidden field).

The general case where all hidden fields are removed from all unauthenticated queries is likely better to be talked in its own issue, rather than tacked on to this PR.

Does this current version require any further changes?

aontas · 2017-04-23T22:10:38Z

Ok. I'll have a look in the next few days.

…

On Mon., 24 Apr. 2017, 9:59 am Florent Vilmart, ***@***.***> wrote: ***@***.**** requested changes on this pull request. @aontas <https://github.com/aontas> still a few pending questions here and there. Thanks for the work you're putting onto it! ------------------------------ In spec/ParseUser.spec.js <#3681 (comment)> : > @@ -2784,4 +2784,74 @@ describe('Parse.User testing', () => { done(); }); }); + + it('should not allow updates to emailVerified', done => { + var emailAdapter = { + sendVerificationEmail: () => {}, + sendPasswordResetEmail: () => Promise.resolve(), + sendMail: () => Promise.resolve() + } + + reconfigureServer({ + appName: 'unused', + verifyUserEmails: true, + emailAdapter: emailAdapter, + publicServerURL: "http://localhost:8378/1" + }) reconfigureServer returns a promise, can you handle that please? const user = new Parse.User(); user.set({ username: 'hello', password: 'world' }); reconfigureServer({ appName: 'unused', verifyUserEmails: true, emailAdapter: emailAdapter, publicServerURL: "http://localhost:8378/1" }).then(() => { return user.signUp() }) ... ------------------------------ In spec/ParseUser.spec.js <#3681 (comment)> : > + done(); + }).catch((err) => { + expect(err.message).toBe("Clients aren't allowed to manually update email verification."); + done(); + }); + }); + + it('should not allow updates to hidden fields', done => { + + var emailAdapter = { + sendVerificationEmail: () => {}, + sendPasswordResetEmail: () => Promise.resolve(), + sendMail: () => Promise.resolve() + } + + reconfigureServer({ same as above ------------------------------ In src/Routers/UsersRouter.js <#3681 (comment)> : > @@ -57,6 +57,17 @@ export class UsersRouter extends ClassesRouter { const user = response.results[0].user; // Send token back on the login, because SDKs expect that. user.sessionToken = sessionToken; + + // Remove hidden properties. + for (var key in user) { just a quick question, Those fields get stripped only when calling /me, but not when let's say we make an unauthenticated query? Could we add a test for that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3681 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEtyzHRTsvcYLFdBWmkcDI3XjEMIX_cvks5ry8mvgaJpZM4MvMky> .

aontas · 2017-05-01T02:38:40Z

PR #3768 seems to have caused this build to fail. It was merged even though the build failed: https://travis-ci.org/parse-community/parse-server/builds/227340428

flovilmart · 2017-05-11T13:44:52Z

Let's merge that one for the upcoming release, Thanks @aontas !

facebook-github-bot added the GH Review: review-needed label Mar 31, 2017

acinader approved these changes Apr 5, 2017

View reviewed changes

acinader requested a review from flovilmart April 5, 2017 20:17

aontas added 2 commits April 13, 2017 10:52

Removed hidden keys from users/me.

862b1e1

Ensured that general users cannot update email verified flag.

140272e

flovilmart suggested changes Apr 13, 2017

View reviewed changes

flovilmart suggested changes Apr 23, 2017

View reviewed changes

craigpatten mentioned this pull request Apr 27, 2017

Add Buddy to the provider list #2996

Closed

Updated tests to reflect email verification changes.

c50b34a

flovilmart merged commit 877ef78 into parse-community:master May 11, 2017

This was referenced Jul 24, 2017

Vulnerability: Email verification can be circumvented by reading _email_verify_token #3432

Closed

Vulnerability: Email verification can be circumvented #3393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve email verification #3681

Improve email verification #3681

aontas commented Mar 31, 2017

facebook-github-bot commented Apr 4, 2017

acinader left a comment

flovilmart commented Apr 5, 2017

aontas commented Apr 12, 2017

flovilmart commented Apr 12, 2017

aontas commented Apr 13, 2017

codecov bot commented Apr 13, 2017 •

edited

Loading

flovilmart left a comment

flovilmart Apr 13, 2017

aontas commented Apr 13, 2017

flovilmart left a comment

flovilmart Apr 23, 2017

flovilmart Apr 23, 2017

flovilmart Apr 23, 2017

aontas Apr 28, 2017

flovilmart Apr 28, 2017

aontas May 1, 2017

aontas commented Apr 23, 2017 via email

aontas commented May 1, 2017

flovilmart commented May 11, 2017

Improve email verification #3681

Improve email verification #3681

Conversation

aontas commented Mar 31, 2017

facebook-github-bot commented Apr 4, 2017

acinader left a comment

Choose a reason for hiding this comment

flovilmart commented Apr 5, 2017

aontas commented Apr 12, 2017

flovilmart commented Apr 12, 2017

aontas commented Apr 13, 2017

codecov bot commented Apr 13, 2017 • edited Loading

Codecov Report

flovilmart left a comment

Choose a reason for hiding this comment

flovilmart Apr 13, 2017

Choose a reason for hiding this comment

aontas commented Apr 13, 2017

flovilmart left a comment

Choose a reason for hiding this comment

flovilmart Apr 23, 2017

Choose a reason for hiding this comment

flovilmart Apr 23, 2017

Choose a reason for hiding this comment

flovilmart Apr 23, 2017

Choose a reason for hiding this comment

aontas Apr 28, 2017

Choose a reason for hiding this comment

flovilmart Apr 28, 2017

Choose a reason for hiding this comment

aontas May 1, 2017

Choose a reason for hiding this comment

aontas commented Apr 23, 2017 via email

aontas commented May 1, 2017

flovilmart commented May 11, 2017

codecov bot commented Apr 13, 2017 •

edited

Loading