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

feat: Add function to verify an App Check token #642

Merged
merged 29 commits into from Sep 29, 2022
Merged

Conversation

dwyfrequency
Copy link
Contributor

@dwyfrequency dwyfrequency commented Sep 7, 2022

Add Firebase functionality to verify an App Check token

RELEASE NOTE: Added a new app_check.verify_token() API to verify App Check tokens.

@dwyfrequency dwyfrequency changed the title Jd verify token Add function to verify an App Check token Sep 8, 2022
@dwyfrequency
Copy link
Contributor Author

I also see the messaging.py has extensive error mapping. Should I be creating this as well?

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you @dwyfrequency !
We are headed in the correct direction. I added a few comments. Thanks!

firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Show resolved Hide resolved
firebase_admin/app_check.py Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

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?

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
@dwyfrequency dwyfrequency marked this pull request as ready for review September 23, 2022 20:45
Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

# 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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

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
Copy link
Member

Choose a reason for hiding this comment

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

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.

)
except ExpiredSignatureError:
raise ValueError(
'The provided App Check token signature has expired.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'The provided App Check token signature has expired.'
'The provided App Check token has expired.'

)
except InvalidSignatureError:
raise ValueError(
'The provided App Check token signature cannot be verified.'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'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).')
Copy link
Member

Choose a reason for hiding this comment

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

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.';
    }
    ```

firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
firebase_admin/app_check.py Outdated Show resolved Hide resolved
Copy link

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

With Kevin's suggested changes, LGTM, thanks!

@lahirumaramba lahirumaramba changed the title Add function to verify an App Check token feat: Add function to verify an App Check token Sep 29, 2022
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.

None yet

4 participants