Skip to content

Commit

Permalink
Merge pull request #53 from fronn/patch-1
Browse files Browse the repository at this point in the history 
Fix for issue #52
  • Loading branch information
@anantn
anantn committed Jul 11, 2013
2 parents 2768e6a + d825540 commit 82e5d17
Showing 1 changed file with 18 additions and 12 deletions.
30 changes: 18 additions & 12 deletions angularFire.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ angular.module("firebase").factory("angularFireCollection", ["$timeout", functio

// Authentication support for AngularFire.
angular.module("firebase").factory("angularFireAuth", [
"$rootScope", "$parse", "$timeout", "$location",
function($rootScope, $parse, $timeout, $location) {
"$rootScope", "$parse", "$timeout", "$location", "$route",
function($rootScope, $parse, $timeout, $location, $route) {

function deconstructJWT(token) {
var segments = token.split(".");
Expand All @@ -261,7 +261,19 @@ angular.module("firebase").factory("angularFireAuth", [
});
}
}


function authRequiredRedirect(route, path, self) {
if(route.authRequired && !self._authenticated){
if(route.pathTo === undefined) {
self._redirectTo = $location.path();
} else {
self._redirectTo = route.pathTo === path ? "/" : route.pathTo;
}
$location.replace();
$location.path(path);
}
}

return {
initialize: function(url, options) {
var self = this;
Expand All @@ -282,16 +294,10 @@ angular.module("firebase").factory("angularFireAuth", [
this._redirectTo = null;
this._authenticated = false;
if (options.path) {
authRequiredRedirect($route.current, options.path, self);

$rootScope.$on("$routeChangeStart", function(e, next, current) {
if (next.authRequired && !self._authenticated) {
if(next.pathTo === undefined) {
self._redirectTo = $location.path();
} else {
self._redirectTo = next.pathTo === options.path ? "/" : next.pathTo;
}
$location.replace();
$location.path(options.path);
}
authRequiredRedirect(next, options.path, self);
});
}

Expand Down

3 comments on commit 82e5d17

@deedubbleyoo
Copy link

Choose a reason for hiding this comment

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

This change caused my app to not be able to initialize successfully, as it could not resolve route.authRequired - it seems it is not being passed $route.current correctly, why could this be the case?

@katowulf
Copy link
Contributor

Choose a reason for hiding this comment

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

current is null for me occasionally. I think it's just on the initial calls, where there actually is no existing route. Could you be running into this use case?

@deedubbleyoo
Copy link

Choose a reason for hiding this comment

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

Having debugged the code, I believe so - I was able to get my tests (and the live page) to pass by changing the code to:

if (options.path) {
            if (typeof $route.current === 'undefined'){
                authRequiredRedirect(options.path, options.path, self);
            } else {
                authRequiredRedirect($route.current, options.path, self);
            }
          $rootScope.$on("$routeChangeStart", function(e, next) {
            authRequiredRedirect(next, options.path, self);
          });
        }

I have made the assumption that options.path is the same route as the initial load. I am wondering if there is a more elegant solution so will be opening a new issue to discuss.

Please sign in to comment.