MarketSquare / robotframework-browser

Robot Framework Browser library powered by Playwright.
Apache License 2.0
530 stars 106 forks source link

Initial support for new_context clientCertificates #3859

Closed okraus-ari closed 2 weeks ago

okraus-ari commented 1 month ago

I would like to contribute support for authentication with client certificates, added in Playwright version 1.46:

https://playwright.dev/docs/release-notes#version-146

Initial version of this functionality covers only adding certificates with keyword New Context:

New Context    clientCertificates=[{'origin': 'https://playwright.dev', 'pfxPath': 'certificate.p12', 'passphrase': 'password'}]

As I am pretty new to Python and Robot framework, I did not cover the implementation with tests. I would probably need some help with that.

Fixes #3860

aaltat commented 1 month ago

Hi You are off with good start. The https://github.com/MarketSquare/robotframework-browser/blob/main/CONTRIBUTING.md has information how to setup your development environment if you have not done that already, invoke to perform actions, like invoke lint or ìnvoke atest. Also you PR is in the right course, you added the argument in the right place.

Few notes

  1. We document arguments also in the keyword documentation so you should add short description in there too.
  2. Node side should be OK, unless testing reveal something else.
  3. Acceptance test is needed, most likely somewhere here

Writing test is tricky, because in our test app we do not have support for this one (I think, I did not look into it that deeply.) At least there should be test which uses the argument. But it would be really nice if you could do modifications on the test app to use the certificate.

aaltat commented 1 month ago

Also look at the CI failure, it seems that you should run invoke lint to fix the problem and commit/push again.

aaltat commented 1 month ago

@allcontributors please add @okraus-ari for code.

allcontributors[bot] commented 1 month ago

@aaltat

I've put up a pull request to add @okraus-ari! :tada:

okraus-ari commented 1 month ago

Ok, I have added the documentation and did some tidying, all tests passed.

Would you like to go forward without the tests or should we cover the case? I think this should not be that complicated if we can agree on adding a certificate just for the client and omitting mTLS authentication part on the server side. That would be quite complicated, I think.

aaltat commented 1 month ago

Yes, we can go forward with just the clint side tests.

aaltat commented 1 month ago

Really nice work on writing that test.

aaltat commented 1 month ago

There is small linting problem with test you wrote. Running invoke lint and commit + push should fixit.

aaltat commented 1 month ago

Attached logs because there seems to be some sort of an error in the new tests ubuntu-latest 3.11 20.x Clean install results.zip

okraus-ari commented 1 month ago

I'm sorry, I have missed your comments, it was work in progress.

In the end I had to drop support for encrypted private keys as I was not able to convince NodeJS to accept the passphrase. I think that it should be ok to stick with plain private keys for now and we can try to add some encryption afterwards.

There is also one problem, I might have broken something. I had to strenghten checks in stop_test_server() and now some other tests started to fail. Can you look into it? I can of course move my code to some other function, but there is a chance, that these problems were previously hidden.

One last thing, I tried to set ${HTTPS_SERVER_PORT} the same way ${SERVER_PORT} is set in init.robot, but it did not work for me and sticked to the default value in variables.resource. Am I missing something?

aaltat commented 1 month ago

At least one run had some unrelated error. Rerunning.

aaltat commented 1 month ago

Looks like Windows file paths are problematic: windows-latest 3.11 18.x Clean install results.zip

And for Mac there some other problem: Test results-macos-latest-1-3.11-20.x.zip

aaltat commented 1 month ago

Linux failure: ubuntu-latest 3.11 20.x Clean install results.zip

Windows failure Test results-windows-latest-1-3.10-18.x.zip

okraus-ari commented 1 month ago

It looks like a bug in Robot Framework, in Windows log you can see that this path:

'D:\a\robotframework-browser\robotframework-browser\atest\output\pabot_results\0/client.crt'

gets translated into:

'D:\x07\robotframework-browser\robotframework-browser\x07test\\output\\pabot_results\x00/client.crt'
aaltat commented 1 month ago

Well, you could add logging in Python and Node side, in various places to see where path is converted incorrectly.

aaltat commented 3 weeks ago

Hi, are you still interested to investigate why this fails?

okraus-ari commented 3 weeks ago

Hello, definitely. Unfortunately I did not have time this week. I have a little bit struggled to setup windows environment, invoke build fails. I hope there will be more time next week to finally finish this.

okraus-ari commented 3 weeks ago

@aaltat I am unable to make it work, is there something to be aware of? inv build fails with:

> inv build     
{project_dir}\.venv\Lib\site-packages\rellu\labels.py:75: SyntaxWarning: invalid escape sequence '\s'
  tokens = re.split('\s{2,}', line)
Requirement already satisfied: pip in {project_dir}\.venv\lib\site-packages (24.3.1)
Requirement already satisfied: uv in {project_dir}\.venv\lib\site-packages (0.4.29)
Audited 32 packages in 33ms
no changes in package-lock.json, skipping npm install
Writing mypy to playwright_pb2.pyi

> robotframework-playwright@18.9.1 grpc_tools_node_protoc
> grpc_tools_node_protoc --js_out=import_style=commonjs,binary:{project_dir}\node\playwright-wrapper\generated --grpc_out=grpc_js:{project_dir}\node\playwright-wrapper\generated --plugin=protoc-gen-grpc={project_dir}\node_modules\.bin\grpc_tools_node_protoc_plugin.cmd -I ./protobuf protobuf/*.proto

{project_dir}\node_modules\grpc-tools\bin\protoc.js:41
    throw error;
    ^

Error: Command failed: {project_dir}\node_modules\grpc-tools\bin\protoc.exe --plugin=protoc-gen-grpc={project_dir}\node_modules\grpc-tools\bin\grpc_node_plugin.exe --js_out=import_style=commonjs,binary:{project_dir}\node\playwright-wrapper\generated --grpc_out=grpc_js:{project_dir}\node\playwright-wrapper\generated --plugin=protoc-gen-grpc={project_dir}\node_modules\.bin\grpc_tools_node_protoc_plugin.cmd -I ./protobuf protobuf/*.proto

    at genericNodeError (node:internal/errors:983:15)
    at wrappedFn (node:internal/errors:537:14)
    at ChildProcess.exithandler (node:child_process:421:12)
    at ChildProcess.emit (node:events:519:28)
    at maybeClose (node:internal/child_process:1104:16)
    at ChildProcess._handle.onexit (node:internal/child_process:304:5) {
  code: 3221225781,
  killed: false,
  signal: null,
  cmd: '{project_dir}\\node_modules\\grpc-tools\\bin\\protoc.exe --plugin=protoc-gen-grpc={project_dir}\\node_modules\\grpc-tools\\bin\\grpc_node_plugin.exe --js_out=import_style=commonjs,binary:{project_dir}\\node\\playwright-wrapper\\generated --grpc_out=grpc_js:{project_dir}\\node\\playwright-wrapper\\generated --plugin=protoc-gen-grpc={project_dir}\\node_modules\\.bin\\grpc_tools_node_protoc_plugin.cmd -I ./protobuf protobuf/*.proto'
}

Windows 11 Node.js v22.9.0 Python 3.12.7

Am I missing something?

aaltat commented 3 weeks ago

Darn, I don’t like windows. Because I don’t have it available I am also guessing.

Can you try to run the command from cmd:

{project_dir}\node_modules\grpc-tools\bin\protoc.exe --plugin=protoc-gen-grpc={project_dir}\node_modules\grpc-tools\bin\grpc_node_plugin.exe --js_out=import_style=commonjs,binary:{project_dir}\node\playwright-wrapper\generated --grpc_out=grpc_js:{project_dir}\node\playwright-wrapper\generated --plugin=protoc-gen-grpc={project_dir}\node_modules.bin\grpc_tools_node_protoc_plugin.cmd -I ./protobuf protobuf/*.proto

But could you try convert the / to windows \ and see what it says. I am mobile, so copy pasting and making sure that full command is correctly copied is challenging, so please use output of your console.

okraus-ari commented 3 weeks ago

Interesting, that exe tool does nothing. Ignores all command line options, no output, no dump, no error, just immediately finishes without a sign of any activity. Should I upgrade some package?

aaltat commented 3 weeks ago

There should not be updated needed, if you have the latest packages. The inv build is also used in our CI with windows and same command does work: https://github.com/MarketSquare/robotframework-browser/actions/runs/11561256048/job/32182338162?pr=3859 Deps are installed little bit differently:

image But in practice invoke runs similar command behind the scenes.

What happens in you run: \node_modules\grpc-tools\bin\protoc.exe --help?

okraus-ari commented 3 weeks ago

Well, nothing, that's the problem, I suppose:

> robotframework-browser\node_modules\grpc-tools\bin\protoc.exe --help
>

I have had already tried commands from the pipeline you have mentioned.

That utility is a little bit outdated, I can try to do some update, but I am not sure about consequences.

aaltat commented 3 weeks ago

Yes you can update the tool, CI will tell is it working with other platforms

aaltat commented 2 weeks ago

I had idea and remembered what might cause this problem. Basically fixed this be creating the list of dictionaries before passing it in the library keyword. See #3885 last two commits, also the Windows throws a different error.

I am not sure what causes this problem, but in practice I am happy with this solution. So the question remains: Do you want to incorporate the solution from #3885 to this PR or would you be happy to merge the #3885 to main? In any way you choose, I will credit you in the release notes and your commits remain unchanged in the history.

okraus-ari commented 2 weeks ago

Perfect, thank you for your help, I really appreciate it. I think this is much simpler to merge the #3885. The most important thing is that it gets merged. It would be sad if we had to abandon this because of some small problem in tests.

aaltat commented 2 weeks ago

OK. lets do that. I will close this one on favor of #3885

okraus-ari commented 2 weeks ago

Thank you, looking forward to the release.

aaltat commented 2 weeks ago

There is one PR that I would like to get in #3887 and also Playwright, usually, makes releases in the middle of the month and I am waiting that one too.

aaltat commented 2 weeks ago

To be more precise, we are waiting this one https://github.com/microsoft/playwright/issues/33047