From ae4b3169fa24e822fcd226cc8ac7e48036df4277 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Sun, 26 Mar 2023 17:04:04 -0400 Subject: [PATCH 01/10] Add API install per playwright-go breaking change docs --- pkg/provider/browser/browser.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/provider/browser/browser.go b/pkg/provider/browser/browser.go index 83314c971..08df2d618 100644 --- a/pkg/provider/browser/browser.go +++ b/pkg/provider/browser/browser.go @@ -26,6 +26,13 @@ func New(idpAccount *cfg.IDPAccount) (*Client, error) { func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error) { + // Automatically download browser downloads to match behavior releases < 2.36.4 + // TODO: provide override to not automatically download playwright browsers + err := playwright.Install() + if err != nil { + return "", err + } + pw, err := playwright.Run() if err != nil { return "", err From f23b70296e4aaf453abfb304d54716089f18c1b9 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Fri, 14 Apr 2023 00:16:46 -0400 Subject: [PATCH 02/10] Setup to allow user to choose to download browser drivers from INI config --- pkg/cfg/cfg.go | 1 + pkg/provider/browser/browser.go | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/pkg/cfg/cfg.go b/pkg/cfg/cfg.go index aac86f275..61cb9a22d 100644 --- a/pkg/cfg/cfg.go +++ b/pkg/cfg/cfg.go @@ -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"` } diff --git a/pkg/provider/browser/browser.go b/pkg/provider/browser/browser.go index 08df2d618..b666fec40 100644 --- a/pkg/provider/browser/browser.go +++ b/pkg/provider/browser/browser.go @@ -16,21 +16,26 @@ var logger = logrus.WithField("provider", "browser") // Client client for browser based Identity Provider type Client struct { - Headless bool + Headless bool + DownloadBrowser bool } // New create new browser based client func New(idpAccount *cfg.IDPAccount) (*Client, error) { - return &Client{Headless: idpAccount.Headless}, nil + return &Client{ + Headless: idpAccount.Headless, + DownloadBrowser: idpAccount.DownloadBrowser, + }, nil } func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error) { - // Automatically download browser downloads to match behavior releases < 2.36.4 - // TODO: provide override to not automatically download playwright browsers - err := playwright.Install() - if err != nil { - return "", err + // Optionally download browser drivers if defined in config + if cl.DownloadBrowser { + err := playwright.Install() + if err != nil { + return "", err + } } pw, err := playwright.Run() From ec1b6936f9cdc205dd68ef73ccbca1c5121a61e4 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Thu, 13 Apr 2023 23:50:16 -0400 Subject: [PATCH 03/10] Added flag to allow set browser driver download behavior Need to figure out: - how to set loginDetails - integrate flag overrides with the creation of IDPAccount value - test for all of the changes. --- cmd/saml2aws/main.go | 1 + pkg/flags/flags.go | 1 + 2 files changed, 2 insertions(+) diff --git a/cmd/saml2aws/main.go b/cmd/saml2aws/main.go index e974e40a4..41db18fe0 100644 --- a/cmd/saml2aws/main.go +++ b/cmd/saml2aws/main.go @@ -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. Defaults to true. (env: SAML2AWS_AUTO_BROWSER_DOWNLOAD)").Envar("SAML2AWS_AUTO_BROWSER_DOWNLOAD").Default("true").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) diff --git a/pkg/flags/flags.go b/pkg/flags/flags.go index 9c741cb94..60d86408a 100644 --- a/pkg/flags/flags.go +++ b/pkg/flags/flags.go @@ -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 From 10506835974732a7a36a91d7b1f040095b09333e Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Sun, 16 Apr 2023 20:09:47 -0400 Subject: [PATCH 04/10] Initial go at allow user to configure browser download behavior --- cmd/saml2aws/commands/login.go | 8 ++++++++ pkg/creds/creds.go | 1 + pkg/provider/browser/browser.go | 4 ++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/cmd/saml2aws/commands/login.go b/cmd/saml2aws/commands/login.go index 97bbb7bd9..c7aceb2f2 100644 --- a/cmd/saml2aws/commands/login.go +++ b/cmd/saml2aws/commands/login.go @@ -224,6 +224,14 @@ func resolveLoginDetails(account *cfg.IDPAccount, loginFlags *flags.LoginExecFla loginDetails.MFAIPAddress = loginFlags.CommonFlags.MFAIPAddress } + // set Browser provider behavior if specified in either config or flag + // overly verbose for a boolean flag? + 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 diff --git a/pkg/creds/creds.go b/pkg/creds/creds.go index 0f846ff02..5aa697103 100644 --- a/pkg/creds/creds.go +++ b/pkg/creds/creds.go @@ -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 diff --git a/pkg/provider/browser/browser.go b/pkg/provider/browser/browser.go index b666fec40..a2974382c 100644 --- a/pkg/provider/browser/browser.go +++ b/pkg/provider/browser/browser.go @@ -30,8 +30,8 @@ func New(idpAccount *cfg.IDPAccount) (*Client, error) { func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error) { - // Optionally download browser drivers if defined in config - if cl.DownloadBrowser { + // Optionally download browser drivers if defined in config or set by flag + if cl.DownloadBrowser || loginDetails.DownloadBrowser { err := playwright.Install() if err != nil { return "", err From 8e50c767a622271c17111557c127f1df11f78454 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Sun, 16 Apr 2023 20:14:15 -0400 Subject: [PATCH 05/10] Have default to not download browsers --- cmd/saml2aws/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/saml2aws/main.go b/cmd/saml2aws/main.go index 41db18fe0..555769321 100644 --- a/cmd/saml2aws/main.go +++ b/cmd/saml2aws/main.go @@ -114,7 +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. Defaults to true. (env: SAML2AWS_AUTO_BROWSER_DOWNLOAD)").Envar("SAML2AWS_AUTO_BROWSER_DOWNLOAD").Default("true").BoolVar(&loginFlags.DownloadBrowser) + cmdLogin.Flag("download-browser-driver", "Automatically download browsers for Browser IDP. Defaults to true. (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) From 2753342d05aeb464279442c97e0a25af8aee7b6d Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Sun, 16 Apr 2023 22:02:43 -0400 Subject: [PATCH 06/10] Change it so we're only fiddling with the flag/option in one place --- cmd/saml2aws/commands/login.go | 3 +-- pkg/provider/browser/browser.go | 10 ++++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cmd/saml2aws/commands/login.go b/cmd/saml2aws/commands/login.go index c7aceb2f2..45abee191 100644 --- a/cmd/saml2aws/commands/login.go +++ b/cmd/saml2aws/commands/login.go @@ -224,8 +224,7 @@ func resolveLoginDetails(account *cfg.IDPAccount, loginFlags *flags.LoginExecFla loginDetails.MFAIPAddress = loginFlags.CommonFlags.MFAIPAddress } - // set Browser provider behavior if specified in either config or flag - // overly verbose for a boolean flag? + // set Browser provider behavior if specified in either flag or config, with flag taking precedence if loginFlags.DownloadBrowser { loginDetails.DownloadBrowser = loginFlags.DownloadBrowser } else if account.DownloadBrowser { diff --git a/pkg/provider/browser/browser.go b/pkg/provider/browser/browser.go index a2974382c..a93199ab6 100644 --- a/pkg/provider/browser/browser.go +++ b/pkg/provider/browser/browser.go @@ -16,22 +16,20 @@ var logger = logrus.WithField("provider", "browser") // Client client for browser based Identity Provider type Client struct { - Headless bool - DownloadBrowser bool + Headless bool } // New create new browser based client func New(idpAccount *cfg.IDPAccount) (*Client, error) { return &Client{ - Headless: idpAccount.Headless, - DownloadBrowser: idpAccount.DownloadBrowser, + Headless: idpAccount.Headless, }, nil } func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error) { - // Optionally download browser drivers if defined in config or set by flag - if cl.DownloadBrowser || loginDetails.DownloadBrowser { + // Optionally download browser drivers if specified + if loginDetails.DownloadBrowser { err := playwright.Install() if err != nil { return "", err From d2e851f3ec1adc8442b6034e5ddb10707dd5c073 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Sun, 16 Apr 2023 22:07:32 -0400 Subject: [PATCH 07/10] Invoke the download as directly part of the test 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 --- .github/workflows/go.yml | 1 - pkg/provider/browser/browser_test.go | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index d3fc9cfca..7f53fbf48 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -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 diff --git a/pkg/provider/browser/browser_test.go b/pkg/provider/browser/browser_test.go index edd2aeda0..54280fb0a 100644 --- a/pkg/provider/browser/browser_test.go +++ b/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, } resp, err := client.Authenticate(loginDetails) assert.Nil(t, err) From 006178e7ce4138623311381033dd4606bed25f8e Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Sun, 16 Apr 2023 22:27:36 -0400 Subject: [PATCH 08/10] Fix up language --- cmd/saml2aws/commands/login.go | 1 - cmd/saml2aws/main.go | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/saml2aws/commands/login.go b/cmd/saml2aws/commands/login.go index 45abee191..0a7766a4e 100644 --- a/cmd/saml2aws/commands/login.go +++ b/cmd/saml2aws/commands/login.go @@ -224,7 +224,6 @@ func resolveLoginDetails(account *cfg.IDPAccount, loginFlags *flags.LoginExecFla loginDetails.MFAIPAddress = loginFlags.CommonFlags.MFAIPAddress } - // set Browser provider behavior if specified in either flag or config, with flag taking precedence if loginFlags.DownloadBrowser { loginDetails.DownloadBrowser = loginFlags.DownloadBrowser } else if account.DownloadBrowser { diff --git a/cmd/saml2aws/main.go b/cmd/saml2aws/main.go index 555769321..328400df3 100644 --- a/cmd/saml2aws/main.go +++ b/cmd/saml2aws/main.go @@ -114,7 +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. Defaults to true. (env: SAML2AWS_AUTO_BROWSER_DOWNLOAD)").Envar("SAML2AWS_AUTO_BROWSER_DOWNLOAD").BoolVar(&loginFlags.DownloadBrowser) + 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) From 1dc50250274e148433cd30acc1a99533a941cdb1 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Tue, 18 Apr 2023 22:27:03 -0400 Subject: [PATCH 09/10] Added test to make sure stuff errors out if download is not explicitly 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 --- pkg/cfg/cfg.go | 9 +++++---- pkg/provider/browser/browser.go | 13 ++++++++++--- pkg/provider/browser/browser_test.go | 15 +++++++++++++++ 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/pkg/cfg/cfg.go b/pkg/cfg/cfg.go index 61cb9a22d..d547edf78 100644 --- a/pkg/cfg/cfg.go +++ b/pkg/cfg/cfg.go @@ -56,10 +56,11 @@ type IDPAccount struct { SAMLCache bool `ini:"saml_cache"` SAMLCacheFile string `ini:"saml_cache_file"` 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 + 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 + BrowserDriverDir string `ini:"browser_driver_dir,omitempty"` // used by browser; hide from user if not set + Headless bool `ini:"headless"` // used by browser Prompter string `ini:"prompter"` } diff --git a/pkg/provider/browser/browser.go b/pkg/provider/browser/browser.go index a93199ab6..4bea17373 100644 --- a/pkg/provider/browser/browser.go +++ b/pkg/provider/browser/browser.go @@ -17,26 +17,33 @@ var logger = logrus.WithField("provider", "browser") // Client client for browser based Identity Provider type Client struct { Headless bool + // Setup alternative directory to download playwright browsers to + BrowserDriverDir string } // New create new browser based client func New(idpAccount *cfg.IDPAccount) (*Client, error) { return &Client{ - Headless: idpAccount.Headless, + Headless: idpAccount.Headless, + BrowserDriverDir: idpAccount.BrowserDriverDir, }, nil } func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error) { + runOptions := playwright.RunOptions{} + if cl.BrowserDriverDir != "" { + runOptions.DriverDirectory = cl.BrowserDriverDir + } // Optionally download browser drivers if specified if loginDetails.DownloadBrowser { - err := playwright.Install() + err := playwright.Install(&runOptions) if err != nil { return "", err } } - pw, err := playwright.Run() + pw, err := playwright.Run(&runOptions) if err != nil { return "", err } diff --git a/pkg/provider/browser/browser_test.go b/pkg/provider/browser/browser_test.go index 54280fb0a..c21cde716 100644 --- a/pkg/provider/browser/browser_test.go +++ b/pkg/provider/browser/browser_test.go @@ -47,6 +47,21 @@ func TestValidate(t *testing.T) { assert.Equal(t, resp, response) } +// Test that if download directory does not have browsers, it fails with expected error message +func TestNoBrowserDriverFail(t *testing.T) { + account := &cfg.IDPAccount{ + Headless: true, + BrowserDriverDir: t.TempDir(), // set up a directory we know won't have drivers + } + loginDetails := &creds.LoginDetails{ + URL: "https://google.com/", + } + client, _ := New(account) + _, err := client.Authenticate(loginDetails) + assert.Error(t, err) + assert.ErrorContains(t, err, "could not start driver") +} + func fakeSAMLResponse(page playwright.Page, loginDetails *creds.LoginDetails) (string, error) { return response, nil } From c539464770026f85702fc9d4ee3ee3abc6e65165 Mon Sep 17 00:00:00 2001 From: Xinke Chen Date: Tue, 18 Apr 2023 22:40:51 -0400 Subject: [PATCH 10/10] Updated README about new flags and browser IDP behavior --- README.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/README.md b/README.md index de530ae3f..5a94e24e7 100644 --- a/README.md +++ b/README.md @@ -248,6 +248,7 @@ Commands: 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) --cache-saml Caches the SAML response (env: SAML2AWS_CACHE_SAML) --cache-file=CACHE-FILE The location of the SAML cache file (env: SAML2AWS_SAML_CACHE_FILE) + --download-browser-driver Automatically download browsers for Browser IDP. (env: SAML2AWS_AUTO_BROWSER_DOWNLOAD) --disable-sessions Do not use Okta sessions. Uses Okta sessions by default. (env: SAML2AWS_OKTA_DISABLE_SESSIONS) --disable-remember-device Do not remember Okta MFA device. Remembers MFA device by default. (env: SAML2AWS_OKTA_DISABLE_REMEMBER_DEVICE) @@ -544,6 +545,18 @@ region = us-east-1 To use this you will need to export `AWS_DEFAULT_PROFILE=customer-test` environment variable to target `test`. +### Playwright Browser Drivers for Browser IDP + +If you are using the Browser Identity Provider, on first invocation of `saml2aws login` you need to remember to install +the browser drivers in order for playwright-go to work. Otherwise you will see the following error message: + +`Error authenticating to IDP.: could not start driver: fork/exec ... no such file or directory` + +To install the drivers, you can: +* Pass `--download-browser-driver` to `saml2aws login` +* Set in your shell environment `SAML2AWS_AUTO_BROWSER_DOWNLOAD=true` +* Set `download_browser_driver = true` in your saml2aws config file, i.e. `~/.saml2aws` + ## Advanced Configuration (Multiple AWS account access but SAML authenticate against a single 'SSO' AWS account) Example: