IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

feat: deployment-based firebase config #2235

Closed chrismclarke closed 3 months ago

chrismclarke commented 4 months ago

PR Checklist

Breaking Changes

In cases where deployments are targetting an older version of the main repo for actions they should also make sure to target the older version of reusable actions if not upgrading to the new syntax

It could be possible in the future to alter github actions to provide config using the reusable actions if useful

Description

Review Notes

The PR removes .firebaserc and firebase.json hardcode files, but also adds them to the .gitignore to allow users to store local copies. I removed the files via git rm so it should force the deletion through, but would be good to check it actually worked as intended and doesn't leave the files populated in local environment.

There will still be a src\environments\firebaseConfig.ts file left (as was previously gitignored) - this can just be deleted

For testing, there's essentially 4 cases to go through - summary of current findings below.

config exists config missing
firebase enabled Works as expected Deployment set and runtime warnings, crashlytics still loads on native using google-services.json
firebase disabled Runtime warnings if calling actions Crashlytics still initialises but then is disabled

In order to test these any config can be selected and modified to include/exclude loading firebase config and toggling features. E.g. placing a firebase.json config file in the deployment encrypted folder and updating deployment

import { loadEncryptedConfig} from "scripts";

config.firebase = {
  config: loadEncryptedConfig('firebase.json'), // set to null to omit debug missing config or do not populate encrypted
  auth:{enabled:true},
  crashlytics:{enabled:true}
}

Any changes should be recompiled to take effect, either by running the yarn workflow deployment set command or manually editing the compiled .idems_app\deployments\activeDeployment.json

Author Notes

I've marked as legacy (but not removed) documentation that refers to how we used to use deployment branches to deploy multiple apps from the main repo. I'm not sure if we want to keep this, and whether the documentation I've added related to content-repo actions are already covered elsewhere (or should be?)

I've also removed the references to firebase config from the quickstart guides. It would be useful if someone could update the documentation (or add to future list) to include how to provide project-specific firebase config using encrypted environment, and that it is only required if using auth actions

Deployment configs now have to specify what firebase services are enabled. I've set the defaults to enable crashlytics and disable auth (as I think that is the most common setup for projects) - so deployments using auth will need to be manually updated, and we may want to consider explicitly setting the crashlytics option on deployments in case we ever decide to revert the default to enabled: false

firebase: {
    auth: { enabled: false },
    crashlytics: { enabled: true },
  }

There is a new utility function loadEncryptedConfig(fileName) function that can be used to load json files from the encrypted folder. I've made a pr to implement these changes on debug repo here: https://github.com/IDEMSInternational/app-debug-content/pull/54

import { loadEncryptedConfig} from "scripts";

config.firebase = {
  config: loadEncryptedConfig('firebase.json'),
  auth:{enabled:true},
  crashlytics:{enabled:true}
}

Dev Notes

The PR includes templated .firebaserc and firebase.json files, however as of now I don't think these are actually used as the reusable github actions overwrite the files. e.g. reusable-deploy-pr-preview.yml

We may want to consider simplifying the approach to work from templates with some sort environment variable substitution (I've defined the template in a way that should be compatible with tools like env_substr) - will make follow-up issue and consult with Chris M

I haven't include a way to fully opt out of firebase altogether as this would require modifying android build.gradle dependencies and capacitor.config.ts, however runtime services are disabled.

I'm hoping that the new loadEncryptedConfig function should make it easier to assign config properties with graceful fail. I've updated the supabaseConfig to operate in a similar way, although wondering if it might also make sense to destructure a little more like firebase, namely {supabase: {config: loadEncryptedConfig()} and assume enable/disabled based on whether config exists (?)

I've removed the @angular/fire package as it was causing issue with conditional app initialisation (possibly related to https://github.com/angular/angularfire/issues/3443), and actually as it is only a wrapper around firebase js sdk we're not really using because we use the @capacitor-firebase wrappers instead. The one use case we had was to initialise the core firebase app globally, so now I've just added a service to handle that instead

Git Issues

Closes #1976

Screenshots/Videos

Example - deployment set warning if encrypted config defined but not available image

Example - runtime console warning during service initialisation if firebase config missing image

Example - runtime console error when using auth actions without firebase config (non-breaking) image

Example - android console when firebase config missing (still enables crashlytics using data from google-services.json) image

chrismclarke commented 3 months ago

Thanks for tidying up the docs @jfmcquade , once merged would be good to move your notes to the documentation repo to outline how to integrate firebase auth. I've also tidied up following your comments

The values for firebase.json/firebaseConfig come from the firebase console, where a config file can be viewed for each registered web app. The only difference between the config file of web apps in the same project (as all our apps currently are) is the value of appId, which varies by app.

Yes that is my understanding also, looking through the various plh-related apps on the firebase console can confirm only the appId property changing

For registered Android apps, there is no file containing a firebaseConfig object available from the console, instead there is an associated google-services.json file, which doesn't contain the data in the same format. How then, do we enable firebase auth for an Android app that doesn't have an associated firebaseConfig object? The appId will also differ from any web app – is this value not actually used anywhere?

The firebaseConfig object is used to interact with web sdks. Some firebase services require this, others don't. E.g. if using firestore db on android you will still need to pass a firebase config, however authentication has fully native methods which do not require. So we should be able to add firebase auth to any project that has an android app registered in the firebase console without a corresponding web config. In this case there would still be an entry in google-services.json, but auth would not work on web deployment versions.

As for the differing info in google-services vs firebase config - they both contain unique IDs which I imagine can be used to retrieve whatever information firebase requires from its backend. Even though the google-services specifies multiple projects I expect it can simply use the package_name property to identify what package is being run and then the client_id to retrieve any additional configuration values.

jfmcquade commented 3 months ago

The firebaseConfig object is used to interact with web sdks. Some firebase services require this, others don't. E.g. if using firestore db on android you will still need to pass a firebase config, however authentication has fully native methods which do not require. So we should be able to add firebase auth to any project that has an android app registered in the firebase console without a corresponding web config. In this case there would still be an entry in google-services.json, but auth would not work on web deployment versions.

Ah yeh, I think I understand now. I was forgetting that google-services.json would still be populated for the Android build from the deployment-level secret.

So to be clear, an author could still legitimately set config.firebase.auth.enabled = true for a deployment without supplying a value for config.firebase.config, and the auth actions would then work for the Android release (provided a valid GOOGLE_SERVICES_JSON secret was included on the deployment repo), but not on the web version?

I think that then needs to be handled in the firebase service: currently the firebase app is not initialised if the config is missing, regardless of platform. In fact a config is required in order to initialise the firebase app via the initializeApp method from firebase/app, so it may not be straightforward to handle that case. Perhaps we just require that there's an associated web app in order to enable firebase auth, even if the auth functionality is only required on Android?

chrismclarke commented 3 months ago

So to be clear, an author could still legitimately set config.firebase.auth.enabled = true for a deployment without supplying a value for config.firebase.config, and the auth actions would then work for the Android release (provided a valid GOOGLE_SERVICES_JSON secret was included on the deployment repo), but not on the web version?

I think that then needs to be handled in the firebase service: currently the firebase app is not initialised if the config is missing, regardless of platform. In fact a config is required in order to initialise the firebase app via the initializeApp method from firebase/app, so it may not be straightforward to handle that case. Perhaps we just require that there's an associated web app in order to enable firebase auth, even if the auth functionality is only required on Android?

We can choose how to handle, but as of now the app expects both the option to be enabled and have a valid web config. Given that I'm also not entirely sure which of the web SDK methods work 100% natively (google sign-in/out does, but would need to confirm things like state listeners or other auth providers), I'd say probably best to keep this way for simplicity

 if (firebase?.auth?.enabled && this.firebaseService.app) {
      this.subscribeToAppConfigChanges();
      this.addAuthListeners();
      this.registerTemplateActionHandlers();
    }
jfmcquade commented 3 months ago

As the filename passed to loadEncryptedConfig() is expected to be already decrypted, I think we need an additional action, or step in the existing build action, that decrypts the contents of the encrypted folder for a deployment, using a private key. Otherwise we can only enable functionality that depends on encrypted config values when building the app locally.

This could make use of the existing yarn workflow deployment decrypt, after populating a file to .idems_app/deployments/<deplyment_name>/encrypted/private.key from the value of a PRIVATE_KEY secret, for example.

chrismclarke commented 3 months ago

As the filename passed to loadEncryptedConfig() is expected to be already decrypted, I think we need an additional action, or step in the existing build action, that decrypts the contents of the encrypted folder for a deployment, using a private key. Otherwise we can only enable functionality that depends on encrypted config values when building the app locally.

This could make use of the existing yarn workflow deployment decrypt, after populating a file to .idems_app/deployments/<deplyment_name>/encrypted/private.key from the value of a PRIVATE_KEY secret, for example.

The reusable build action should already include this when running on CI image

And the deployment set action automatically includes decrypt as part of the workflow image

Not sure if there's another case I'm missing here?

jfmcquade commented 3 months ago

The reusable build action should already include this when running on CI

Right you are. This was the case I was thinking of, I'd missed this step in the action.

I think should be good to merge pending review from @esmeetewinkel

jfmcquade commented 3 months ago

@esmeetewinkel, to summarise our discussion for clarity: this PR does not effect web preview deployments to Firebase hosting.

Deploying to Firebase hosting is handled by the reusable-deploy-web-preview github action, which takes its variables from hardcoded github secrets on the deployment repo (minimally required are: a Firebase project ID, e.g. plh-teens-app1; a Firebase hosting target, e.g. plh-kids-tz; and the credentials of a service account that is authorised to deploy to Firebase hosting for the relevant project). These properties could potentially be included as (encrypted) parts of the deployment config in the future if desired.

The "Firebase config" that is the concern of this PR is used for runtime Firebase functionality (auth and crashlytics). This is complicated a bit by the fact that Android apps are configured in a different way to web apps – as part of the build process, an Android app needs to pull its Firebase config data from a google-services.json file. This is handled via a secret on the deployment repo and is not changed by this PR (again, this could potentially be configured to be read from the deployment config if desired). So technically the changes of this PR only affect the web app version. However, the way that the deployment config is interpreted, in order to enable Firebase features, e.g. auth, a valid web app-related Firebase config must be provided, even if the final app will be an Android app.

A major benefit of this PR is that authors and devs are no longer required to populate a secret firebaseConfig file as part of setting up the dev environment. When working on a deployment locally that does not make explicit use of Firebase features (auth), the locally built web app will not connect to Firebase at all. This wasn't previously possible, as all apps connected to Firebase regardless.

@chrismclarke – feel free to clarify/correct anything

chrismclarke commented 3 months ago

Thanks for this, I've read through and I think I'm understanding the main points.

To test this on PLH Teens TZ, I made the changes described above, see this content PR. I believe I can only test whether the Google Auth still works with the Appetize action on this PR, but I don't know how to enable it (see #2182)

I think with the way the reusable actions are setup it might be hard to test action changes that are part of a PR .

A content repo refers directly to the master branch action, so in order to call the code that is sitting in this pr the content repo would also have to update the target for the action, e.g.

.github/workflows/deploy-pr-preview.yml

jobs:
  pr_preview:
    uses: IDEMSInternational/parenting-app-ui/.github/workflows/reusable-deploy-pr-preview.yml@master
    secrets: inherit

There is then an added challenge that the action also checks out the repo at a given reference, using an environment variable set at the parent repo level e.g.

.github/workflows/reusable-app-build.yml

   steps:
        - name: Check out app code
          uses: actions/checkout@v3
          with:
            repository: "IDEMSInternational/parenting-app-ui.git"
            ref: ${{env.APP_CODE_BRANCH}}

So I think we would likely need to update the content repo actions to use the same APP_CODE_BRANCH variable, and then create some means of triggering the actions with that variable (likely by creating a 3rd action within the content repo itself which can be manually triggered)

- uses: IDEMSInternational/parenting-app-ui/.github/workflows/reusable-deploy-pr-preview.yml@master
+ uses: IDEMSInternational/parenting-app-ui/.github/workflows/reusable-deploy-pr-preview.yml@${{env.APP_CODE_BRANCH}}

(although I doubt you can even pass a dynamic string to a github uses block, would need testing)

We'd also need to do a bit of extra work on the debug content repo to provide some sort of test_action action that takes the name of the reusable action to trigger and override of any other environment variables as required. (not sure if this makes sense to you @jfmcquade and whether something you might have capacity to do - otherwise we could create a follow-up issue and try to clarify a bit more)

Otherwise the only way to functionally test will be post-merge

jfmcquade commented 3 months ago

We'd also need to do a bit of extra work on the debug content repo to provide some sort of test_action action that takes the name of the reusable action to trigger and override of any other environment variables as required. (not sure if this makes sense to you @jfmcquade and whether something you might have capacity to do - otherwise we could create a follow-up issue and try to clarify a bit more)

I think a follow up issue with a bit more clarity would be good. I'm happy to look into it but am unlikely to do so immediately

chrismclarke commented 3 months ago

We'd also need to do a bit of extra work on the debug content repo to provide some sort of test_action action that takes the name of the reusable action to trigger and override of any other environment variables as required. (not sure if this makes sense to you @jfmcquade and whether something you might have capacity to do - otherwise we could create a follow-up issue and try to clarify a bit more)

I think a follow up issue with a bit more clarity would be good. I'm happy to look into it but am unlikely to do so immediately

I've added as https://github.com/IDEMSInternational/parenting-app-ui/issues/2264