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

Playwright auth header #235

Merged
merged 4 commits into from
Mar 1, 2024
Merged

Playwright auth header #235

merged 4 commits into from
Mar 1, 2024

Conversation

adiel
Copy link
Member

@adiel adiel commented Feb 27, 2024

Playwright can add headers to page requests meaning we can pass any basic auth credentials properly as an Authorization header not just in the URL which is not really supported in browsers these days. As I understand it this is not possible with Selenium.

Part of #232

}).Sync();
} else {
var base64EncodedAuthenticationString = Convert.ToBase64String(Encoding.ASCII.GetBytes(userInfo));
_context.SetExtraHTTPHeadersAsync(new Dictionary<string, string> {
Copy link

Choose a reason for hiding this comment

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

This was the workaround I used, but this was my only option since the _context was created by the driver. The drawback of setting extra headers is that these are sent with every request, also to any API's or resources that the browser picks up while loading the site. That does not really match how the browser acts when the userInfo is part of the URL - it would then only be applied to that particular site. It should still work in most scenarios, but if there is other authentication involved towards APIs then this might interfere.

To mimic the browser behavior, there is an option to the context creation. Unfortunately there is no API to modify the HttpCredentials of the options afterwards, so using that option would require recreating the _context on every visit. This could be optimized to only recreating it if userInfo or site actually changes.

If you decide to stick with the current implementation, please note in the docs that this will be the behavior. Also, if userInfo is empty - set the extra headers to an empty dictionary, otherwise the requests will contain a blank Authorization header.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. So first of all, when I tested this Playwright does pass through the credentials to the browser if you pass them in the URL, so my assumption is that whatever you are seeing is a problem downstream from there. It's not really the best way to be doing this anyway so I don't think we can rely on credentials in the URL working.

I hadn't realised that even cross site requests initiated by the web page would end up carrying through the header like you say, but I tested that and confirmed Playwright is doing that. I kinda feel Playwright itself should let you scope which domains/paths you want request headers to be sent to when you set them and/or per page visit. That would solve this problem for every case I think.

As you say, recreating the context on the fly when you happen to pass credentials isn't really a goer because of the way Coypu handles multiple window scopes. Playwright is basically saying you have to reopen a window to use different credentials for a different website which is pretty nasty if you want to authorise against two different websites in the same session and interact with them side by side in one Coypu BrowserSession - which you could otherwise.

What we can do though is set the HttpCredentials on the BrowserSession when you use the AppHost parameter to define the website under test when creating a BrowserSession. This would allow you to match Playwright's behaviour and create a BrowserSession for each time you need a separate Playwright context. That's not a change to our API, just respecting the credentials in the URL that you set in AppHost.

Not sure what more we can do. What do you think?

Copy link

Choose a reason for hiding this comment

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

Strange that you could get the credentials through the URL - may depend on which browser you are targeting. In my case it was Edge.

Picking up the credentials through AppHost in BrowserSession sounds like a good middle ground. In my scenarios it is fully ok to use multiple Coypu BrowserSessions side by side to manage tests with multiple users involved.

I'm hesitant whether to pass the origin parameter of HttpCredentials also, to limit which sites playwright will pass the credentials to. It would work in my scenarios but might be breaking for others. Lets stay with your implementation to be on the safe side. At least the credentials will only be passed to sites that explicitly request Basic authentication.

Kudos for implementing this!

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't see a reason not to add the origin now that you mention it. I hadn't even spotted it tbh.

No ones used this yet with Playwright and it would also match the behaviour of any old test suits that are successfully relying on passing auth in the url with Selenium.

lets add it in then ship this

Copy link
Member Author

Choose a reason for hiding this comment

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

@bjowes done this now. Look ok?

Copy link

Choose a reason for hiding this comment

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

Looks good. I tested out this branch in my own environment, works like a charm!

@adiel adiel merged commit af2fc5b into master Mar 1, 2024
@sbowler sbowler deleted the playwright-auth-header branch April 8, 2024 16:13
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