Introduce auth provider interface #902
Conversation
This introduces an abstraction between firebase and our system called "AuthProvider". Right now the only auth provider is Firebase, but it could be extended in the future, with the primary use case being automated testing. There's currently no way to configure a different auth provider without writing custom code.
Part of this required using more of the Firebase server APIs. However, I found a way to plumb through the user's auth token and use that (instead of our service account), so rate limiting should be much less restrictive. In my testing, I was only able to get rate limited once and I must have tried a few hundred times over an hour. Immediately trying with a different email succeeded, demonstrating that the limiting is scoped to email (idToken) and not our service account.
The vast majority of the work involved **decoupling our controller logic from firebase** and **decoupling firebase from our smtp implementation**. There are some weird qwirks in the interface (including two places where I decided to accept an `interface{}` and force the type decision onto the implementer for flexibility), but overall I think it's pretty good.
This isn't 100% complete and will need a few tweaks to the interface{}, as we add more testing, but I think it's good to merge and iterate on in its current state.
Some other things wrapped up in the refactor:
- Extract email, email verified, and MFA factors from the session cookie. This saves a few round trip lookups to firebase for both performance and rate limiting purposes.
- Invitations for new system admins can use the system SMTP (if configured) to send invitations.
googlebot
added
the
cla: yes
| @@ -9,14 +8,12 @@ | |||
| }); | |||
|
|
|||
| function setSession(user) { | |||
| let factorCount = user.multiFactor.enrolledFactors.length; | |||
There was a problem hiding this comment.
This is available in the cookie now, we don't need to pull it from the javascript.
| + "The code may be malformed, expired, or has already been used."); | ||
| }); | ||
| .then(function(resp) { | ||
| window.location.assign("/"); |
There was a problem hiding this comment.
The previous implementation assumed that the user was logged in when they verified their email. It's possible they were on a different computer (e.g. desktop but opened on mobile) or incognito. The result would be a successful verification, then an unauthorized page. This fixes it by redirecting people to the home page (after which our normal flow picks up).
| } | ||
| {{end}} | ||
| // Get an ID token and embed it onto the page. | ||
| user.getIdToken().then(idToken => { |
There was a problem hiding this comment.
There's technically a race here. If the user leaves this page open for a few hours, the code will expire. I think it's fine.
| return | ||
| } | ||
|
|
||
| if err := c.db.PasswordChanged(email, time.Now()); err != nil { | ||
| if err := c.db.PasswordChanged(email, time.Now().UTC()); err != nil { |
There was a problem hiding this comment.
This is better, but I think the db retains the local timezone info, so when we later subtract to get time since last pwd change, it still works.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This introduces an abstraction between firebase and our system called "AuthProvider". Right now the only auth provider is Firebase, but it could be extended in the future, with the primary use case being automated testing. There's currently no way to configure a different auth provider without writing custom code.
Part of this required using more of the Firebase server APIs. However, I found a way to plumb through the user's auth token and use that (instead of our service account), so rate limiting should be much less restrictive. In my testing, I was only able to get rate limited once and I must have tried a few hundred times over an hour. Immediately trying with a different email succeeded, demonstrating that the limiting is scoped to email (idToken) and not our service account.
The vast majority of the work involved decoupling our controller logic from firebase and decoupling firebase from our smtp implementation. There are some weird qwirks in the interface (including two places where I decided to accept an
interface{}and force the type decision onto the implementer for flexibility), but overall I think it's pretty good.This isn't 100% complete and will need a few tweaks to the interface{}, as we add more testing, but I think it's good to merge and iterate on in its current state.
Some other things wrapped up in the refactor:
Extract email, email verified, and MFA factors from the session cookie. This saves a few round trip lookups to firebase for both performance and rate limiting purposes.
Invitations for new system admins can use the system SMTP (if configured) to send invitations.
Release Note