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

feat: client certificate selector option during e2e tests #30203

Conversation

alienkarma
Copy link

@alienkarma alienkarma commented Sep 8, 2024

Additional details

Added an extra certificate filter when certificate is chosen for requests based on groups.
This can be used to tackle the problem of testing multiple certificate cases for the same URL.
Needs review and feedback.

Implementation

Certificates will be marked with a group tag as below in config:-

clientCertificates: [
    {
      url: 'https://a.host.com',
      ca: ['certs/user.pem'],
      certs: [
        {
          group: "user",
          cert: 'certs/user.pem',
          key: 'certs/user.key',
        },
      ],
    },
    {
      url: 'https://a.host.com',
      ca: ['certs/admin.pem'],
      certs: [
        {
          group: "admin",
          cert: 'certs/admin.pem',
          key: 'certs/admin.key',
        },
      ],
    },
]

And then intended to be swapped using cy.chooseCert(group_name) during tests. (Null or empty can be provided for resetting the certificate selection)

Steps to test

Need help implementing testing for this

How has the user experience changed?

Check implementation above

PR Tasks

Added an extra certificate filter when certificate is chosen for requests based on groups, This can be used to tackle the problem of testing multiple certificate cases for the same URL. Needs review and feedback.
@cypress-app-bot
Copy link
Collaborator

@CLAassistant
Copy link

CLAassistant commented Sep 8, 2024

CLA assistant check
All committers have signed the CLA.

@alienkarma alienkarma changed the title Client selector option during e2e tests feat: client selector option during e2e tests Sep 8, 2024
@alienkarma alienkarma changed the title feat: client selector option during e2e tests feat: client certificate selector option during e2e tests Sep 9, 2024
@jennifer-shehane
Copy link
Member

@alienkarma We would need tests to cover this new behavior in order to accept this PR. Can you add those?

…g, fixed ts errors on spec file and added relevant tests
@alienkarma
Copy link
Author

@alienkarma We would need tests to cover this new behavior in order to accept this PR. Can you add those?

Hi
Yes I initially needed help on this PR to proceed. But now I have a rough idea on how to go about it.
I will convert this PR into a draft until I am ready.
Thank you

@alienkarma alienkarma marked this pull request as draft September 10, 2024 01:19
…ports and error messages and fixed client certificate bug
…iple-certificates-selectCert" and test "multiple_certificates_selectCert"

Note: Certs folder unfortunately has to be added to prevent start up errors in referencing the certificates
@alienkarma alienkarma marked this pull request as ready for review September 10, 2024 07:03
@alienkarma
Copy link
Author

Hi @jennifer-shehane
I added the tests, fixed the types and few bugs
Let me know if I need to do anything else or if you have any other feedback
(I can also work on the cypress documentation once this gets checked and released)
Thank you

@alienkarma
Copy link
Author

Added the relevant docs - cypress-io/cypress-documentation#5922

@alienkarma
Copy link
Author

I think the linux-x64-contributor workflow check has hung and cannot proceed.

@alienkarma
Copy link
Author

Hi @jennifer-shehane
Just pinging that this PR is ready but I think it is stuck on a workflow.
If anything is needed let me know.
Thanks

@alienkarma
Copy link
Author

Hi @jennifer-shehane
PR is ready for review and is pending workflow approval.
It is up to date with develop branch.

@jennifer-shehane
Copy link
Member

@alienkarma Let me run the tests and see if someone can pick up review in this sprint.

@jennifer-shehane
Copy link
Member

@alienkarma Hey, we appreciate the time you put into this PR. The team did take a look over the intentions and implementation of the PR. There are a few implementation details that differ from our desired goal for this functionality.

  • Since there is existing config for the client certificates, it would be best to open up the config to be writable (instead of read only today) instead of introducing an entirely new command.
  • To truly change a client certificate in the browser requires an entire restart of the browser. This is why the config is not writable today (those config values require a restart of the browser to take affect). So for this to actually take affect in the browser, we would need to code a mechanism to restart the browser midway in a test once we recognize this config is being overwritten.

Coding these changes would be quite a rewrite and quite complex to have working across browsers. Right now we don't have the capacity to focus on this rewrite and would recommend closing this PR at this time. Sorry for the lack of communications upfront - we do appreciate your shedding more light on this problem to our team.

@alienkarma
Copy link
Author

Hi @jennifer-shehane
Thank you for the feedback. I am aware that changing certificates for browser midway creates other challenges, and thus this implementation was a way to introduce a solution to the problem.
I appreciate your time and consideration for this PR.

@alienkarma alienkarma closed this Sep 27, 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.

Override clientCertificates configuration
4 participants