Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

fix(users) Stops error on signin/signup #1495

Merged
merged 2 commits into from
Sep 11, 2016
Merged

fix(users) Stops error on signin/signup #1495

merged 2 commits into from
Sep 11, 2016

Conversation

Wuntenn
Copy link
Contributor

@Wuntenn Wuntenn commented Sep 9, 2016 •
edited
Loading

Resolves issue relating to info object being used as the redirect path.

  • Improves use of the passport info object allowing slight simplification of login; and
  • removes the need to temporarily cache the redirect within the session.

Fixes #1290, Fixes #1284

fix(authentication) Stops error on signin/signup
c6ae53c 
Uses the passport info object to simplify login and remove the need to
temporarily cache the redirect within the session.
@coveralls
Copy link

coveralls commented Sep 9, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 73.058% when pulling 00cdf823dae824cd2b459d438a1209c59b2d0501 on Wuntenn:fixPassportRedirect into 17772fe on meanjs:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 73.058% when pulling 00cdf823dae824cd2b459d438a1209c59b2d0501 on Wuntenn:fixPassportRedirect into 17772fe on meanjs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 73.058% when pulling 00cdf823dae824cd2b459d438a1209c59b2d0501 on Wuntenn:fixPassportRedirect into 17772fe on meanjs:master.

@coveralls
Copy link

coveralls commented Sep 9, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 73.058% when pulling fb8e902 on Wuntenn:fixPassportRedirect into 17772fe on meanjs:master.

@mleanos
Copy link
Member

mleanos commented Sep 10, 2016

Restarted the build, since it looked like it was just a timeout issue with the last run.

@mleanos
Copy link
Member

mleanos commented Sep 10, 2016

LGTM. I've pulled this down, and tested. I can verify this indeed fixes the issues of the redirects.

However (there's always a "however"), I ran into an issue that's unrelated to the aim of this PR..

With this PR, we introduced a fix to enabled the sparse index option. This handles the case where a provider doesn't send back an email when our User is registering as a new user. The issue I just ran into is that here, the email field is set to an empty string if providerUserProfile.email is undefined, since we have a default value of the email field here. This will, in effect, negate our use of the sparse option.

In my PR that introduced the sparse option, I wrote a test to verify the sparse was working correctly, but failed to realize the impact of the default value of email. Doh!

@Wuntenn I think we can fix this issue in this PR. What say you?

My two suggested fixes are:

  1. Remove this line & add user.email = providerUserProfile.email after we instantiate the new User object. This may look like a band-aid or hack-ish fix but we should only run into this issue when creating a new User during this Provider authentication process.
    OR
  2. Remove our default value of the Email field. I can't think of any reason why we have this default value, and I don't see any negative impact of doing so.

Option #2 may have more "business rule" impact for our users (i.e. more controversial to our users) , so I'm sort of leaning more toward option #1.

@lirantal lirantal added this to the 0.5.0 milestone Sep 10, 2016
@mleanos mleanos merged commit 67d1a5a into meanjs:master Sep 11, 2016
hyperreality pushed a commit to hyperreality/mean that referenced this pull request Sep 17, 2016
@Wuntenn @hyperreality
fix(authentication) Stops error on signin/signup (meanjs#1495)
b77508f 
Uses the passport info object to simplify login and remove the need to
temporarily cache the redirect within the session.
Wuntenn added a commit to Wuntenn/mean that referenced this pull request Sep 20, 2016
@Wuntenn
fix(authentication) Stops error on signin/signup (meanjs#1495)
d7d6828 
Uses the passport info object to simplify login and remove the need to
temporarily cache the redirect within the session.
mleanos pushed a commit that referenced this pull request Oct 5, 2016
@hyperreality @mleanos
feat(core): Minor UI fixes for mobile; autofocus (#1499)
73a7c2c 
* Added configuration for owasp. Synchronize client owap configs with the server configs.
Also added a time indicator on failed login attempts to give the user feedback on subsequent failed login attempts.

* switched to handlebar template for passing the server's owasp config down to the client.

reverted some of the other changes (regarding the http request).

* Removed debug code.

* Changed variable name to owaspConfig

* Fixed minor type-o's and set owasp.config() rather than the underlying configs.

* chore(tidy): tidying up minor lint and layout issues

* fix(lint): CSS alphabetize warnings (#1498)

Fixes css lintings warnings of properties not alphabetized.

* fix(authentication) Stops error on signin/signup (#1495)

Uses the passport info object to simplify login and remove the need to
temporarily cache the redirect within the session.

* Moved owasp config into default and reverted other config files.

Modified config to be "shared". This will allow future configurations to be easily passed to the client.

* fixed 403 redirect if not signed in (#1496)

* Update form-article.client.view.html

For New Article, delete function no required

* UI changes for mobile; autofocus

* fixed broken password popover balloon

* add e2e test for autofocus

* Remove test, fix delete social login button

* feat(core): Move template to .github folder

* Deprecated $http success/error promise methods (#1508)

Replaces the $http service calls with promise based methods
of the client-side UsersService for the following:
  Users Change Password
  Users Manage Social Accounts
  Users Password Forgot
  Users Password Reset
  Users Signup
  Users Signin

Modifies tests to reflect changes.

Closes #1479

* rebase
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@mleanos mleanos

Labels
platform:express
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Wuntenn @coveralls @mleanos @lirantal