examind-ai / ltijs-firestore

4 stars 0 forks source link

Integrating ltijs in firebase cloud functions #3

Closed MarcVanDaele90 closed 1 year ago

MarcVanDaele90 commented 2 years ago

I have an EdTech application using a firebase/firestore backend, including some cloud functions.
In my index.js, I already use firestore and hence I already call admin.initializeApp();

I want to add ltijs (with ltijs-firestore) to this setup but when deploying my functions using firebase deploy --only functions I get the error below

Would it be possible to pass admin and db as constructor arguments to Firebase instead of importing them from "./firebase" ?

Error: Error occurred while parsing your function triggers.

Error: The default Firebase app already exists. This means you called initializeApp() more than once without providing an app name as the second argument. In most cases you only need to call initializeApp() once. But if you do want to initialize multiple apps, pass a second argument to initializeApp() to give each app a unique name.
    at FirebaseAppError.FirebaseError [as constructor] (.../functions/node_modules/firebase-admin/lib/utils/error.js:44:28)
    at FirebaseAppError.PrefixedFirebaseError [as constructor] (...functions/node_modules/firebase-admin/lib/utils/error.js:90:28)
    at new FirebaseAppError (.../functions/node_modules/firebase-admin/lib/utils/error.js:125:28)
    at AppStore.initializeApp (.../functions/node_modules/firebase-admin/lib/app/lifecycle.js:42:23)
    at FirebaseNamespaceInternals.initializeApp (.../functions/node_modules/firebase-admin/lib/app/firebase-namespace.js:43:33)
    at FirebaseNamespace.initializeApp (.../functions/node_modules/firebase-admin/lib/app/firebase-namespace.js:311:30)
    at Object.<anonymous> (.../functions/node_modules/@examind/ltijs-firestore/dist/firebase.js:27:7)
    at Module._compile (node:internal/modules/cjs/loader:1099:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
johnnyoshika commented 2 years ago

I think I can fix this by passing in an arbitrary name as a second argument to admin.initializeApp(). That way, ltijs-firestore can create a new instance of an app without colliding with the one you're already using. It's important for ltijs-firestore to use a different app than yours because it requires this setting that may be incompatible with yours. I may need to bump the version when I do this because the way to authenticate the app may need to change (and not rely on the Firebase default GOOGLE_APPLICATION_CREDENTIALS environment variable), thereby making it a breaking change.

While technically I think this can be solved, do you really want to share the same Firestore database between LTIJS and your application? ltijs-firestore creates documents with names that can collide with your application. Even if that doesn't happen now, there's no guarantee that it won't happen in the future. @Cvmcosta (the author of LTIJS) recommends that LTIJS be used as a middleware between your application and the LMS. Something like this diagram where databases are separate and credentials aren't shared:

image

MarcVanDaele90 commented 2 years ago

Thank you for your quick reply and the clear drawing! I have no strong opinion on which one is 'better':

  1. a separate firebase project(middleware) with its own firestore database and its own cloud functions
  2. separate collections in an existing application database (eg with a configurable prefix like "ltijs_" to avoid naming conflicts) Note: Currently I'm also using the "Trigger email extension" ( https://firebase.google.com/docs/extensions/official/firestore-send-email)

    and this also adds a collection to my application database (name of the collection is configurable to avoid collisions)

If we use a separate Firebase project (option 1) with its own firestore, no changes are needed at your side I guess.

If we want to use it inside an existing application, you state that a different app would be needed (due to the ignoreUndefinedProperties setting). (Avoiding name collisions is a valid point as well but can be circumvented by having a configurable prefix for the ltijs collections I guess) I'm not sure I understand what this different app exactly is (I'm no firebase/firestore expert):

If the latter case is true, I see no benefit in this approach.

Unless of course, setting the ignoreUndefinedProperties=false property could be avoided somehow. In this case, the same app could be used and adding ltijs-firestore to an existing application could become straightforward

On Thu, 28 Jul 2022 at 18:22, Johnny Oshika @.***> wrote:

I think I can fix this by passing in an arbitrary name as a second argument to admin.initializeApp(). That way, ltijs-firestore can create a new instance of an app without colliding with the one you're already using. It's important for ltijs-firestore to use a different app than yours because it requires this setting https://github.com/examind-ai/ltijs-firestore/blob/main/src/firebase.ts#L9 that may be incompatible with yours. I may need to bump the version when I do this because the way to authenticate the app may need to change (and not rely on the Firebase default GOOGLE_APPLICATION_CREDENTIALS environment variable), thereby making it a breaking change.

While technically I think this can be solved, do you really want to share the same Firestore database between LTIJS and your application? ltijs-firestore creates documents with names that can collide with your application. Even if that doesn't happen now, there's no guarantee that it won't happen in the future. @Cvmcosta https://github.com/Cvmcosta (the author of LTIJS) recommends that LTIJS be used as a middleware between your application and the LMS. Something like this diagram where databases are separate and credentials aren't shared:

[image: image] https://user-images.githubusercontent.com/504505/181588499-6b204e6e-fdc4-44b5-9198-d8805d0c3781.png

— Reply to this email directly, view it on GitHub https://github.com/examind-ai/ltijs-firestore/issues/3#issuecomment-1198368085, or unsubscribe https://github.com/notifications/unsubscribe-auth/AVQAGKWYWWSWSGTT5K6B5WLVWKXURANCNFSM544OQSDA . You are receiving this because you authored the thread.Message ID: @.***>

johnnyoshika commented 2 years ago

Generally speaking, it's best to follow the principle of least privilege. As the ltijs-firestore package doesn't need access to your production database, it would be preferrable to not give it the credentials to do so. Having said that, if you still want to use a common database, I can modify this library to support it.

