Skip to content

feat: allow using service account with environment variables #1319

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

Closed
wants to merge 2 commits into from

Conversation

noook
Copy link

@noook noook commented Mar 12, 2023

Attempt to resolve issue #1297:

When using environment variables to use the service account, the service account is considered as not configured if it's something else than the service account's file's path. This PR keeps the initial check of the GOOGLE_APPLICATION_CREDENTIALS environment variable to determine if the service account file's path is provided, then checks if all the required fields of the service account's options are provided to consider as configured.

@@ -8,7 +8,6 @@ import {
createResolver,
defineNuxtModule,
} from '@nuxt/kit'
import type { NuxtModule } from '@nuxt/schema'
Copy link
Author

Choose a reason for hiding this comment

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

import was unused

@noook
Copy link
Author

noook commented Mar 16, 2023

@robokozo This PR allows registering the API session handler along the cookie minting when the service account is configured as an object

@dword-design
Copy link

@noook I'd like to have this feature merged! But it's not working for me right now because my service account files contain snake case entries, not camel case. Also, I think the check function can be simplified to this:

export function isServiceAccountConfigured(options) {
  return process.env.GOOGLE_APPLICATION_CREDENTIALS
    || (options.admin.serviceAccount?.client_email && options.admin.serviceAccount?.private_key && options.admin.serviceAccount?.project_id)
}

I tried to publish my own version from the fork but the fork is outdated and needs to be synced.

@posva
Copy link
Member

posva commented Jun 29, 2023

FYI I refactored the code that handles this and it should be fixed now but it's not released yet. We are allocating time for this and many more improvements for Nuxt starting next week with Firebase 🎉 . Expect the fix to be released very soon

@dword-design
Copy link

Oh that's great to hear! Then I'm maybe going with an in-between solution for now. Looking forward, this module is amazing and way better than what I tried some time ago.

@noook
Copy link
Author

noook commented Jun 29, 2023

@dword-design IIRC, I published a version of this package at @noook/vuefire on npm while waiting for this improvement (if this is very urgent)
Repository: https://github.com/noook/vuefire (Commit should be the one proposed in this PR)
Published package: https://www.npmjs.com/package/@noook/vuefire

@dword-design
Copy link

@noook Yeah I've seen this, thx for it! But it's not working for me, as written above the casing is different to what is in the service account JSON. And I tried to fix your change but I get errors during pnpm install, I think your branch needs to be rebased.

@posva
Copy link
Member

posva commented Jul 7, 2023

I updated the packages and it should now work with the env variable. I deprecated the serviceAccount option as it was dangerous. Let me know if it works

@dword-design
Copy link

@posva Currently having an issue where multiple versions of a dependency of a module (in this case lru-cache) are wrongly resolved. Just reported it. Probably a Nuxt issue.

@dword-design
Copy link

@posva Ok I artificially installed lru-cache at top level and now it works. Basically seems to work. I had the issue that if I set the client config via FIREBASE_CONFIG, it won't load GOOGLE_APPLICATION_CREDENTIALS. Looks like wrong precedence.

Why is the serviceAccount option dangerous?

Copy link
Member

posva commented Jul 10, 2023

Because it leads to leaking keys.
The precedence should be the same as Firebase (I think they have some docs) if it’s not, it should be adapted

@noook noook closed this Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants