Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 #1006

xinkecf35 · 2023-03-26T21:23:17Z

Description

This PR seeks to have the browser IDP option to once again automatically download Browser Drivers as was the default behavior for saml2aws versions <= 2.36.4

This PR would functionally close #1005

Motivation

When saml2aws was upgraded to 2.36.5 and using the IDP option of Browser, one would get this failure message:

Error to authenticating to IdP: could not start driver: fork/exec /Users/alice/Library/Caches/ms-playwright-go/1.20.0-beta-1647057403000/playwright.sh: no such file or directory

It appears that as part of upgrading browser binaries and thus playwright as part of issue #952 and PR #982 introduced a version of playwright-go that had breaking change to the underlying playwright driver that will no longer automatically acquire browser drivers.

Per the breaking change notice in playwright-go, this PR seeks to add the API Install method of acquiring the browser driver.

Alternatives

Alternatively, instead of baking in the install, the README could be updated to reflect this requirement, and have the user on their own acquired the driver via either the playwright-go CLI or the upstream Playwright Typescript project.

This however does add friction to use saml2aws, especially if one is using a package manager like Homebrew or Chocolatey, i.e. needing to on top of acquiring saml2aws via the documented means to also separately acquire Go Tools or NPM/NPX/Node to acquire playwright driver.

Additional Comments

The author recognizes the implementation is somewhat naive in that it gives no configuration to avoid downloading the drivers which might desirable, e.g. avoid re-downloading drivers for CI/CD tests

codecov-commenter · 2023-03-26T23:57:49Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 57.89474% with 8 lines in your changes missing coverage. Please review.

Project coverage is 36.40%. Comparing base (6755c3a) to head (c539464).
Report is 614 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/saml2aws/commands/login.go	0.00%	3 Missing and 2 partials ⚠️
pkg/provider/browser/browser.go	78.57%	2 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1006      +/-   ##
==========================================
+ Coverage   36.33%   36.40%   +0.06%     
==========================================
  Files          52       52              
  Lines        7530     7546      +16     
==========================================
+ Hits         2736     2747      +11     
- Misses       4412     4415       +3     
- Partials      382      384       +2

Flag	Coverage Δ
unittests	`36.40% <57.89%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mapkon

@xinkecf35

This is good, and it was by design that we do not impose the downloading of the playwright during the login process. From my analysis, it appears that the upstream project doesn't check or store info which allows it to invalidate the cache on upgrade, something that has been an issue on the upstream project: playwright-community/playwright-go#236

I see two solutions here:

Either way check that the binary exists and only download if it doesn't
Document the presence of playwright as a requisite library
Find a way to bundle both of them so they are installed together.

@briantist / @gliptak - any thoughts?

briantist · 2023-03-27T01:43:31Z

Hey @mapkon I'd like to help where I can, but I've actually never used golang and I'm not really familiar with the code in this project. I do have a lot of experience with GitHub Actions though, so hopefully I can continue to assist in those areas!

mapkon · 2023-03-27T01:45:48Z

Hey @mapkon I'd like to help where I can, but I've actually never used golang and I'm not really familiar with the code in this project. I do have a lot of experience with GitHub Actions though, so hopefully I can continue to assist in those areas!

No worries! Thanks for clarifying. I would still want your thoughts on whether documentation or auto-download is the way forward here?

gliptak · 2023-03-27T01:48:37Z

I can speak to what I observed working through the patch.

The downloads are fairly sizeable, so it is a lot of overhead to force for each interaction. On the other side, the cache expiration is unclear.

Would separating the download (allowing for external "cache refresh") be large impact?

xinkecf35 · 2023-03-27T21:59:24Z

mapkon/gliptak, yeah that's reasonable for not wanting to bake in the browser download anymore, at least in the exact way it was before with old version of playwright.

As another option, how palatable would be to have an option specific to IDP browser option or even a separate command to ask to install the driver?

Focusing largely as an user experience, it feels less "irritating" if one needs to acquire drivers to invoke a specific option on the saml2aws instead being surprised to needing acquiring tooling/toolchains that may not even use on a daily basis.

gliptak · 2023-03-29T01:07:06Z

including this with saml2aws sounds like a good approach (setting up playwright.Install() call at another control flow path)

reading through both flags and commands, both don't seem to offer provider specific overrides.

mapkon · 2023-03-29T01:27:36Z

including this with saml2aws sounds like a good approach (setting up playwright.Install() call at another control flow path)

reading through both flags and commands, both don't seem to offer provider specific overrides.

How about we document it? I am not really sure about downloading dependencies to enact the basic functionality of the program, which is login?

gliptak · 2023-03-29T02:08:05Z

download outside saml2aws would require enduser installing go

gliptak · 2023-03-31T01:03:55Z

other browser related #786 #729 #815

mapkon · 2023-04-13T22:58:47Z

Ref: #1011 (reply in thread)

@wolfeidau We are starting to get users reporting issues related to the playwright dependency not downloading, something that affects the functioning of the app. How do you suggest we proceed on this?

…nfig

Need to figure out: - how to set loginDetails - integrate flag overrides with the creation of IDPAccount value - test for all of the changes.

xinkecf35 · 2023-04-14T04:22:13Z

Just to provide a suggestion as a way to move forward:

Perhaps add a new configuration property to the config called download_browser_driver to choose to allow saml2aws to install the playwright browser for you with the new default going forward being not to?

Or maybe even a flag on saml2aws login called --download-browser-driver with environment variable to match?

mapkon · 2023-04-16T23:27:40Z

Just to provide a suggestion as a way to move forward:

Perhaps add a new configuration property to the config called download_browser_driver to choose to allow saml2aws to install the playwright browser for you with the new default going forward being not to?

Or maybe even a flag on saml2aws login called --download-browser-driver with environment variable to match?

I think this is a superb middle ground. Let's go with this!

Not the best thing to do since it brings us back to almost square one. Just the easiest thing I can think of before seriously breaking other parts of the interface/trying to figure out how to assert that playwright.Install() was called

gliptak · 2023-04-17T02:27:04Z

pkg/provider/browser/browser_test.go

@@ -39,7 +39,8 @@ func TestValidate(t *testing.T) {
 	client, err := New(account)
 	assert.Nil(t, err)
 	loginDetails := &creds.LoginDetails{
-		URL: "https://google.com/",
+		URL:             "https://google.com/",
+		DownloadBrowser: true,


is there a way to test fail without download?

In a word, yes. It should be easy to copy/paste this test without the flag set and check if the error is not nil since I believe that's how the error is being propagated up to the user.

Okay, so I've added the test, though the change was larger than I had thought it'll be.

I have added a new config parameter to allow one to set the directory that Playwright installs into though I've purposely not documented it.

I did this way because otherwise my test was going to be sensitive if a previous test ran and installed playwright before this test which introduces all sorts of headaches. Also couldn't think of a clean way to tear that down without being a royal pain to do so.

So instead, you technically can now choose where you download the drivers to, defaulting to the default location if you don't care.

I left the existing test so that it downloads the drivers to the default location though an argument could be made it shouldn't.

gliptak · 2023-04-17T02:28:06Z

pkg/provider/browser/browser.go

 }

 func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error) {

+	// Optionally download browser drivers if specified
+	if loginDetails.DownloadBrowser {


will this (re)download/install at every run?

It shouldn't re-download since playwright-go does have that caching mechanism. Just by manually invoking the relevant playwright install command it doesn't re-download the binary, at least not without first nuking the relevant cache.

With this code, if you were to set it in the config file, it would functionally bring you back to the 2.36.4 behavior where it always checks if you have the drivers or not. If you do, it outputs the relevant message about how the drivers are installed but with no progress bar for downloading the binaries.

Alternatively, if you don't set it in the config, it will only download the driver if you set the relevant flag.

mapkon

@xinkecf35 / @gliptak

I don't see anything holding this up. Are we good to go?

mapkon

Oh wait! I don't see the documentation updated. So that should be part of this

…y invoked Not super great a testing detail is leaked out to the config, but seems the cleanest way to do it without breaking the client creation function

mapkon · 2023-04-20T04:05:11Z

@gliptak Thoughts?

mikex99 · 2023-04-20T20:35:49Z

Any way to pass a flag during 'saml2aws configure' that would set the 'download_browser_driver = true' to the config file?

gliptak · 2023-04-20T22:42:17Z

@mikex99 I recommend targeting saml2aws configure in followup PR

mikex99 · 2023-04-27T16:18:25Z

When will a new version with this merge be released?

mapkon · 2023-05-04T00:52:51Z

When will a new version with this merge be released?

We are organizing a test of the current codebase. Please see here: #1029 (comment)

If you can spend sometime to test out the current master and ascertain release readiness, I will appreciate

mikex99 · 2023-05-04T15:22:45Z

When will a new version with this merge be released?

We are organizing a test of the current codebase. Please see here: #1029 (comment)

If you can spend sometime to test out the current master and ascertain release readiness, I will appreciate

I can test, but I would need a binary to test with.

xinkecf35 changed the title ~~Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers~~ Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 Mar 26, 2023

mapkon suggested changes Mar 27, 2023

View reviewed changes

Add API install per playwright-go breaking change docs

ae4b316

xinkecf35 force-pushed the fix-browser-idp-auto-download branch from f046cbb to ae4b316 Compare April 14, 2023 03:19

xinkecf35 added 2 commits April 14, 2023 00:17

Setup to allow user to choose to download browser drivers from INI co…

f23b702

…nfig

Added flag to allow set browser driver download behavior

ec1b693

Need to figure out: - how to set loginDetails - integrate flag overrides with the creation of IDPAccount value - test for all of the changes.

xinkecf35 added 5 commits April 16, 2023 20:09

Initial go at allow user to configure browser download behavior

1050683

Have default to not download browsers

8e50c76

Change it so we're only fiddling with the flag/option in one place

2753342

Invoke the download as directly part of the test

d2e851f

Not the best thing to do since it brings us back to almost square one. Just the easiest thing I can think of before seriously breaking other parts of the interface/trying to figure out how to assert that playwright.Install() was called

Fix up language

006178e

gliptak reviewed Apr 17, 2023

View reviewed changes

Merge branch 'master' into fix-browser-idp-auto-download

b1946bf

mapkon approved these changes Apr 18, 2023

View reviewed changes

mapkon reviewed Apr 18, 2023

View reviewed changes

mapkon and others added 3 commits April 19, 2023 11:05

Merge branch 'master' into fix-browser-idp-auto-download

6956721

Added test to make sure stuff errors out if download is not explicitl…

1dc5025

…y invoked Not super great a testing detail is leaked out to the config, but seems the cleanest way to do it without breaking the client creation function

Updated README about new flags and browser IDP behavior

c539464

gliptak approved these changes Apr 20, 2023

View reviewed changes

mapkon merged commit b4cff63 into Versent:master Apr 23, 2023

xinkecf35 deleted the fix-browser-idp-auto-download branch April 24, 2023 23:07

mapkon mentioned this pull request Aug 29, 2023

--download-browser-driver recurring failure #1098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 #1006

Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 #1006

xinkecf35 commented Mar 26, 2023 •

edited

Loading

codecov-commenter commented Mar 26, 2023 •

edited

Loading

mapkon left a comment

briantist commented Mar 27, 2023

mapkon commented Mar 27, 2023 •

edited

Loading

gliptak commented Mar 27, 2023

xinkecf35 commented Mar 27, 2023

gliptak commented Mar 29, 2023

mapkon commented Mar 29, 2023

gliptak commented Mar 29, 2023

gliptak commented Mar 31, 2023

mapkon commented Apr 13, 2023

xinkecf35 commented Apr 14, 2023 •

edited

Loading

mapkon commented Apr 16, 2023

gliptak Apr 17, 2023

xinkecf35 Apr 17, 2023

xinkecf35 Apr 19, 2023 •

edited

Loading

xinkecf35 Apr 19, 2023 •

edited

Loading

gliptak Apr 17, 2023

xinkecf35 Apr 17, 2023 •

edited

Loading

mapkon left a comment

mapkon left a comment

mapkon commented Apr 20, 2023

mikex99 commented Apr 20, 2023

gliptak commented Apr 20, 2023

mikex99 commented Apr 27, 2023

mapkon commented May 4, 2023

mikex99 commented May 4, 2023

Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 #1006

Browser IDP - Configure Browser IDP to automatically install Playwright Browser Drivers per old behavior <= 2.36.4 #1006

Conversation

xinkecf35 commented Mar 26, 2023 • edited Loading

Description

Motivation

Alternatives

Additional Comments

codecov-commenter commented Mar 26, 2023 • edited Loading

Codecov Report

mapkon left a comment

Choose a reason for hiding this comment

briantist commented Mar 27, 2023

mapkon commented Mar 27, 2023 • edited Loading

gliptak commented Mar 27, 2023

xinkecf35 commented Mar 27, 2023

gliptak commented Mar 29, 2023

mapkon commented Mar 29, 2023

gliptak commented Mar 29, 2023

gliptak commented Mar 31, 2023

mapkon commented Apr 13, 2023

xinkecf35 commented Apr 14, 2023 • edited Loading

mapkon commented Apr 16, 2023

gliptak Apr 17, 2023

Choose a reason for hiding this comment

xinkecf35 Apr 17, 2023

Choose a reason for hiding this comment

xinkecf35 Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

xinkecf35 Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

gliptak Apr 17, 2023

Choose a reason for hiding this comment

xinkecf35 Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

mapkon left a comment

Choose a reason for hiding this comment

mapkon left a comment

Choose a reason for hiding this comment

mapkon commented Apr 20, 2023

mikex99 commented Apr 20, 2023

gliptak commented Apr 20, 2023

mikex99 commented Apr 27, 2023

mapkon commented May 4, 2023

mikex99 commented May 4, 2023

xinkecf35 commented Mar 26, 2023 •

edited

Loading

codecov-commenter commented Mar 26, 2023 •

edited

Loading

mapkon commented Mar 27, 2023 •

edited

Loading

xinkecf35 commented Apr 14, 2023 •

edited

Loading

xinkecf35 Apr 19, 2023 •

edited

Loading

xinkecf35 Apr 19, 2023 •

edited

Loading

xinkecf35 Apr 17, 2023 •

edited

Loading