Skip to content

Conversation

jteplitz
Copy link
Contributor

@jteplitz jteplitz commented Feb 5, 2016

Part of #4

@jeffbcross
Copy link
Contributor

@jteplitz602 great PR! It brings up some questions of how to best abstract Firebase.

The current implementation essentially proxies to Firebase SDK methods, so any component in the app that would want to log in would need to know the right method and configuration to use to log in.

// This would probably be imported from a config, or injected
const provider:AuthProviders = AuthProviders.Github;

@Component({
  selector: 'my-login'
})
export class MyLogin {
  constructor(private _af:AngularFire) {}
  login () {
    this._af.authWithOAuthPopup(provider);
  }

  logout () {
    this._af.unauth()
  }
}

Could you explore moving some of the options to injectable configuration, and make the auth service just have login and logout? The login method should accept options to override defaults of course. For example:

bootstrap(App, [
  FIREBASE_PROVIDERS,
  defaultFirebase('http://somefirebase'),
  defaultAuthProvider(AuthProviders.Github),
  // We could even have a default if we find that one method is far more common than others
  defaultAuthMethod(AuthMethods.PopUp)
]);

@Component({
  template: `
    <button (click)="af.auth.login()">Login with Github</button>
    <button (click)="twitterLogin()">Login with Twitter</button>
  `
})
class App {
  constructor(public af:AngularFire) {}
  twitterLogin() {
    // Would still use default method of PopUp
    this.af.auth.login({provider: AuthProviders.Twitter});
  }
}

Maybe you've already given this approach of "fewer methods+more configurability" some thought and have some reasons why it provides a worse user experience.

@jteplitz
Copy link
Contributor Author

@jeffbcross In general I love the idea of reducing the number of public methods, but I'm not sure it will work here. That design works well for popup auth, redirect auth, and anonymous auth. But it runs into problems with customToken, email, and oauthToken login. These methods all require specific arguments. It feels weird to me that your call to login could suddenly become invalid and throw an error if you change your default auth method in your bootstrap file. Especially because we wouldn't be able to provide static errors for this since all the arguments to login would have to be optional.

We could separate the firebase login methods into two groups: automatic login (popup, redirect, and anonymous) and manual login (customToken, email, and oauthToken). We could provide one generic login method for the first group and then three manual login methods for the second group, but that might be overkill. What do you think?

@TylerEich
Copy link

I've never actually implemented Firebase's login, please forgive my naivety!

The auth service seems to be inherently stateful, and the internal details of providers, credentials, and tokens are nearly constant through the entire life of the app.

Could we possibly have a separate means of providing options (like custom OAuth tokens, passwords/emails, etc.) with some kind of setOptions() method? In a well designed app, those details should be in a single component/service that manages the user's session.

I'm assuming the .login() or .auth() methods are used for e.g. @CanActivate and other conditional behavior. The methods should be as simple as possible; keep the options separate, since they'll rarely change.

Please correct anything I've got backwards!

@jteplitz
Copy link
Contributor Author

@TylerEich thanks for the input!
I just chatted with @jeffbcross about this. Here's what we decided on. We're going to have one login method that by default uses options provided through DI. So if your app generally authenticates users by doing Github login with a popup you would provide these options to DI using something like

// Providers to give bootstrap
provide(DefaultFirebaseAuthProvider, AuthProviders.Github),
provide(DefaultFirebaseAuthMethod, AuthMethods.Popup)

// to login
auth.login();

However, all of these options can be overwritten by an optional config object that gets passed into login. So if you actually wanted to use Facebook instead of Github you could do:

// Providers to bootstrap
provide(DefaultFirebaseAuthProvider, AuthProviders.Github),
provide(DefaultFirebaseAuthMethod, AuthMethods.Popup)

// this will login the user with Facebook popup since Facebook will override Github, but nothing overwrote Popup
auth.login({
  provider: AuthProviders.Facebook
});

login will also take a second optional argument for credentials. So if you're using customToken, email, or OAuthToken login you'll do something like this to login:

auth.login({
  method: AuthMethods.Password
}, {
  email: 'test@example.com',
  password: 'password'
})

@jeffbcross
Copy link
Contributor

@jteplitz602 +1 I think we could drop the Default from the provider name.

@jteplitz
Copy link
Contributor Author

Talked to @jeffbcross and @hansl a bit more. We're going to create one config provider, and have two signatures for the login method. I'll write up a longer description this afternoon.

@TylerEich
Copy link

@jteplitz602 @jeffbcross yeah, I like that design better. Keeps the API clean and centralized 😎👍

@jteplitz
Copy link
Contributor Author

Here's a more complete overview of the design that we settled on:

You can, but are not required to, provide a single set of configuration arguments to DI like this:

bootstrap(app, [
  FIREBASE_PROVIDERS,
  defaultFirebase('https://<some-firebase>.firebaseio.com'),
  firebaseAuthConfig({
    provider: AuthProviders.Facebook,
    method: AuthMethods.Popup,
    remember: 'default',
    scope: ['email']
  })
]);

Once you've done that you can simply call login on the auth object. This will automatically use the options that were configured with DI. You can override those options by providing an optional configuration object to the login method like so:

// This will perform popup auth with google oauth and the scope will be email
auth.login({
  provider: AuthProviders.Google
});

The Auth service will use all values provided by the given configuration object over those provided through DI, and will fall back to the DI provided config object (if available) for any options not provided by the caller.

Additionally, if you're using OAuthToken, CustomToken, or Password auth then you can pass a credentials object to login like so:

bootstrap(app, [
  FIREBASE_PROVIDERS,
  defaultFirebase('https://<some-firebase>.firebaseio.com'),
  firebaseAuthConfig({
    method: AuthMethods.Password,
    remember: 'default'
  })
]);
auth.login({
  email: 'test@example.com',
  password: 'password'
});

If you want to pass both a credentials object and a configuration object you can do so like this:

auth.login({
  email: 'test@example.com',
  password: 'password'
}, {
  method: AuthMethods.Password,
  remember: 'sessionOnly'
});

If two arguments are passed to login than the service will assume that the first one is a credentials object and the second one is a configuration object. If only one is passed then it will assume that argument is a configuration object unless the auth method is set to Password, OAuthToken, or CustomToken in which case it will assume that object is a credentials object since credentials are not optional for any of those three methods and the configuration object is always optional. Essentially the login method has two signatures:

login(config?: AuthConfiguration): Promise<FirebaseAuthState>;
login(credentials: AuthCredentials, config?: AuthConfiguration): Promise<FirebaseAuthState>;

The first signature is used for Popup, Redirect, and anonymous auth. The second signature is used for
Password, OAuthToken, and CustomToken.

@jeffbcross does that sound like a good overview of everything we discussed?

README.md Outdated
@@ -1,4 +1,4 @@
# AngularFire2
# AngularFire3
Copy link
Contributor

Choose a reason for hiding this comment

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

haha what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got ahead of myself 😉

There's a vim key that increments numbers. I guess I hit it by accident...

}
config = this._mergeConfigs(config);

if (!config.hasOwnProperty('method')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace all hasOwnProperty instances with something inheritance-friendly.

@jeffbcross
Copy link
Contributor

LGTM with comments addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants