DYN-9707: Refactor ForceBlockRun to instance #17153
Conversation
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9707
|
|
Hi @ivaylo-matov , I did see this PR and then saw your message on the previous PR. Since you posted the message advertising this PR, I was wondering if it is ready for review then? Thanks. |
|
Hi @jasonstratton , the PR is ready for review. Thanks |
|
I see SelfServe has failed, but I can't see the details. |
jasonstratton
left a comment
jasonstratton
left a comment
There was a problem hiding this comment.
A couple of code changes needed and to test the CLI, but it looks good.
| Assert.IsFalse(string.IsNullOrEmpty(ViewModel.FileTrustViewModel.DynFileDirectoryName)); | ||
|
|
||
| // Arrange - place file B in a dedicated trusted subcategory | ||
| var trustedDir = Path.Combine(TempFolder, trustedDirName); |
There was a problem hiding this comment.
Local var here. I think you should use the instance var
| [TestFixture] | ||
| public class FileTrustWarirngTests : DynamoTestUIBase | ||
| { | ||
| private string trustedDir; |
| [TearDown] | ||
| public void Cleanup() | ||
| { | ||
| if (trustedDir != null) |
There was a problem hiding this comment.
Because the test is using a local var, the instance var is always null and the cleanup does not happen
| /// <summary> | ||
| /// Returns true when grahp execution shoudl be blocked pending file trust aknowledgement | ||
| /// </summary> | ||
| /// <param name="filePath"></param> |
There was a problem hiding this comment.
Nitpick: Typos in the
| namespace DynamoCoreWpfTests | ||
| { | ||
| [TestFixture] | ||
| public class FileTrustWarirngTests : DynamoTestUIBase |
There was a problem hiding this comment.
Class name and file name typo
| /// </summary> | ||
| /// <param name="filePath"></param> | ||
| /// <returns></returns> | ||
| internal bool ShouldForceBlockRun(string filePath) |
There was a problem hiding this comment.
Please test in headless mode.
ShouldForceBlockRun() is called from OpenJsonFile(), which is called in both Dynamo UI and Dynamo headless ... but there is no UI to trust and set homeWorkspace.RunSettings.ForceBlockRun back to false.
Only places to clear homeWorkspace.RunSettings.ForceBlockRun are in the view layer. OpenJsonFile() here is in the DynamoModel.
I think what will end up happening is that if this method returns true in headless mode, it will silently fail to run the graphs.
The precondition for this is: DynamoCLI opens a .dyn where IsTestMode = false and the file's directory is not in the trusted locations list and DisableTrustWarnings = false. ... Is this a realistic scenario? I'm not 100% sure. If so, this needs to be fixed and maybe add some tests.
2 participants
Purpose
This PR is related to DYN-9707 and is a follow-up to this comment.
Changes:
RunSettings.ForceBlockRunfrom static to instance to prevent potential race conditions caused by opening two consecutive.dynfiles from untrusted locations.DynamoViewModelto ensure the file trust warning is dismissed properly. Previously, the warning closed only when the user clicks the Yes/No buttons or closes the untrusted graph. If a user opened a new graph from a trusted location without closing the prior untrusted graph or dismissing its file trust warning, the warning would remain stale and visible, even though the new trusted graph was permitted to execute.Before:
After:
Declarations
Check these if you believe they are true
Release Notes
This PR refactors RunSettings.ForceBlockRun to instance and ensures file trust warning does not ream stale in some edge cases.
Reviewers
@DynamoDS/eidos
@jasonstratton
FYIs
@dnenov
@johnpierson