feat: Add function to verify an App Check token#642
Conversation
|
I also see the messaging.py has extensive error mapping. Should I be creating this as well? |
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you @dwyfrequency !
We are headed in the correct direction. I added a few comments. Thanks!
lahirumaramba
left a comment
There was a problem hiding this comment.
Looks great! Thanks @dwyfrequency!
I think we are good to work on the tests now.
Should we also add a new error type instead of raising value errors?
firebase_admin/app_check.py
Outdated
| # Note: It is not recommended to hard code these keys as they rotate, | ||
| # but you should cache them for up to 6 hours. | ||
| # Default lifespan is 300 seconds (5 minutes) so we change it to 21600 seconds (6 hours). | ||
| jwks_client = PyJWKClient(self._JWKS_URL, lifespan=21600) |
There was a problem hiding this comment.
Sorry for not catching earlier, I think we should initialize this in the constructor and make jwks_client a class member. Otherwise, every time we call verify_token() it will initialize a new instance and I don't think that is necessary plus it will also break the caching mechanism.
firebase_admin/app_check.py
Outdated
| verified_claims = self._decode_and_verify(token, signing_key.key) | ||
|
|
||
| # The token's subject will be the app ID, you may optionally filter against | ||
| # an allow list |
There was a problem hiding this comment.
I think we can remove you may optionally filter against an allow list part. It was a comment in the code sample to help developers, but we don't do the filtering in the SDK.
firebase_admin/app_check.py
Outdated
| ) | ||
| except ExpiredSignatureError: | ||
| raise ValueError( | ||
| 'The provided App Check token signature has expired.' |
There was a problem hiding this comment.
| 'The provided App Check token signature has expired.' | |
| 'The provided App Check token has expired.' |
firebase_admin/app_check.py
Outdated
| ) | ||
| except InvalidSignatureError: | ||
| raise ValueError( | ||
| 'The provided App Check token signature cannot be verified.' |
There was a problem hiding this comment.
| 'The provided App Check token signature cannot be verified.' | |
| 'The provided App Check token has invalid signature.' |
| if not isinstance(audience, list) or self._scoped_project_id not in audience: | ||
| raise ValueError('Firebase App Check token has incorrect "aud" (audience) claim.') | ||
| if not payload.get('iss').startswith(self._APP_CHECK_ISSUER): | ||
| raise ValueError('Token does not contain the correct "iss" (issuer).') |
There was a problem hiding this comment.
Can we also validate the sub claim?
example from Node.js:
} else if (typeof payload.sub !== 'string') {
errorMessage = 'The provided App Check token has no "sub" (subject) claim.';
} else if (payload.sub === '') {
errorMessage = 'The provided App Check token has an empty string "sub" (subject) claim.';
}
```
egilmorez
left a comment
There was a problem hiding this comment.
With Kevin's suggested changes, LGTM, thanks!
Add Firebase functionality to verify an App Check token
RELEASE NOTE: Added a new
app_check.verify_token()API to verify App Check tokens.