Allow password reset request for users in realms #2980

beeman · 2016-11-23T23:55:15Z

Description

Currently the User.resetPassword method only accepts an email address as a parameter.

When you have a LoopBack application that partitions the users in Realms you need to find the user based on it's email address + realm, because it allows for 2 users with the same email address.

This PR fixes the issue by adding support for an optional realm property, that will enhance the filter used in the findOne method.

@bajtos @raymondfeng @superkhau could you please review?

Related issues

User.resetPassword for different REALM #2866

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style guide

slnode · 2016-11-23T23:55:16Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2016-11-23T23:55:17Z

Can one of the admins verify this patch?

slnode · 2016-11-23T23:55:17Z

Can one of the admins verify this patch?

slnode · 2016-11-23T23:55:17Z

Can one of the admins verify this patch?

slnode · 2016-11-23T23:55:17Z

Can one of the admins verify this patch?

davidcheung · 2016-11-25T20:04:49Z

test/user.test.js


  var validCredentialsEmail = 'foo@bar.com';
-  var validCredentials = {email: validCredentialsEmail, password: 'bar'};
+  var validCredentials = {email: validCredentialsEmail, password: 'bar', realm: 'foobar'};


@beeman would this make previous test using this variable test a different thing, perhaps we should just introduce a new variable to test this feature, then we can ensure the old no realm users work fine too

beeman · 2016-11-25T21:08:25Z

@davidcheung yep makes sense, I'll update the tests.

beeman · 2016-11-27T15:50:51Z

@davidcheung I've updated the tests as per your suggestion.

Please let me know if you have any more comments.

davidcheung · 2016-11-28T15:55:40Z

@slnode test please
@slnode ok to test

davidcheung · 2016-11-29T15:32:20Z

Thanks @beeman for the contribution! the change LGTM, can you squash the second commit?
we should also

forward port this to 3.x once merged

@loay can you also PTAL

beeman · 2016-11-29T21:17:20Z

My pleasure @davidcheung, thanks for looking at it.

Commits have been squashed into one.

Do you want me to look at getting it into 3.x? Any special instructions for doing that or is it just a matter of getting it working in master?

davidcheung · 2016-11-29T22:27:01Z

Do you want me to look at getting it into 3.x?

Thanks! that would be amazing, yeah there shouldnt any special instructions, i would like to see if @loay has any comments before we proceed though then we dont need to apply changes in 2 places.

loay · 2016-11-30T17:59:59Z

@davidcheung LGTM

beeman · 2016-11-30T19:43:33Z

Thanks @loay

@davidcheung I've checked the code in master and it looks like the code touched in this PR is similar in 2.x and 3.x.

What I could do is create separate PR targeting master after this is merged, as this PR targets 2.x.

davidcheung · 2016-11-30T20:27:43Z

@slnode test please

beeman · 2016-11-30T20:35:23Z

I'll rebase when I'm back at my machine

davidcheung · 2016-11-30T22:29:47Z

@slnode test please

bajtos

I took a look, the changes LGMT too.

davidcheung · 2016-12-01T15:48:55Z

@slnode ok to test

davidcheung · 2016-12-01T15:49:07Z

@slnode test please

beeman · 2016-12-03T20:28:44Z

I've created a PR for adding the same feature to the 3.x branch #2998.

Please let me know if there's anything more to do on either one of these PR's

davidcheung · 2016-12-05T16:27:39Z

@slnode test please

davidcheung · 2016-12-05T16:29:01Z

@beeman thanks!
there was some issue on CI that delayed merging of this, they are resolved now, lets see how they go this time

beeman · 2016-12-05T17:06:56Z

@davidcheung it looks like there are still a few tests failing, I don't have access to them though.

Any insights on what makes them fail?

davidcheung · 2016-12-05T19:37:55Z

i believe they are unrelated to your change
@pthieu can you PTAL so we can merge

superkhau · 2016-12-05T19:54:34Z

Please send a PR to downstream ignore dashboard controller/gateway-director and land that first -- then return this PR.

superkhau · 2016-12-05T19:55:12Z

Storage might be related as master has always been green on ci.strongloop.com

pthieu · 2016-12-05T19:55:49Z

