Cvmcosta / ltijs

Turn your application into a fully integratable LTI 1.3 tool provider.
https://cvmcosta.github.io/ltijs/
Apache License 2.0
299 stars 67 forks source link

Dynamic Registration options can't be overriden #144

Closed pogotc closed 5 months ago

pogotc commented 2 years ago

Describe the bug I'm hooking up dynamic registration and I'd like to have a custom field set that's specific to the installation, I noticed in the register function there's an option parameter:

https://github.com/Cvmcosta/ltijs/blob/a89346f5d81732e7902854f3ad1c2ce4bc4acbe9/src/Provider/Services/DynamicRegistration.js#L84-L90

The comment for that sounds like exactly what I need so I can set my custom fields by setting customParameters here rather than in lti.setup, but options isn't referenced anywhere in that function.

Expected behavior Values passed into options should override the registration defaults.

Ltijs version

Cvmcosta commented 2 years ago

Hello @pogotc, this was an oversight on my end but it doesn't impact the functionality (leftover from previous way of doing what you want). There's already a way to customize this flow as described here: https://cvmcosta.me/ltijs/#/dynamicregistration?id=custom-flow

Please let me know if this is what you are looking for.

pogotc commented 2 years ago

Hey, thanks for the response. That's the flow I'm using, but it's that call to the register function where I'd like to pass in some extra parameters that are specific to the installation, in particular a query string parameter in the url. So I'm hoping to do something like this:

lti.onDynamicRegistration(async (req, res, next) => {
    const registrationKey = req.query.rkey;

    const message = await lti.DynamicRegistration.register(
        req.query.openid_configuration, 
        req.query.registration_token,
        {
          custom_parameters: { registrationKey }
        }
    );

    res.setHeader('Content-type', 'text/html')
    res.send(message)

}

Having that registration key in the custom parameters of the tool would help us with part of the LTI launch workflow and tying it back to a registration. In our case we need it because our tool registration process is a little complicated and requires a few fields we don't get during registration, but do get during the LTI launch, so being able to link the two would be quite helpful.

Because the registration key is specific to a customer we can't set it in the setup function, that's why when I noticed the options parameter I thought it would help be do this.

pogotc commented 2 years ago

Just wanted to check back in on this, if supporting overrides like this is something you'd be happy to go into the library I could look at creating a pull request to add it? I just wouldn't want to do it if was something you were against the library doing or would prefer it done a different way.

rscott78 commented 2 years ago

I noticed this also, and I have the exact same scenario that I need to accomplish. Did you come up with anything @pogotc ?

Cvmcosta commented 2 years ago

@pogotc Sure! I'd be great if you could create a PR for this functionality. It's something i want to add but haven't had the time to do proper development on this. Reviewing a PR would be a lot quicker so i could get it done asap.

cxn-mkalinovits commented 2 years ago

Hi, I would like to set the domain dynamically for eliminating addition of one more environment specific configuration to the system.

I expected that the above suggested flow customization could help. Is there any way to do this instead of creating a PR on fixing options behaviour (that I'm willing to do)?

Thanks

Cvmcosta commented 5 months ago

https://github.com/Cvmcosta/ltijs/pull/217 Is now released, so it's possible to override options when using a custom flow:

lti.onDynamicRegistration(async (req, res, next) => {
  try {
    if (!req.query.openid_configuration) return res.status(400).send({ status: 400, error: 'Bad Request', details: { message: 'Missing parameter: "openid_configuration".' } })
    const message = await lti.DynamicRegistration.register(req.query.openid_configuration, req.query.registration_token, {
      'https://purl.imsglobal.org/spec/lti-tool-configuration': {
        custom_parameters: {
          'custom1': 'value1',
          'custom2': 'value2'
        }
      }
    })
    res.setHeader('Content-type', 'text/html')
    res.send(message)
  } catch (err) {
    if (err.message === 'PLATFORM_ALREADY_REGISTERED') return res.status(403).send({ status: 403, error: 'Forbidden', details: { message: 'Platform already registered.' } })
    return res.status(500).send({ status: 500, error: 'Internal Server Error', details: { message: err.message } })
  }
})