Firebase Admin SDK's multiple app architecture allows you to call initializeApp() multiple times to create multiple instances. Every time you register an app there, it stores it in the admin.apps property. It's just a way to separate instances and configurations. You can use this strategy to access multiple Firebase projects or the same one. For an independent library that doesn't need to share instances of this app with your consuming code, it would be cleaner to create a separate app and not reach in and use the consuming code's app.

I'll try to work on this enhancement in the next few days.

johnnyoshika commented 2 years ago

@scottgeleas Have you tried testing ltijs with their sample demo-server? I found that that's the best way to get started.

Assuming you've already got something like the demo-server working, to use LTIJS as a middleware, you need to set ltiaas to true during lti.setup(). This is my setup:

lti.setup(
  process.env.LTI_KEY,
  {
    plugin: new Firestore(),
  },
  {
    cookies: {
      secure: true, // Set secure to true if the testing platform is in a different domain and https is being used
      sameSite: 'None', // Set sameSite to 'None' if the testing platform is in a different domain and https is being used
    },
    devMode: false,
    ltiaas: true, // Disables cookie validation (set this to true when using LTIJS as a middleware)
    dynReg: {
      url: `${process.env.SERVER_URL}/`,
      name: 'COMPANY NAME',
      logo: 'https://url-to-logo.com',
      description: 'LTI integration',
      autoActivate: false, // Whether or not dynamically registered Platforms should be automatically activated. Defaults to false.
    },
  },
);

If you refer to my image above, I have ltiServer set up in a Google Cloud Function, but any server will do. The ltijs-firestore library is particularly well suited to a serverless environment, whereas the default database connector for ltijs (Mongoose for MongoDB) didn't work for me in a serverless environment, as the connection would get lost every time my cloud function woke up from being asleep.

My ltiServer has its own url (e.g. https://lti.myapp.com). The communication with the LMS all happens from ltiServer. ltiServer will also route the user to my main application during LTI Launch via this code:

lti.onConnect(async (token, req, res) =>
  lti.redirect(res, 'https://main.myapp.com'),
);

LTIJS will pass along an ltik query string parameter to the redirect URL you specify above, so your client app can now include that ltik value when sending LTI requests to your main server. Your main server will then act as a proxy and use that ltik when making requests to the ltiServer. When doing so, add this HTTP header:

Authorization: LTIK-AUTH-V1 Token={ltik},Additional=Bearer {your_secret}

Where {ltik} is the ltik sent from your client and {your_secret} is any secret that you want to pass to ltiServer. When LTIJS parses your Authorization header, it'll validate ltik and replace req.headers.authorization with this:

Bearer {your_secret}

That way, you can use req.headers.authorization to validate that the request came from a reliable source. More info on this authorization header is available here: https://cvmcosta.me/ltijs/#/provider?id=bearer-authorization-header

So the communication between your client and ltiServer will look like this:

image

I hope that gets your started.

In terms of your other question re: storing documents in a collection, can you elaborate with some examples? Do you mean storing them as a sub-collection of a document?

johnnyoshika commented 2 years ago

Even if I don't use ltiaas I still set it as true?

Yes, if you don't set ltiaas to true, client requests to your LTI Server will be rejected unless it contains a cookie that's set by LTIJS. While that's the correct behavior in the sample demo-server, in a server-to-server communication environment where you have LTI Server set up as a middleware, such cookie won't exist.

As for my other question: My DB Admin wants all the collections created by the LTI Tool to go under one mother collection (or similar) in our Firestore. Instead of on the "root" firestore which will make everything a bit messy

Do you mean something like this?

lti/index/accsesstoken
lti/index/platforms
lti/index/nonce

...where lti is an example collection name and index is an example document ID?

Not sure if this is possible with firestone as I'm new to it

The example I showed above is possible and I can support it once I implement the configurable prefix for @MarcVanDaele90.

or if we are better off making a new firestore for only the LTI stuff

I think it's better to create a new Firebase project for ltijs-firestore. I don't see any downside to doing that other than the extra 10 minutes or so to create a new project. It's pretty much set and forget. But I understand that different developers have different needs, so I'll make an enhancement to support sharing Firestore databases.

johnnyoshika commented 2 years ago

@MarcVanDaele90 @scottgeleas

I just published beta version 1.0.0-beta.2 of ltijs-firestore. Can you please test it? Once you confirm that it works as intended, I'll release it to the main stream.

Documentation has been updated here to reflect the changes: https://github.com/examind-ai/ltijs-firestore/tree/beta

Some notable changes:

import { Firestore } from '@examind/ltijs-firestore';

OR

const { Firestore } = require('@examind/ltijs-firestore');
MarcVanDaele90 commented 2 years ago

I certainly want to test 1.0.0-beta.2 but can you give a hint on how to use this version? I currently have "@examind/ltijs-firestore": "^0.1.7", in my package.json

Note however that I followed your advice and I now use a separate Firebase project so I can't test all changes. The separate Firebase project does not work 100% yet (see https://github.com/Cvmcosta/ltijs/issues/157) but I'm getting close. A separate Firebase project is indeed pretty elegant.

johnnyoshika commented 2 years ago

I certainly want to test 1.0.0-beta.2 but can you give a hint on how to use this version?

You can install the beta version like this:

npm install @examind/ltijs-firestore@1.0.0-beta.2
MarcVanDaele90 commented 2 years ago

using 1.0.0-beta.2 works fine as far as I'm concerned (though, as said before, I couldn't test all the new features anymore)

scottgeleas commented 2 years ago

Using the beta works fine for me, I basically only tested the prefix functionality. Works well.

johnnyoshika commented 1 year ago

Issues discussed here were addressed in version 1.0.0