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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 12 commits into from
Apr 23, 2023
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ae4b316
Add API install per playwright-go breaking change docs
xinkecf35 Mar 26, 2023
f23b702
Setup to allow user to choose to download browser drivers from INI co…
xinkecf35 Apr 14, 2023
ec1b693
Added flag to allow set browser driver download behavior
xinkecf35 Apr 14, 2023
1050683
Initial go at allow user to configure browser download behavior
xinkecf35 Apr 17, 2023
8e50c76
Have default to not download browsers
xinkecf35 Apr 17, 2023
2753342
Change it so we're only fiddling with the flag/option in one place
xinkecf35 Apr 17, 2023
d2e851f
Invoke the download as directly part of the test
xinkecf35 Apr 17, 2023
006178e
Fix up language
xinkecf35 Apr 17, 2023
b1946bf
Merge branch 'master' into fix-browser-idp-auto-download
mapkon Apr 18, 2023
6956721
Merge branch 'master' into fix-browser-idp-auto-download
mapkon Apr 19, 2023
1dc5025
Added test to make sure stuff errors out if download is not explicitl…
xinkecf35 Apr 19, 2023
c539464
Updated README about new flags and browser IDP behavior
xinkecf35 Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/go.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ jobs:

- name: Test
run: |
go run github.com/playwright-community/playwright-go/cmd/playwright install
go test -v ./... -coverprofile=${{ matrix.os }}_coverage.txt -covermode=atomic

- name: Upload coverage report
Expand Down
6 changes: 6 additions & 0 deletions cmd/saml2aws/commands/login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,12 @@ func resolveLoginDetails(account *cfg.IDPAccount, loginFlags *flags.LoginExecFla
loginDetails.MFAIPAddress = loginFlags.CommonFlags.MFAIPAddress
}

if loginFlags.DownloadBrowser {
loginDetails.DownloadBrowser = loginFlags.DownloadBrowser
} else if account.DownloadBrowser {
loginDetails.DownloadBrowser = account.DownloadBrowser
}

// log.Printf("loginDetails %+v", loginDetails)

// if skip prompt was passed just pass back the flag values
Expand Down
1 change: 1 addition & 0 deletions cmd/saml2aws/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ func main() {
cmdLogin.Flag("credentials-file", "The file that will cache the credentials retrieved from AWS. When not specified, will use the default AWS credentials file location. (env: SAML2AWS_CREDENTIALS_FILE)").Envar("SAML2AWS_CREDENTIALS_FILE").StringVar(&commonFlags.CredentialsFile)
cmdLogin.Flag("cache-saml", "Caches the SAML response (env: SAML2AWS_CACHE_SAML)").Envar("SAML2AWS_CACHE_SAML").BoolVar(&commonFlags.SAMLCache)
cmdLogin.Flag("cache-file", "The location of the SAML cache file (env: SAML2AWS_SAML_CACHE_FILE)").Envar("SAML2AWS_SAML_CACHE_FILE").StringVar(&commonFlags.SAMLCacheFile)
cmdLogin.Flag("download-browser-driver", "Automatically download browsers for Browser IDP. (env: SAML2AWS_AUTO_BROWSER_DOWNLOAD)").Envar("SAML2AWS_AUTO_BROWSER_DOWNLOAD").BoolVar(&loginFlags.DownloadBrowser)
cmdLogin.Flag("disable-sessions", "Do not use Okta sessions. Uses Okta sessions by default. (env: SAML2AWS_OKTA_DISABLE_SESSIONS)").Envar("SAML2AWS_OKTA_DISABLE_SESSIONS").BoolVar(&commonFlags.DisableSessions)
cmdLogin.Flag("disable-remember-device", "Do not remember Okta MFA device. Remembers MFA device by default. (env: SAML2AWS_OKTA_DISABLE_REMEMBER_DEVICE)").Envar("SAML2AWS_OKTA_DISABLE_REMEMBER_DEVICE").BoolVar(&commonFlags.DisableRememberDevice)

Expand Down
1 change: 1 addition & 0 deletions pkg/cfg/cfg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ type IDPAccount struct {
TargetURL string `ini:"target_url"`
DisableRememberDevice bool `ini:"disable_remember_device"` // used by Okta
DisableSessions bool `ini:"disable_sessions"` // used by Okta
DownloadBrowser bool `ini:"download_browser_driver"` // used by browser
Headless bool `ini:"headless"` // used by browser
Prompter string `ini:"prompter"`
}
Expand Down
1 change: 1 addition & 0 deletions pkg/creds/creds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package creds
type LoginDetails struct {
ClientID string // used by OneLogin
ClientSecret string // used by OneLogin
DownloadBrowser bool // used by Browser
MFAIPAddress string // used by OneLogin
Username string
Password string
Expand Down
1 change: 1 addition & 0 deletions pkg/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ type CommonFlags struct {
// LoginExecFlags flags for the Login / Exec commands
type LoginExecFlags struct {
CommonFlags *CommonFlags
DownloadBrowser bool
Force bool
DuoMFAOption string
ExecProfile string
Expand Down
12 changes: 11 additions & 1 deletion pkg/provider/browser/browser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,21 @@ type Client struct {

// New create new browser based client
func New(idpAccount *cfg.IDPAccount) (*Client, error) {
return &Client{Headless: idpAccount.Headless}, nil
return &Client{
Headless: idpAccount.Headless,
}, nil
}

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

// Optionally download browser drivers if specified
if loginDetails.DownloadBrowser {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@xinkecf35 xinkecf35 Apr 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

err := playwright.Install()
if err != nil {
return "", err
}
}

pw, err := playwright.Run()
if err != nil {
return "", err
Expand Down
3 changes: 2 additions & 1 deletion pkg/provider/browser/browser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to test fail without download?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@xinkecf35 xinkecf35 Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@xinkecf35 xinkecf35 Apr 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
resp, err := client.Authenticate(loginDetails)
assert.Nil(t, err)
Expand Down