Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented auth package; Exposing Auth service via App #3

Merged
merged 4 commits into from May 18, 2017

Conversation

hiranya911
Copy link
Contributor

  • Added the auth package which defines the Auth interface.
  • Exposing CustomToken, CustomTokenWithClaims and VerifyIDToken functions via the Auth service.
  • Adding Auth service to App

auth/auth.go Outdated
"github.com/firebase/firebase-admin-go/internal"
)

const firebaseAudience string = "https://identitytoolkit.googleapis.com/google.identity.identitytoolkit.v1.IdentityToolkit"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really need the types on here? I thought it was good go practice to omit types on constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

auth/auth.go Outdated
// all the key-value pairs in the provided map as claims in the resulting JWT.
CustomTokenWithClaims(uid string, devClaims map[string]interface{}) (string, error)

// VerifyIDToken verifies the signature and payload of the provided JWT.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you're referring to the use of JWT here. Clarified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, I'm referring to this comment floating out here in the middle of nowhere, when there's already a VerifyIDToken comment on the VerifyIDToken function. Maybe the next line needs a "//", if this is supposed to be one long comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's one long comment. Fixed.

auth/auth.go Outdated
return "", errors.New("parent Firebase app instance has been deleted")
}
if len(uid) == 0 || len(uid) > 128 {
return "", errors.New("uid must be non-empty, and less than 128 characters")
Copy link
Collaborator

Choose a reason for hiding this comment

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

"less than or equal"? Or is the condition wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed error message

auth/auth.go Outdated
err = fmt.Errorf("ID token has no 'kid' header")
}
} else if h.Algorithm != "RS256" {
err = fmt.Errorf("ID token has invalid incorrect algorithm. Expected 'RS256' but got '%s'. %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, technically, these aren't in the proper go error format, but I think changing them would make them worse, so I think this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

auth/auth.go Outdated
err = fmt.Errorf("ID token has invalid incorrect algorithm. Expected 'RS256' but got '%s'. %s",
h.Algorithm, verifyTokenMsg)
} else if p.Audience != projectID {
err = fmt.Errorf("ID token has invalid 'aud' (audience) claim. Expected '%s' but got '%s'. %s %s",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use %q instead of '%s' everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

for _, tc := range cases {
token, err := impl.CustomTokenWithClaims(tc.uid, tc.claims)
if token != "" || err == nil {
t.Errorf("CustomTokenWithClaims(%q) = (%q, %v); want: ('', error)", tc.name, token, err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would put "" instead of '' everywhere, but I don't feel strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

auth/auth.go Outdated
} else if p.Issuer != issuer {
err = fmt.Errorf("ID token has invalid 'iss' (issuer) claim. Expected '%s' but got '%s'. %s %s",
issuer, p.Issuer, projectIDMsg, verifyTokenMsg)
} else if p.IssuedAt > time.Now().Unix() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be a good idea to make a way to mock the clock for an authImpl so that you can test these conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

auth/crypto.go Outdated
return k.CachedKeys, nil
}

func (f *fileKeySource) Keys() ([]*publicKey, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's pretty confusing how httpKeySource and fileKeySource are all mixed up in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rearranged the code to make it more clear.

return &m.Response, m.Err
}

type mockReadCloser struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would easier to just use:

rc := ioutil.NopCloser(bytes.NewBuffer(data))

Wait, "counter" is how many times it's closed? It's very strange to call Close() multiple times...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the same mockReadCloser instance in multiple HTTP requests in this test. This allows us to use Close() as an indication of how many times the key source attempted to make HTTP calls (as opposed to reading from the cache). Under normal conditions (i.e. non-test), this will be an actual HTTP response instance created by the underlying http.Client.

}

mc.now = time.Unix(101, 0)
keys, err := ks.Keys()
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like it's not really safe to call Keys() on the same httpKeySource twice. Won't the how rc have been read by now anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be able to call it multiple times without issues. Each call may fetch the keys from the local cache, or a remote server. The mockReadCloser has a string, which it returns as a byte array in each read attempt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Documenting in-person conversation: let's figure out why this works when rc returns EOF on any subsequent call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clearing the cache when calling refreshKeys(). Resetting index in mock reader closer in Close().

@hiranya911 hiranya911 assigned hiranya911 and unassigned bklimt May 16, 2017
@hiranya911
Copy link
Contributor Author

Made the suggested changes

@hiranya911 hiranya911 assigned bklimt and hiranya911 and unassigned hiranya911 and bklimt May 16, 2017
@hiranya911
Copy link
Contributor Author

I've fixed the httpKeySource to reset the cache when refresh is called. Updated the test case accordingly. Introduced a mutex to prevent multiple threads from updating the shared cache concurrently.

@hiranya911 hiranya911 assigned bklimt and unassigned hiranya911 May 18, 2017
@bklimt
Copy link
Collaborator

bklimt commented May 18, 2017

LGTM

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

Successfully merging this pull request may close these issues.

None yet

2 participants