Load Activity UI from URL in ActivitySpec

[avaidyam]

Currently the HTML code for an Activity's UI is loaded statically by LAMP-dashboard like so:

let activityURL = "https://raw.githubusercontent.com/BIDMCDigitalPsychiatry/LAMP-activities/"
activityURL += process.env.REACT_APP_GIT_SHA === "dev" ? "dist/out" : "latest/out"

While we require this to still work as a fallback for older dashboard and server versions, the proper way of checking the URL in EmbeddedActivity should be:

let activity = ... // the activity object we need to load as a UI
let activitySpec = LAMP.ActivitySpec.view(activity.spec) // activity.spec === "lamp.tips" for example
// We use ActivitySpec.view() to return the SINGLE ActivitySpec object WITH all image/description/code data.
if (activitySpec?.executable?.startsWith("data:")) {
    iframe.srcdoc = atob(activitySpec.executable) // TODO strip "data:" URI prefix
} else if (activitySpec?.executable?.startsWith("https:")) {
    iframe.srcdoc = atob(await (await fetch(activitySpec.executable)).text())
} else if(fallbackURLs.includes(activitySpec?.executable ?? "")) {
    iframe.srcdoc =  atob(await (await fetch(fallbackURLs[activitySpec.executable])).text()) 
} else {
    iframe.srcdoc = "about:blank" // display an error dialog instead
}

This way, each individual API Server controls the code location. If a staging server requires different versions of the activities, the dashboard will load only what the staging server provides for URLs. Production servers should ideally be loading the code into the ActivitySpec object itself.

[sarithapillai8]

@avaidyam Will the activityspec 'executable' field be updated after each build automatically or do we need to do anything for that?

[avaidyam]

The activitySpec.executable field is controlled by the server. For the BIDMC staging and production servers, it will either be an automated rollout or I will personally update it manually.

[sarithapillai8]

@avaidyam Could you explain what are the possible values for fallbackURLs?

[avaidyam]

It's a dictionary (aka object in JS) containing the activity specs we currently have mapped to the URLs we currently have - for production.

[sarithapillai8]

@avaidyam So what should I use for comparison? I mean with which values I can initialize that variable?

[avaidyam]

The ones that are already in EmbeddedActivity...? There's already a mapping of activity specs, and currently we're deriving the URL using a function - just hard-code these URLs.

[sarithapillai8]

Do you mean https://raw.githubusercontent.com/BIDMCDigitalPsychiatry/LAMP-activities/dist/ files?

[avaidyam]

Yes, exactly.

[sarithapillai8]

Server side/LAAMP-js changes are required. Currently ActivitySpec Object is not contained with executable field.

[sarithapillai8]

https://github.com/BIDMCDigitalPsychiatry/LAMP-js/pull/4 https://github.com/BIDMCDigitalPsychiatry/LAMP-server/pull/175 https://github.com/BIDMCDigitalPsychiatry/LAMP-dashboard/pull/495 @avaidyam Could you please check and let me know these are the changes expected.

[avaidyam]

@sarithapillai8 It still looks incomplete here. Please ensure you are calling LAMP.ActivitySpec.view() to get the full contents of the spec including the executable field, and that LAMP.ActivitySpec.list() does NOT return the executable field. Also please make sure the TODO is completed. The code I provided was pseudocode, not the implementation itself.

[sarithapillai8]

@avaidyam We have added the executable changes in LAMP-server as said. And I didn't use the exact code given, but missed to do the data strip. Todo is completed and updated. https://github.com/BIDMCDigitalPsychiatry/LAMP-server/pull/175/files

[sarithapillai8]

Waiting for LAMP-server and LAMP-js

[avaidyam]

@sarithapillai8 Have you or @Linoy339 tested these two PRs? If so I can merge them, since they are minor and do not change much. Please double check that LAMP-server does not return the executable field when calling ActivitySpec.list(), and it does so only on view().

[sarithapillai8]

@avaidyam We have tested. Could you please update LAMP-server and LAMP-js? I can update package.json for lamp-core update in LAMP-dashboard repo for QA.

[avaidyam]

@sarithapillai8 This is complete. Please test the LAMP-dashboard implementation of this issue with both the staging and production servers.

[sarithapillai8]

@avaidyam Do we have any executable data in production/staging database?

[avaidyam]

@sarithapillai8 Not at the moment, but please try modifying the executable data using the API on the staging server to confirm it's implemented correctly on the backend too.

[Linoy339]

@avaidyam. As participants do not have permission to access the activity_spec resource, the data(executable) cannot be retrieved for the particular participant.

i.e : https://api-staging.lamp.digital/activity_spec/lamp.scratch_image will give "403.security-context-out-of-scope" for participant.

Can we have change of adding "me"in https://github.com/BIDMCDigitalPsychiatry/LAMP-server/blob/master/src/service/ActivitySpecService.ts#L24 as third parameter ?

Can you suggest?

[avaidyam]

As participants do not have permission to access the activity_spec resource, the data(executable) cannot be retrieved for the particular participant.

That's a good point and a security over-complication. We should be able to change that such that any logged-in user is able to see (but not edit/create) the ActivitySpecs.

Can we have change of adding "me" as third parameter?

There's no me option for the _verify function - only self, sibling, or parent. For example:

activity_id = await _verify(auth, ["self", "sibling", "parent"], activity_id)

This uses the current auth from the request, saying "ensure that it is the Activity itself, a sibling Activity, or a parent of the Activity" and provides the activity_id. There's a reassignment of activity_id here to allow me as a dynamic parameter, but this is not relevant to the current issue. Another example, to restrict the API method to only root-users:

const _ = await _verify(auth, [])

However, In ActivitySpec we have:

const _ = await _verify(auth, ["self", "sibling", "parent"])

So there's no authObject to verify against in _verify, so we hit the branch at line 57. This is because the call _verify(auth, ["self", "sibling", "parent"]) is actually _verify(auth, ["self", "sibling", "parent"], undefined), since we didn't specify that third parameter at all.

I think here, the only remaining possibility is that _verify is checking "is the auth subject (i.e. the Participant requesting the ActivitySpec) the owner of the ActivitySpec?" but runs into the issue that ActivitySpecs do not have owners as they are not resources in the API (like Researcher, Activity, Study, Participant). So this _verify function needs to be modified to understand _verify(auth, ["self", "sibling", "parent"], undefined) to mean "is the request authenticated at all as a valid resource?".

[Linoy339]

@avaidyam . Thank you for the good explanation. As the ActivitySpecs do not have owners, this becomes difficult to handle. Can we have the idea of keeping the valid resource in authTypearray itself? i.e, authType in _verify function would need to be tweaked to accept type specs

Thus, we can pass new type specs to _verifyfunction here. i.e, const _ = await _verify(auth, ["self", "sibling", "parent","specs"])

As a final step, the following should be added here: if (!isRoot && ((authType.includes("self") && (authSubject.origin === authObject)) || (authType.includes("specs"))) )

[avaidyam]

@Linoy339 I agree with your strategy for the most part, but instead of introducing a new specs authType, I think we should use this to handle these cases:

JSON.stringify(authType) === JSON.stringify(["self", "sibling", "parent"]) && authObject === undefined

Right now this specific condition is only provided by ActivitySpec and SensorSpec get/list functions, so that should work.

(Please note that we can't use authType == ["self", "sibling", "parent"] as part of the condition since array equality doesn't work with ==. Instead we need to stringify both arrays to compare - but this is order specific and will fail if authType === ["parent", "sibling", "self"], for example.)

[Linoy339]

@avaidyam .Thank you. Can you please check this: https://github.com/BIDMCDigitalPsychiatry/LAMP-server/pull/178

[avaidyam]

Looks good! Please do test it in Zco staging and Zco production (BIDMC staging) to double check though!

[Linoy339]

Yes. We will do that and let you know

[Linoy339]

@avaidyam . This is tested and found working.

[toujames]

Hello,

Continue from the discourse post.

Here's contents of the activity_spec document in my database:

{
  "_id": "org.laureateinstitute.test",
  "_rev": "1-f0730beaa31719cefe13befeaf5f282a",
  "help_contents": null,
  "script_contents": null,
  "static_data_schema": {},
  "temporal_slice_schema": {},
  "settings_schema": {},
  "category": null,
  "executable": "https://raw.githubusercontent.com/toujames/react-test/main/dist.html.b64"
}

In dashboard, I do not see as a possible activity. Only the demo activities. Attached is the screenshot.

I'm using the dashboard on dashboard.lamp.digital i'm hosting my own server at api.lamp-dev.laureateinstitute.org. The version i used is the docker image: ghcr.io/bidmcdigitalpsychiatry/lamp-server:2022

[toujames]

I've also tried using ghcr.io/bidmcdigitalpsychiatry/lamp-server:latest. Still no task being populated.

I'm also attaching the developer console log I noted. For the test activity_spec, executable is not showing. Although I'm not sure if it's's supposed to.

[avaidyam]

@toujames The executable field won't show up in list() function to minimize excessive data transfer. But it should be showing up in the dashboard. @sarithapillai8 Do you know why it wouldn't?

[sarithapillai8]

@avaidyam We are limiting the activity spec to control the activities on patient side with the implemented ones. We were updating the list with implementing each activity. Do you want to allow all the activity spec on the list to create activities?

[avaidyam]

We were updating the list with implementing each activity.

@sarithapillai8 I think we need to remove this artificial limit. The list of ActivitySpec objects MUST come from the server ONLY.

I realize right now we've hardcoded the JSONSchema for configuration. Even this must be part of the ActivitySpec; only when lamp.survey doesn't have a provided settings field should we use the internal fallback schema that is part of the LAMP-dashboard codebase.

(Please keep in mind WE WILL NOT break legacy compatibility for organizations that have not upgraded their LAMP server, so this fallback path MUST be implemented and confirmed working with older server versions.)

[sarithapillai8]

@avaidyam We are using ActivitySpec, but limiting the list. https://github.com/BIDMCDigitalPsychiatry/LAMP-dashboard/blob/84872f53758b3c5176b6d9e5af317de209648f89/src/components/Researcher/ActivityList/AddActivity.tsx#L133 So can you confirm to remove this condition and use the entire list coming from the server? Can we just show the popup saying 'Unimplemented', if no executable value found for the activity created with any spec which are out of the current list? If there is any executable value, that will be loaded for the activity in the patient side. But if there is any issue with communicating through the parent window/or any loading issues, we won't be able to handle such kind of bugs. We can do exception handling in that case and notify the user with some popup. I hope this is fine.

This is still not a bug, we haven't considered this requirement before. So please consider this as feature. //cc @michaelmenon @ZCOEngineer

[avaidyam]

So can you confirm to remove this condition and use the entire list coming from the server?

Yes, use the list from the server, and filter each spec:

1. If spec has no executable field OR settings field:
   1. If spec belongs to internal fallback list:
      1. Substitute missing executable field using fallback url.
      2. Substitute missing settings field using fallback settings.
   2. Otherwise:
      1. Disable the spec (hide it from menus/show an error when loading).
2. Otherwise:
   1. Enable the spec (show in new activity menu/load embedded activity normally).

But if there is any issue with communicating through the parent window/or any loading issues, we won't be able to handle such kind of bugs.

@sarithapillai8 I don't understand what you mean by this? What would be different enough to prevent us from handling such bugs?

We can do exception handling in that case and notify the user with some popup.

This should already be implemented across the entire codebase. Exceptions cannot be ignored and must be caught and logged/displayed to the user.

[sarithapillai8]

@avaidyam

I understood the way of implementation. Thanks for the explanation.

I don't understand what you mean by this? What would be different enough to prevent us from handling such bugs?

How can we ensure the executable value added by the client will work as expected? Currently we are loading the activities that are developed by our team. So we have control over that.

This should already be implemented across the entire codebase. Exceptions cannot be ignored and must be caught and logged/displayed to the user.

Error handling is already there. But I told about specifically handling such exceptions with some popup if the client is using an executable value for a new activity spec which is out of our control.

Please correct me if anything wrongly interpreted.

[avaidyam]

How can we ensure the executable value added by the client will work as expected?

We won't develop or support any third-party ActivitySpecs - that's for the third party developer to diagnose and fix. We just need to maintain the JSONSchema/IFrame/PostMessage protocol we use to embed activities and support only that.

But I told about specifically handling such exceptions with some popup if the client is using an executable value for a new activity spec which is out of our control.

That's completely fine, we should ignore those situations for now until better embedded activity error handling is required in the future.

[sarithapillai8]

@avaidyam

If spec has no executable field OR settings field: If spec belongs to internal fallback list: Substitute missing executable field using fallback url. Substitute missing settings field using fallback settings.

Sorry that I missed something here. ActivitySpec settings have no role in loading activity actually. If there are no settings, the activity created with name, group, streak details and tab(category). Why should we consider settings here? (Journal activity having no settings. ) So can we just filter through the executable field and fallback URL option?

[avaidyam]

Why should we consider settings here?

Because legacy servers will have an ActivitySpec with id = lamp.survey, executable = null, and settings = null. If you don't handle this case, clinicians using the older server version will try to create a survey and it won't let them configure a survey.

activity created with name, group, streak details and tab(category).

Here, group really refers to the _parent of the Activity that is created, and name and category are official properties of the Activity object. Streak details are not a formalized part of the API but we do need to document how this is configured and stored. Are you handling the description and icon field (which may need to become official properties of the Activity object) as well?

So can we just filter through the executable field and fallback URL option?

If you are opening an activity, yes. For configuring an activity you do need to consider the settings field as I described above.

[sarithapillai8]

