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

Fix JWT format and interop with Storage Emulator. Fixes #3442. #5002

Merged
merged 8 commits into from
Aug 9, 2021

Conversation

abeisgoat
Copy link
Contributor

In rules-unit-testing, our naive implementation of unsigned JWT implementation wasn't spec compliant so it was breaking the storage emulator

@changeset-bot
Copy link

changeset-bot bot commented Jun 9, 2021

⚠️ No Changeset found

Latest commit: f602b9c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@abeisgoat
Copy link
Contributor Author

I broke... something this is gonna need a bit more work

@samtstern
Copy link
Contributor

samtstern commented Jun 10, 2021

@abeisgoat looks like the rules have begun failing, which probably means the token is not encoded correctly:

INFO: operation failed: 
Property custom_claim is undefined on object. for 'update' @ L3
com.google.cloud.datastore.core.exception.DatastoreException: 
Property custom_claim is undefined on object. for 'update' @ L3
	at com.google.cloud.datastore.core.exception.DatastoreException$Builder.build(DatastoreException.java:111)

@alejo4373
Copy link

Hi All! Any updates on this? I'm running into firebase-tools/issues/3442

Copy link
Member

@yuchenshi yuchenshi left a comment

Choose a reason for hiding this comment

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

BLOCKING: We also need to update https://github.com/firebase/firebase-js-sdk/blob/master/packages/util/src/emulator.ts#L138

I suggest we just switch to base64url and trim the padding ourselves instead of introducing another dependency since we don't want to bloat @firebase/util which is used in almost all firebase packages. Plus it's a much smaller change and easier to reason about, especially given the broken decoding right now.

@@ -197,11 +197,7 @@ function createUnsecuredJwt(token: TokenOptions, projectId?: string): string {

// Unsecured JWTs use the empty string as a signature.
const signature = '';
return [
base64.encodeString(JSON.stringify(header), /*webSafe=*/ false),
Copy link
Member

Choose a reason for hiding this comment

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

This needs use base64url instead of base64.

export const base64Encode = function (str: string): string {
is the utility for that. I think we still need to trim padding ourselves though.

@abeisgoat
Copy link
Contributor Author

@yuchenshi plz take a look and I'm not sure why things are failing after the package.json merge, any idea what that token error is?

Copy link
Member

@yuchenshi yuchenshi left a comment

Choose a reason for hiding this comment

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

Let's get this in first and then in a later PR I'll port the fix to https://github.com/firebase/firebase-js-sdk/blob/master/packages/util/src/emulator.ts#L138

@yuchenshi yuchenshi merged commit a65cd9a into firebase:master Aug 9, 2021
yuchenshi added a commit that referenced this pull request Aug 9, 2021
@yuchenshi
Copy link
Member

Fixes firebase/firebase-tools#3442.

This will be included in the next release.

@yuchenshi yuchenshi changed the title Move from manual JWT creation to jsonwebtoken Fix JWT format and interop with Storage Emulator. Fixes #3442. Aug 9, 2021
yuchenshi added a commit that referenced this pull request Aug 10, 2021
@firebase firebase locked and limited conversation to collaborators Sep 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants