Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add device endpoints to push/pull files and pull directory. #96

Closed
wants to merge 2 commits into from

Conversation

{
return MissingParameter("path");
}
if (!System.IO.File.Exists(request.Path)) {

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
if (!System.IO.File.Exists(request.Path)) {
return FileNotFound(request.Path);
}
var data = await System.IO.File.ReadAllBytesAsync(request.Path);

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
{
return MissingParameter("path");
}
if (!Directory.Exists(request.Path))

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
.
@aristotelos
Copy link
Collaborator

I am wondering whether this should be implemented in FlaUI.WebDriver directly, or instead in an NPM wrapper library that provides it as Appium driver. For example, https://github.com/appium/appium-windows-driver/blob/master/lib/commands/file-movement.js implements this in the Appium driver, not in the WinAppDriver. Is there a reason to implement this and other Appium commands in FlaUI.WebDriver directly?

@aristotelos
Copy link
Collaborator

aristotelos commented Sep 30, 2024

If we decide to implement this functionality here (see previous comment), the README should be extended as well to document which Appium commands are implemented.

Copy link
Collaborator

@aristotelos aristotelos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution. I have added some questions and minor suggestions.



[Test]
public void PushFile_FileExist_FileIsOverwritten()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void PushFile_FileExist_FileIsOverwritten()
public void PushFile_FileExists_FileIsOverwritten()

}

[Test]
public void PullFolder_FolderDoesNotExists_ErrorRaised()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void PullFolder_FolderDoesNotExists_ErrorRaised()
public void PullFolder_FolderDoesNotExist_ErrorRaised()

{
driver.PushFile(Path.Join(dir, i.ToString()), $"{i} data");
}
var zipBytes = driver.PullFolder(dir);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use newlines to separate arrange/act/assert parts of the test. In this case, surround PullFolder with newlines and remove other newlines. See also other tests.

Suggested change
var zipBytes = driver.PullFolder(dir);
var zipBytes = driver.PullFolder(dir);

{
public class PullFileRequest
{
public string? Path { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing request validation ourselves, wouldn't it be simpler to make the models do that?

Suggested change
public string? Path { get; set; }
public string Path { get; set; }

Comment on lines +5 to +6
public string? Path { get; set; }
public string? Data { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public string? Path { get; set; }
public string? Data { get; set; }
public string Path { get; set; }
public string Data { get; set; }

return MissingParameter("path");
}
var parent = Path.GetDirectoryName(request.Path)!;
if (!Directory.Exists(parent))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add a little bit of security, we could use a "feature flag" mechanism such as appium-windows-driver to only allow this endpoint if it has been enabled, see https://github.com/appium/appium-windows-driver/blob/master/lib/commands/file-movement.js#L18

@bmarroquin
Copy link
Contributor Author

I am wondering whether this should be implemented in FlaUI.WebDriver directly, or instead in an NPM wrapper library that provides it as Appium driver. For example, https://github.com/appium/appium-windows-driver/blob/master/lib/commands/file-movement.js implements this in the Appium driver, not in the WinAppDriver. Is there a reason to implement this and other Appium commands in FlaUI.WebDriver directly?

Interesting, i didn't realize that the Appium server layer added features. I was avoiding running the Appium wrapper because we need to jump through some hoops to get things to run in our Windows test environment, so I wanted to avoid having to configure node.js there. We get more features though so it could be worth the extra work. I will close for now.

Thanks.

@bmarroquin bmarroquin closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants