change merge properties sequence for defaults

[lenisha]

Resolves #2575

What is being addressed

Defaults set by Admins in bundle template definition are overridden by API default template

How is this addressed

• Change order of priority for the template properties

[github-actions]

Unit Test Results

538 tests   538 ✔️  18s ⏱️
    1 suites      0 💤
    1 files        0 ❌

Results for commit 484a4b7.

♻️ This comment has been updated with latest results.

[marrobi]

@lenisha thanks very much for the PR. It think this an appropriate change as a step towards #1695 . There are a couple of linting errors - trailing white space, and also can you increate the api version in api_app/_version.py. Thanks.

[tamirkamara]

Purely from the description - is this something we want given the security risk? We sometimes have settings in schemas stored in the api app that we should never override...

[marrobi]

We had a discussion around this. The concern is template schema could override values set for system reasons in the API schema, this could be a security issue, or cause functionality issues.

1. I'm not sure this is a major issue, as the role publishing/registering the bundles can go edit the API json schema files if they so wish.
2. The alternative is we only allow overwrite of default values. So "if this exists in both, take the default value only from the tempalte_schema.json"

@tamirkamara @stuartleeks is that a reflection of the discussion? Thoughts?

[marrobi]

@tamirkamara @damoodamoo can we look at this again, and clarify the issue why this approach isn't ideal?

[damoodamoo]

I'm fine with this - @lenisha correct me if I'm wrong but as I understand it this just allows a template author to specify a property for in their template and have that override the static json file stored with the API? So I could define my own display_name property, for instance, and use that over the top of the default one?

If so, I think it's good work, and what we need. @tamirkamara not sure where the security risk would come from?

[tamirkamara]

I'm fine with this - @lenisha correct me if I'm wrong but as I understand it this just allows a template author to specify a property for in their template and have that override the static json file stored with the API? So I could define my own display_name property, for instance, and use that over the top of the default one?

If so, I think it's good work, and what we need. @tamirkamara not sure where the security risk would come from?

After another looks and chat, this doesn't change the behavior of user provided values so I don't think there's a risk here.

[marrobi]

/test 068aace

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3397604028 (with refid 20ab61f9)

(in response to this comment from @marrobi)

[marrobi]

/test 3982821

[github-actions]

🤖 pr-bot 🤖

🏃 Running tests: https://github.com/microsoft/AzureTRE/actions/runs/3623248460 (with refid 20ab61f9)

(in response to this comment from @marrobi)

[marrobi]

Recreated PR at #2953

[tamirkamara]

/test-extended 484a4b7

[github-actions]

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/3647741977 (with refid 20ab61f9)

(in response to this comment from @tamirkamara)

[marrobi]

@tamirkamara how did you get this merged in the end?

@lenisha thank you, it's taken a while, but now merged :)

[tamirkamara]

how did you get this merged in the end?

@marrobi The issue was around security of which action is allowed to report UT results. I hardened it a few weeks ago allowing most check just for 1st part actions, but for the UT to support our forks it needs to be "any action" which is how I defined it today.