Test fails look unrelated initially but if you look at http://ci.strongloop.com/job/loopback-component-storage/, it's always been green. But if you look at https://cis-jenkins.swg-devops.com/job/ds/job/loopback-component-storage~1.x/16/console, seems like a legit fail.

From what @davidcheung says, strongloop/loopback-component-storage#179 fixes this, so that will have to land before merging this PR.

davidcheung · 2016-12-05T20:14:07Z

Storage might be related as master has always been green on ci.strongloop.com

@superkhau it is passing on master here too
it is only failing on 1.x, because 1.x has always been failing https://cis-jenkins.swg-devops.com/job/ds/job/loopback-component-storage~1.x/

davidcheung · 2016-12-05T20:17:58Z

@superkhau everytime we add downstream ignore to package.json, the PR will need to be rebased, and if we know we are going to ignore that build, i dont think the order of which happens matter.

pthieu · 2016-12-05T20:24:42Z

LGTM. Confirming fails can be ignored. Looks like they've always been failing on those specific branches. Approved for merging.

superkhau · 2016-12-06T08:04:30Z

everytime we add downstream ignore to package.json, the PR will need to be rebased, and if we know we are going to ignore that build, i dont think the order of which happens matter.

@davidcheung This is really depends on the repo downstream (and whether it blocks your merge/needs override). In this case it doesn't, so the merge order doesn't matter. 👍

crandmck added the #community contribution label Nov 23, 2016

davidcheung reviewed Nov 25, 2016

View reviewed changes

davidcheung self-assigned this Nov 28, 2016

beeman force-pushed the bb/password-reset-realms branch from e973c6d to bc4b5c0 Compare November 29, 2016 20:44

Allow password reset request for users in realms

e7831f6

beeman force-pushed the bb/password-reset-realms branch from bc4b5c0 to e7831f6 Compare November 30, 2016 21:58

bajtos approved these changes Dec 1, 2016

View reviewed changes

beeman mentioned this pull request Dec 3, 2016

Allow password reset request for users in realms (v3.x) #2998

Merged

2 tasks

davidcheung merged commit 1fa785f into strongloop:2.x Dec 5, 2016

davidcheung removed the #community contribution label Dec 5, 2016

davidcheung mentioned this pull request Dec 5, 2016

Opt-out downstream builds that are unstable #3000

Merged

Allow password reset request for users in realms #2980

Allow password reset request for users in realms #2980

Uh oh!

Conversation

beeman commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

slnode commented Nov 23, 2016

Uh oh!

davidcheung Nov 25, 2016

Choose a reason for hiding this comment

Uh oh!

beeman commented Nov 25, 2016

Uh oh!

beeman commented Nov 27, 2016

Uh oh!

davidcheung commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidcheung commented Nov 29, 2016

Uh oh!

beeman commented Nov 29, 2016

Uh oh!

davidcheung commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loay commented Nov 30, 2016

Uh oh!

beeman commented Nov 30, 2016

Uh oh!

davidcheung commented Nov 30, 2016

Uh oh!

beeman commented Nov 30, 2016

Uh oh!

davidcheung commented Nov 30, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

davidcheung commented Dec 1, 2016

Uh oh!

davidcheung commented Dec 1, 2016

Uh oh!

beeman commented Dec 3, 2016

Uh oh!

davidcheung commented Dec 5, 2016

Uh oh!

davidcheung commented Dec 5, 2016

Uh oh!

beeman commented Dec 5, 2016

Uh oh!

davidcheung commented Dec 5, 2016

Uh oh!

superkhau commented Dec 5, 2016

Uh oh!

superkhau commented Dec 5, 2016

Uh oh!

pthieu commented Dec 5, 2016

Uh oh!

davidcheung commented Dec 5, 2016

Uh oh!

davidcheung commented Dec 5, 2016

Uh oh!

pthieu commented Dec 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

superkhau commented Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beeman commented Nov 23, 2016 •

edited

Loading

davidcheung commented Nov 28, 2016 •

edited

Loading

davidcheung commented Nov 29, 2016 •

edited

Loading

pthieu commented Dec 5, 2016 •

edited

Loading

superkhau commented Dec 6, 2016 •

edited

Loading