Skip to content

Commit

Permalink
fix(auth): check protocol before calling getRedirectResult (#271)
Browse files Browse the repository at this point in the history 
When using AngularFire in non-browser environments, such as Ionic,
subscribing to the AngularFireAuth service would cause an error to
be thrown. This was because getRedirectResult() is always called
at the beginning of the auth observable, to determine if the page
is being loaded after a redirect-based oAuth flow. However, calling
this method is not supported if location.protocol is not http or https.
In the case of Ionic, the protocol is file:.
This change adds a check before calling getRedirectResult to make sure
the protocol is http or https.

BREAKING CHANGE:

The AngularFireAuth class has changed the order of its constructor arguments.
Since this is usually instantiated automatically via dependency injection,
it shouldn't affect common usage of the library. However, if manually
instantiating AngularFireAuth in tests or in an application, the order of
arguments is now: `(AuthBackend, WindowLocation[, AuthConfiguration])`.

Fixes #243
  • Loading branch information
@jeffbcross @davideast
jeffbcross authored and davideast committed Jun 22, 2016
1 parent 5ab37bf commit f38e9d7
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 37 deletions.
11 changes: 8 additions & 3 deletions src/angularfire2.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
FirebaseObjectFactory
} from './utils/firebase_object_factory';
import * as utils from './utils/utils';
import {FirebaseConfig, FirebaseApp} from './tokens';
import { FirebaseConfig, FirebaseApp, WindowLocation } from './tokens';
import { FirebaseAppConfig } from './interfaces';
import {
AuthBackend,
Expand Down Expand Up @@ -55,7 +55,11 @@ export const FIREBASE_PROVIDERS:any[] = [
provide: AuthBackend,
useFactory: _getAuthBackend,
deps: [FirebaseApp]
}
},
{
provide: WindowLocation,
useValue: window.location
},
];

function _getAuthBackend(app: firebase.app.App): FirebaseSdkAuthBackend {
Expand Down Expand Up @@ -86,7 +90,8 @@ export {
firebaseAuthConfig,
FirebaseAuthState,
AuthMethods,
AuthProviders
AuthProviders,
WindowLocation
}

export { FirebaseConfig, FirebaseApp, FirebaseAuthConfig, FirebaseRef, FirebaseUrl } from './tokens';
Expand Down
78 changes: 54 additions & 24 deletions src/providers/auth.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import {
AngularFireAuth,
AuthMethods,
firebaseAuthConfig,
AuthProviders
AuthProviders,
WindowLocation
} from '../angularfire2';
import { COMMON_CONFIG } from '../test-config';

Expand Down Expand Up @@ -99,20 +100,37 @@ describe('FirebaseAuth', () => {
let backend: AuthBackend;
let afAuth: AngularFireAuth;
let authSpy: jasmine.Spy;
var fbAuthObserver: Observer<firebase.User>;

beforeEachProviders(() => [
FIREBASE_PROVIDERS,
defaultFirebase(COMMON_CONFIG),
provide(FirebaseApp, {
useFactory: (config: FirebaseAppConfig) => {
var app = initializeApp(config);
(<any>app).auth = () => authSpy;
return app;
},
deps: [FirebaseConfig]
})
]);
let fbAuthObserver: Observer<firebase.User>;
let windowLocation: any;

beforeEachProviders(() => {
windowLocation = {
hash: '',
search: '',
pathname:'/',
port: '',
hostname:'localhost',
host:'localhost',
protocol:'https:',
origin:'localhost',
href:'https://localhost/'
};
return [
FIREBASE_PROVIDERS,
defaultFirebase(COMMON_CONFIG),
provide(FirebaseApp, {
useFactory: (config: FirebaseAppConfig) => {
var app = initializeApp(config);
(<any>app).auth = () => authSpy;
return app;
},
deps: [FirebaseConfig]
}),
provide(WindowLocation, {
useValue: windowLocation
})
]
});

beforeEach(() => {
authSpy = jasmine.createSpyObj('auth', authMethods);
Expand Down Expand Up @@ -202,7 +220,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.Anonymous
};
afAuth = new AngularFireAuth(backend, config);
afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login();
expect(app.auth().signInAnonymously).toHaveBeenCalled();
});
Expand All @@ -211,7 +229,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.Anonymous
};
afAuth = new AngularFireAuth(backend, config);
afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login({
method: AuthMethods.Popup,
provider: AuthProviders.Google
Expand All @@ -227,7 +245,7 @@ describe('FirebaseAuth', () => {
provider: AuthProviders.Google,
scope: ['email']
};
afAuth = new AngularFireAuth(backend, config);
afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login({
provider: AuthProviders.Github
});
Expand All @@ -252,7 +270,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.Password
};
let afAuth = new AngularFireAuth(backend, config);
let afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login()
.then(done.fail, done);
});
Expand All @@ -261,7 +279,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.CustomToken
};
let afAuth = new AngularFireAuth(backend, config);
let afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login()
.then(done.fail, done);
});
Expand All @@ -270,7 +288,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.OAuthToken
};
let afAuth = new AngularFireAuth(backend, config);
let afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login()
.then(done.fail, done);
});
Expand All @@ -279,7 +297,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.Popup
};
let afAuth = new AngularFireAuth(backend, config);
let afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login()
.then(done.fail, done);
});
Expand All @@ -288,7 +306,7 @@ describe('FirebaseAuth', () => {
let config = {
method: AuthMethods.Redirect
};
let afAuth = new AngularFireAuth(backend, config);
let afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login()
.then(done.fail, done);
});
Expand Down Expand Up @@ -359,7 +377,7 @@ describe('FirebaseAuth', () => {
email: 'david@fire.com',
password: 'supersecretpassword'
};
let afAuth = new AngularFireAuth(backend, config);
let afAuth = new AngularFireAuth(backend, windowLocation, config);
afAuth.login(credentials);
expect(app.auth().signInWithEmailAndPassword).toHaveBeenCalledWith(credentials.email, credentials.password);
});
Expand Down Expand Up @@ -433,6 +451,18 @@ describe('FirebaseAuth', () => {
fbAuthObserver.next(firebaseUser);
});
}, 10);


