Skip to content
This repository was archived by the owner on Mar 17, 2025. It is now read-only.

Add requireEmailVerification argument to $requireSignIn() firebase auth router helper #887

Merged
jwngr merged 9 commits into from
Dec 21, 2016
Merged

Add requireEmailVerification argument to $requireSignIn() firebase auth router helper #887

jwngr merged 9 commits into from
Dec 21, 2016

Conversation

@ericmorgan1
Copy link
Contributor

@ericmorgan1 ericmorgan1 commented Dec 19, 2016 •
edited
Loading

Description

This adds an argument (requireEmailVerification) to the $requireSignIn router helper in the angularfire auth service.
When set to true, this will fail if the firebase auth "emailVerified" property is false.
This is useful if you do not want users to access your app until they have verified their email address.

Code sample

_routerMethodOnAuthPromise: function(rejectIfAuthDataIsNull, rejectIfEmailNotVerified) {
      var self = this;

      // wait for the initial auth state to resolve; on page load we have to request auth state
      // asynchronously so we don't want to resolve router methods or flash the wrong state
      return this._initialAuthResolver.then(function() {
        // auth state may change in the future so rather than depend on the initially resolved state
        // we also check the auth data (synchronously) if a new promise is requested, ensuring we resolve
        // to the current auth state and not a stale/initial state
        var authData = self.getAuth(), res = null;
        if (rejectIfAuthDataIsNull && authData === null) {
          res = self._q.reject("AUTH_REQUIRED");
        }
        else if (rejectIfEmailNotVerified && !authData.emailVerified) {
            res = self._q.reject("EMAIL_NOT_VERIFIED");
        }
        else {
          res = self._q.when(authData);
        }
        return res;
      });
    },

@katowulf
Copy link
Contributor

katowulf commented Dec 19, 2016

I wonder if this would be more appropriate as an argument to $requireSignIn?

Is there a case where you would require email verification but not sign in? I don't think there is.

@ericmorgan1
Copy link
Contributor Author

ericmorgan1 commented Dec 19, 2016

Good point! You're right it wouldn't make sense to need email verification but not sign in.
I updated it-- what do you think?

@ericmorgan1 ericmorgan1 changed the title Add $requireEmailVerification firebase auth router helper Add requireEmailVerification argument to $requireSignIn() firebase auth router helper Dec 19, 2016
@katowulf
Copy link
Contributor

katowulf commented Dec 19, 2016

Looks good! Could we also get this into the test units?

@ericmorgan1
Copy link
Contributor Author

ericmorgan1 commented Dec 19, 2016 •
edited
Loading

Sure! :)

Btw, are you familiar with this error?
I'm getting it when running the tests, but I can look into what's going on as well.

      √ should return true once $loaded() promise is rejected
    $ref
      √ should return Firebase instance it was created with
Chrome 56.0.2924 (Windows 8.1 0.0.0) ERROR
  Uncaught Possibly unhandled rejection: {}
  at .../angularfire/node_modules/angular-mocks/angular-mocks.js:250
Chrome 56.0.2924 (Windows 8.1 0.0.0) ERROR
  Uncaught Possibly unhandled rejection: {}
  at .../angularfire/node_modules/angular-mocks/angular-mocks.js:250

Chrome 56.0.2924 (Windows 8.1 0.0.0): Executed 50 of 274 ERROR (6.772 secs / 6.475 se
cs)

jwngr
jwngr suggested changes Dec 20, 2016
View reviewed changes
Copy link

@jwngr jwngr left a comment

Choose a reason for hiding this comment

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

This is looking solid to me. I agree we need to add some tests for it though.

Regarding the tests not working for you locally: what version of Node.js are you using to run the tests? I know Chrome recently added in error messages about uncaught promises, so that is probably what is yelling. It may be safe to ignore or maybe we need to do some more digging. I can't reproduce what you're seeing though, so knowing what version of Node.js you're on will be helpful.

src/auth/FirebaseAuth.js Outdated
res = self._q.reject("AUTH_REQUIRED");
}
else if (rejectIfEmailNotVerified && !authData.emailVerified) {
res = self._q.reject("EMAIL_NOT_VERIFIED");
Copy link

@jwngr jwngr Dec 20, 2016

Choose a reason for hiding this comment

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

Suggestion: "EMAIL_VERIFICATION_REQUIRED"

@ericmorgan1
Copy link
Contributor Author

ericmorgan1 commented Dec 20, 2016

Thanks for the help!

I added some unit tests and updated the error code as you suggested.

All the Auth tests passed, but I still couldn't get the others working, so I ignored them.

FYI:
I was using node 5.2.0, but updated to 6.9.2, but still got that error.
My Chrome version is 56.0.2924.28 beta (64-bit).
Not very sure how to debug it, but if you have any suggestions, I'll try it out!

@jwngr
Copy link

jwngr commented Dec 20, 2016

Looks great! Thanks for your contribution! I'm going to hold off on merging this in just yet since it updates documentation and I want to time it with the next release. I'll cut a new release before I head off for the holidays at the end of this week.

I'll also look into the testing issues. I was able to reproduce them with Node.js 4.4.5 / Chrome 55.0.2883. Thanks for the added setup info you shared.

@jwngr
Copy link

jwngr commented Dec 20, 2016

FYI I just fixed the unit test issue with #890.

@jwngr jwngr self-assigned this Dec 20, 2016
@ericmorgan1
Copy link
Contributor Author

ericmorgan1 commented Dec 21, 2016

Great! Thank you both for all the help and for looking at this so quickly! :)

@jwngr jwngr merged commit fffdc0c into FirebaseExtended:master Dec 21, 2016
@jwngr
Copy link

jwngr commented Dec 21, 2016

This has been released as part of AngularFire 2.2.0. Thanks again for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jwngr jwngr jwngr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jwngr jwngr

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ericmorgan1 @katowulf @jwngr