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: Make ADC + human account work with firebase-admin #2553

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

foxrafa
Copy link
Contributor

@foxrafa foxrafa commented May 13, 2024

This PR is intended to fix a current bug in firebase-admin.

Google Cloud recommends utilizing [Application Default Credentials (ADC)] (https://cloud.google.com/docs/authentication/application-default-credentials), however this feature does not fully work with the firebase-admin-node library when you want to utilize a human account (person@domain.com) instead of a service account.

Some GCP APIs require you to specify the quota project associated to the request, however, the current implementation of the firebase-admin-node library does not provide the project ID when making requests to GCP APIs, even if you set them through gcloud auth application-default set-quota-project YOUR_PROJECT, because the code that handles the requests never adds the project ID to the request. And you get errors like this:

Your application is authenticating by using local Application Default Credentials. The identitytoolkit.googleapis.com API requires a quota project, which is not set by default. To learn how to set your quota project, see https://cloud.google.com/docs/authentication/adc-troubleshooting/user-creds .

Therefore, this pull request aims to fix this issue by checking if a projectId was provided when calling the initializeApp() function and adds it to the x-goog-user-project header of requests made to GCP APIs, just like the documentation states it should be set.

This implementation is similar to how the (working) google-auth-library library handles ADC authentication (src/auth/authclient.ts:270):

    /**
     * Append additional headers, e.g., x-goog-user-project, shared across the
     * classes inheriting AuthClient. This method should be used by any method
     * that overrides getRequestMetadataAsync(), which is a shared helper for
     * setting request information in both gRPC and HTTP API calls.
     *
     * @param headers object to append additional headers to.
     */
    addSharedMetadataHeaders(headers) {
        // quota_project_id, stored in application_default_credentials.json, is set in
        // the x-goog-user-project header, to indicate an alternate account for
        // billing and quota:
        if (!headers['x-goog-user-project'] && // don't override a value the user sets.
            this.quotaProjectId) {
            headers['x-goog-user-project'] = this.quotaProjectId;
        }
        return headers;
    }

This should also solve some problems described in #1377

Copy link

google-cla bot commented May 13, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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!

@lahirumaramba lahirumaramba changed the title Make ADC + human account work with firebase-admin fix: Make ADC + human account work with firebase-admin May 14, 2024
@lahirumaramba
Copy link
Member

Hi @foxrafa could you please take a look at the lint errors in the failing CIs? Thanks!

@foxrafa
Copy link
Contributor Author

foxrafa commented May 14, 2024

@lahirumaramba fixed it.

@foxrafa
Copy link
Contributor Author

foxrafa commented May 14, 2024

Oh, all of the test cases are not expecting the x-goog-user-project header. Can I change them @lahirumaramba ?

@lahirumaramba
Copy link
Member

@lahirumaramba fixed it.

Thanks! We also need to update the unit tests :)

@foxrafa
Copy link
Contributor Author

foxrafa commented May 14, 2024

@lahirumaramba fixed it.

Thanks! We also need to update the unit tests :)

I updated them now.

@rogsilva
Copy link

When will a new version with this change be released?

@foxrafa
Copy link
Contributor Author

foxrafa commented Jun 2, 2024

@lahirumaramba any ETA on when this fix will be generally available?

@lahirumaramba
Copy link
Member

Thanks for the updating the tests! This issue will also be addressed in the credentials migration work #2466 (which I think is the proper way to address this issue). For now, I am okay with us merging this PR as a stopgap

@lahirumaramba lahirumaramba self-assigned this Jun 18, 2024
@lahirumaramba lahirumaramba merged commit 5f0f253 into firebase:master Jun 19, 2024
8 checks passed
@lahirumaramba
Copy link
Member

Thanks @foxrafa ! this change is now included in the v12.2.0 release!

maxl0rd added a commit to firebase/genkit that referenced this pull request Jun 25, 2024
- Picks up firebase/firebase-admin-node#2553
  which unlocks using application default crendentials for local development.
@erlichmen
Copy link

This fixes so much pain, thank you for that.

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.

4 participants