it('should not call getRedirectResult() if location.protocol is not http or https', (done) => {
windowLocation.protocol = 'file:';
afAuth
.take(1)
.do(() => {
expect(authSpy['getRedirectResult']).not.toHaveBeenCalled();
})
.subscribe(done, done.fail);
fbAuthObserver.next(firebaseUser);
});
});

describe('authWithOAuthRedirect', () => {
Expand Down
24 changes: 14 additions & 10 deletions src/providers/auth.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import 'rxjs/add/operator/concat';
import 'rxjs/add/operator/skip';
import 'rxjs/add/observable/of';

import {FirebaseApp, FirebaseAuthConfig} from '../tokens';
import { FirebaseApp, FirebaseAuthConfig, WindowLocation } from '../tokens';
import {isPresent} from '../utils/utils';
import * as utils from '../utils/utils';
import {
Expand All @@ -36,23 +36,27 @@ export const firebaseAuthConfig = (config: AuthConfiguration): Provider => {
export class AngularFireAuth extends ReplaySubject<FirebaseAuthState> {
private _credentialCache: {[key:string]: OAuthCredential} = {};
constructor(private _authBackend: AuthBackend,
@Inject(WindowLocation) loc: Location,
@Optional() @Inject(FirebaseAuthConfig) private _config?: AuthConfiguration) {
super(kBufferSize);

let firstPass = true;
this._authBackend.onAuth()
.mergeMap((authState: FirebaseAuthState) => {
// TODO: get rid of side effect
if (firstPass) {
firstPass = false;
return this._authBackend.getRedirectResult()
.map((userCredential: firebase.auth.UserCredential) => {
if (userCredential && userCredential.credential) {
authState = attachCredentialToAuthState(authState, userCredential.credential, userCredential.credential.provider);
this._credentialCache[userCredential.credential.provider] = <OAuthCredential>userCredential.credential;
}
return authState;
})
if(['http:', 'https:'].indexOf(loc.protocol) > -1) {
// Only call getRedirectResult() in a browser
return this._authBackend.getRedirectResult()
.map((userCredential: firebase.auth.UserCredential) => {
if (userCredential && userCredential.credential) {
authState = attachCredentialToAuthState(authState, userCredential.credential, userCredential.credential.provider);
this._credentialCache[userCredential.credential.provider] = <OAuthCredential>userCredential.credential;
}
return authState;
});
}

}
return Observable.of(authState);
})
Expand Down
1 change: 1 addition & 0 deletions src/tokens.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {OpaqueToken} from '@angular/core';
export const FirebaseConfig = new OpaqueToken('FirebaseUrl');
export const FirebaseApp = new OpaqueToken('FirebaseApp')
export const FirebaseAuthConfig = new OpaqueToken('FirebaseAuthConfig');
export const WindowLocation = new OpaqueToken('WindowLocation');
// TODO: Deprecate
export const FirebaseRef = FirebaseApp;
export const FirebaseUrl = FirebaseConfig;

0 comments on commit f38e9d7

Please sign in to comment.