Because legacy servers will have an ActivitySpec with id = lamp.survey, executable = null, and settings = null. If you don't handle this case, clinicians using the older server version will try to create a survey and it won't let them configure a survey.

Ok Got it. So it can be do like either the executable or the settings field should not be null.

Are you handling the description and icon field (which may need to become official properties of the Activity object) as well?

No they are still part saved as tag attachment. Do you want to change it now?

[avaidyam]

So it can be do like either the executable or the settings field should not be null.

The settings field can be null/empty ({}). It is just that we need to check that if we "own" the activity, such as lamp.activity, and the settings is null/empty, check to see if we have a hard-coded version of the settings (i.e. what we do right now).

No they are still part saved as tag attachment. Do you want to change it now?

Not right now - I just wanted to confirm this. //cc @lukeoftheshire Can you make a new GitHub issue about documenting the Tag that has these two parameters & streak configuration? Thanks!

[sarithapillai8]

The settings field can be null/empty ({}). It is just that we need to check that if we "own" the activity, such as lamp.activity, and the settings is null/empty, check to see if we have a hard-coded version of the settings (i.e. what we do right now).

This is not clear to me. Can you confirm on this :

While filtering activity spec list(for creating an activity), we just need to filter the activity spec with executable and fallback URL. Settings are part of activity object, not on activity spec, right? Below is the activity spec object for lamp.survey:

{
    "id": "lamp.survey",
    "_parent": null,
    "_rev": "1-967a00dff5e02add41819138abb3284d",
    "category": null,
    "executable": "https://raw.githubusercontent.com/BIDMCDigitalPsychiatry/LAMP-activities/dist/out/survey.html.b64",
    "help_contents": null,
    "script_contents": null,
    "settings_schema": null,
    "static_data_schema": null,
    "temporal_slice_schema": null
}

And for listing activity on patient side, the activities are to be filtered with settings and executable field, right?

Currently we are actually having hard-coded settings, the settings are loaded through rjsf JSON schema.

[avaidyam]

Settings are part of activity object, not on activity spec, right?

I'm referring to settings_schema which needs to be renamed to settings. (script_contents was renamed to executable, help_contents to description, and all the _schema suffixes are dropped.)

In this case, the dashboard sees settings_schema == null and it will check fallbackSettings["lamp.survey"] = { ... the JSON Schema object } and use that instead.

[sarithapillai8]

@avaidyam

I'm referring to settings_schema which needs to be renamed to settings. (script_contents was renamed to executable, help_contents to description, and all the _schema suffixes are dropped.)

The above data provided for lamp.survey is from api-staging server. In the production, we are getting the below data :

{
    "id": "lamp.survey",
    "_parent": null
}

There is no settings_schema or executable fields. And another spec is like below :

{
    "id": "lamp.recording",
    "help_contents": null,
    "script_contents": null,
    "static_data_schema": {},
    "temporal_slice_schema": {},
    "settings_schema": {}
}

I could not see the renaming done for the parameters. Could you confirm?

[avaidyam]

Yes, this is probably because 2-3 years ago, the specs were loaded in incorrectly for the staging server. That's fine - just ignore the incorrectly named fields and assume you will have properly named ones according to my prior comment. I or @lukeoftheshire will later go through and update the specs on both servers and advise other collaborators to do the same for their servers.

[sarithapillai8]

@avaidyam I cannot use settings parameter name now, as this is not available in ActivitySpec object and it is showing error. So can I use settingsSchema for now?

[avaidyam]

No. Please update LAMP-js instead to add these parameters properly to the type definitions. I'll make the release once the PR is merged.

[sarithapillai8]

Ok Thanks @avaidyam

[sarithapillai8]

@avaidyam I have merged the PRs. Once the field changes are ready in database and released LAMP-js and LAMP-server, we can update LAMP-dashboard. Development is completed. - https://github.com/BIDMCDigitalPsychiatry/LAMP-dashboard/pull/561

[sarithapillai8]

@avaidyam QA is completed for this update along with #542 changes in our zco staging. Could you please confirm, whether we can update this in dashboard-staging for a final round testing tomorrow morning? Or do we need to hold it for production release? //cc @michaelmenon @ZCOEngineer

[avaidyam]

Please do update this for staging. It needs to work in staging and in production separately. It needs to be tested in the following combinations:

1. Staging dashboard + staging server
2. Production dashboard + staging server
3. Staging dashboard + production server (please use both the latest production version AND an older :2021 server)

[sarithapillai8]

@avaidyam We have updated the changes in dashboard-staging. We created different activity specs with executable/settings values. If the new spec is not having any of these, then it won't be listed for creating activity.

Note : Settings for this new activity spec need to added in similar way of JSON schema that supports rjsf format.