feat(appcheck): Add initial classes for App Check #672
Conversation
| } else if (!claimsSet.getIssuer().startsWith(APP_CHECK_ISSUER)) { | ||
| errorMessage = "invalid iss"; | ||
| } else if (claimsSet.getSubject().isEmpty()) { | ||
| errorMessage = "invalid sub"; |
There was a problem hiding this comment.
Please ignore the error messages for now, these need to be improved to add more detail.
| + "environment variable."); | ||
| AppCheckTokenVerifier appCheckTokenVerifier = AppCheckTokenVerifier.builder() | ||
| .setProjectId(projectId) | ||
| .build(); |
There was a problem hiding this comment.
I think we can use a token factory to clean this up a bit more later. That way we can also configure the JWKS url, cache duration, etc.
| INTERNAL, | ||
|
|
||
| /** | ||
| * User is not authenticated. |
There was a problem hiding this comment.
Just to confirm: this error is used for token minting using ExchangeCustomToken, right? Maybe we can be specific here and say "The service account credential is invalid."
There was a problem hiding this comment.
You are right. We don't need this for verify token API. Let's remove it for now and add when we implement the createToken API later.
| final class AppCheckTokenVerifier { | ||
|
|
||
| private final URL jwksUrl; | ||
| private final String projectId; |
There was a problem hiding this comment.
Minor point: Is it possible to base this on project number (a long)? This will comply with https://google.aip.dev/cloud/2510 for third-parties only using project numbers.
There was a problem hiding this comment.
So this is interesting. The Admin SDK uses the project ID (string) and does not have any context of the project number. We addressed this by converting aud to an array and adding both id and number in the same claim.
| DecodedAppCheckToken verifyToken(String token) throws FirebaseAppCheckException { | ||
| SignedJWT signedJWT; | ||
| JWTClaimsSet claimsSet; | ||
| String scopedProjectId = String.format("projects/%s", projectId); |
There was a problem hiding this comment.
Nit: instead of scopedProjectId, we should use the term projectName (in the spirit of https://google.aip.dev/122).
| errorMessage = String.format("The provided App Check token has incorrect 'aud' (audience) " | ||
| + "claim. Expected %s but got %s. %s", | ||
| scopedProjectId, claimsSet.getAudience().toString(), projectIdMatchMessage); | ||
| } else if (!claimsSet.getIssuer().startsWith(APP_CHECK_ISSUER)) { |
There was a problem hiding this comment.
Prefer strict .equals() here instead of .startsWith().
There was a problem hiding this comment.
The issuer contains the App ID (which is not known to the Admin SDK). That's why we decided to do a startsWith here. Also see the equivalent of Node.js https://github.com/firebase/firebase-admin-node/blob/a32195daa9848b261fe892d9f606152a40ff2915/src/app-check/token-verifier.ts#L121
| * A decoded and verified Firebase App Check token. See {@link FirebaseAppCheck#verifyToken(String)} | ||
| * for details on how to obtain an instance of this class. | ||
| */ | ||
| public final class DecodedAppCheckToken { |
There was a problem hiding this comment.
I almost wonder whether we should be exposing all the contents of the token or offer our own opinionated data layer. For example, we can offer just one method getAppId() for now like our Node.js (I think).
There was a problem hiding this comment.
In go/fac-admin-verify-token we decided to wrap the full token in a response object with helper methods like getAppId(). So it will be something similar to:
public final class VerifyAppCheckTokenResponse {
public String getAppId()
public DecodedAppCheckToken getToken()
}
This is similar to the Node.js API: https://github.com/firebase/firebase-admin-node/blob/a32195daa9848b261fe892d9f606152a40ff2915/src/app-check/app-check-api.ts#L95
|
Is there an estimate for when this logic will be available in a release ❓🛳️ We are using a java-only backend and would like to verify (don't know if this the right place to post/ask about this, please advise if that's not the case 👀) |
|
Hey @bhackstock you at the right place! I can't promise you a timeline, but this is a feature that we currently have on our roadmap. Stay tuned! :) |
| } | ||
|
|
||
| /** Returns the Expiration Time for this token. */ | ||
| public String getExpirationTime() { |
There was a problem hiding this comment.
IIRC we don't yet require Java 8+ for our SDKs, so we can't use the java.time API. But we could at least return an integer (and document it as seconds since the Unix epoch) instead of a string.
| } | ||
|
|
||
| /** Returns the Issued At for this token. */ | ||
| public String getIssuedAt() { |
There was a problem hiding this comment.
ditto. Ideally that'd return java.time.Instant, but barring that this should return an integer like getExpirationTime()
There was a problem hiding this comment.
Good call. Change to Long here and above. Thanks!
|
Is this being worked on? |
lahirumaramba
force-pushed
the
lm-fac-init
branch
from
22e4bf1
to
5e1aea3
Compare
lahirumaramba
changed the title
lahirumaramba
force-pushed
the
lm-fac-init
branch
from
5e1aea3
to
b8bfc49
Compare
|
Any progress on this? It's kind of strange that AppCheck is not part of the SDK API after such a long time... |
|
Hi, is there any chance that it will be implemented? |
verifyToken()API