Added support for browser-type and browser-executable-path #816

eliat123 · 2022-05-03T00:05:05Z

Improvement details:

This is the implementation to #815

An additional saml2aws command line argument & configuration to allow to the specification of BrowserTypeLaunchOptions.Channel for 'Browser' idp-provider.

Usage example:

This allows opening a chrome browser (instead of Chromium) when running saml2aws.

$ saml2aws configure --idp-provider=Browser --browser-type=chrome --username=${USERNAME} --region=${REGION} --url=${URL} --role=${AWS_ROLE} --idp-account=${IDP_ACCOUNT} --profile=${IDP_ACCOUNT} --skip-prompt
$ ${SAML2AWS} login --idp-account=${IDP_ACCOUNT} --profile=${IDP_ACCOUNT} --skip-prompt

eliat123 · 2022-05-11T19:34:01Z

@wolfeidau If possible, can I kindly ask you to have a look at this addition to the 'Browser' provider and let me know what you think.

efernandes-dev-ops · 2022-05-23T07:47:58Z

@eliat123 I believe this might also resolve 729.

MatteoJoliveau · 2022-07-05T08:38:07Z

Hi, I would like to push this PR because it would allow NixOS users like me to use the Browser provider. In NixOS you cannot simply download a random executable from the internet and run it because it doesn't use the Linux Filesystem Hierarchy Standard (FHS), therefore the setup script and subsequently downloaded chromium (after I patched the script by hand) won't work (see screenshots)

As a rule of thumb downloading executables to the user machine is not a really good practice. I prefer having complete control over what is in my system and installing software through the package manager.

The other thing that bothers me with the downloaded chrome executable is that it is run in Incognito mode, meaning I have to re-authenticate to my Google account every time. If saml2aws were to use the system browser (firefox in my case) it could reuse the active login session.

mlai · 2022-07-11T19:10:31Z

Looks like a good feature. Can you help to review and merge?

efernandes-dev-ops · 2022-07-13T07:52:01Z

@MatteoJoliveau We have the same usecase with regards to the chrome executable running in incognito mode which is a blocker for us in adopting this atm.

eliat123 · 2022-07-25T19:32:42Z

@wolfeidau kind reminder.
If possible, can I kindly ask you to have a look at this addition to the 'Browser' provider and let me know what you think?

TwiN · 2023-01-09T19:17:12Z

That'd definitely be nice, because the chromium browser is launched with incognito, which forces users to re-enter their credentials every single time.

@eliat123 any chance you can pull the recent changes from the base branch so that @wolfeidau can review & merge it in one go when they have time?

… into add_browser_type

eliat123 · 2023-01-09T21:30:19Z

Thank you @TwiN.
As requested, I pulled the recent changes from the upstream branch so that @wolfeidau can review & merge it in one go when they have time.

TwiN · 2023-01-10T18:42:44Z

cmd/saml2aws/main.go

@@ -70,6 +70,7 @@ func main() {
 	app.Flag("config", "Path/filename of saml2aws config file (env: SAML2AWS_CONFIGFILE)").Envar("SAML2AWS_CONFIGFILE").StringVar(&commonFlags.ConfigFile)
 	app.Flag("idp-account", "The name of the configured IDP account. (env: SAML2AWS_IDP_ACCOUNT)").Envar("SAML2AWS_IDP_ACCOUNT").Short('a').Default("default").StringVar(&commonFlags.IdpAccount)
 	app.Flag("idp-provider", "The configured IDP provider. (env: SAML2AWS_IDP_PROVIDER)").Envar("SAML2AWS_IDP_PROVIDER").EnumVar(&commonFlags.IdpProvider, "Akamai", "AzureAD", "ADFS", "ADFS2", "Browser", "GoogleApps", "Ping", "JumpCloud", "Okta", "OneLogin", "PSU", "KeyCloak", "F5APM", "Shibboleth", "ShibbolethECP", "NetIQ", "Auth0")
+	app.Flag("browser-type", "The configured browser type when the IDP provider is set to Browser. (env: SAML2AWS_BROWSER_TYPE)").Envar("SAML2AWS_BROWSER_TYPE").EnumVar(&commonFlags.BrowserType, "chrome", "chrome-beta", "chrome-dev", "chrome-canary", "msedge", "msedge-beta", "msedge-dev", "msedge-canary")


Is there a reason why there's no support for Firefox? 👀

browser, err := pw.Chromium.Launch(launchOptions) if err != nil { return "", err }

It looks like it always launches a Chromium-based browser, so I'd guess that's the reason why.

@TwiN, @zemberdotnet Added Firefox.

The original values were set based on the comment of the supported values by the playwright Channel; see: https://github.com/playwright-community/playwright-go/blob/8e8f670b5fa7ba5365ae4bfc123fea4aac359763/generated-structs.go#L441-L444

However, it totally made sense to me to add this capability, so I added support for Firefox and Webkit. (msedge is also supported using Chromium which is the default).

@eliat123 I looked at the code and it looks like the value from the browser-type flag isn't getting passed down when running the login command.

The NewSAMLClient gets constructed here on 78:

saml2aws/cmd/saml2aws/commands/login.go

Lines 70 to 78 in 0d135dc

loginDetails, err := resolveLoginDetails(account, loginFlags)

if err != nil {

log.Printf("%+v", err)

os.Exit(1)

}

logger.WithField("idpAccount", account).Debug("building provider")

provider, err := saml2aws.NewSAMLClient(account)

And the account that gets passed in is constructed here in buildIdpAccount where I assume we want to make sure the BrowserType property is set.

saml2aws/cmd/saml2aws/commands/login.go

Lines 31 to 34 in 0d135dc

account, err := buildIdpAccount(loginFlags)

if err != nil {

return errors.Wrap(err, "Error building login details.")

}

I may have a different use case than you, so not sure if this is outside the scope of your PR or not.

For reference I was running this command:

saml2aws login --browser-type=chrome

@zemberdotnet thank you for noticing that.

The reason for this behaviour is that the commonFlags can be passed both in the configure or other steps such as login. In my case, i have set the --browser-type in the configure and then it was available in the login.

To support providing the flag in the login command, I removed the additional check for commonFlags.IdpProvider == "Browser" since the BrowserType is only used by the Browser Provider, the functionality is still the same.

Please check it again as it should work now.

merusso · 2023-01-22T16:57:34Z

Hi, I would like to push this PR because it would allow NixOS users like me to use the Browser provider. In NixOS you cannot simply download a random executable from the internet and run it because it doesn't use the Linux Filesystem Hierarchy Standard (FHS), therefore the setup script and subsequently downloaded chromium (after I patched the script by hand) won't work (see screenshots)

@MatteoJoliveau, does this PR fix the issue you described? Skimming the code, I'm thinking it still uses a playwright browser, something that was downloaded to the playwright cache directory. It just allows users to change the playwright browser being used instead of only using Chromium.

MadOx710 · 2023-02-06T20:41:24Z

Definitely looking forward to Provider Browser support outside of Chromium. Context-Aware policies in Google Workspace leverages the Endpoint Verification Chrome plugin, and currently no Provider option for saml2aws works when Context-Aware policies are enabled.

eliat123 · 2023-03-03T19:20:58Z

@MadOx710, @MatteoJoliveau, @zemberdotnet, @TwiN, looking at the previous comments, are we ok with the playwright browser for now? or do want to wait for non-downloadable browser support?
If we are ok to continue with the first approach, let's merge this PR.
Otherwise, is there a feature request for non-downloadable browser support? Do you have a GitHub issue with its requirements? Is someone actively working on it?

I would be happy to contribute, but I would like to know the direction of this project.

eliat123 · 2023-06-15T01:42:51Z

@GuillaumeRoss playwright-go comes with "chromium," "firefox", "webkit", see: https://playwright-community.github.io/playwright-go/

This means that If you don't have one of the three browsers installed, it will download the browser for you and then perform the browser IDP authentication.

I have also added the ability to set the --browser-executable-path.
This means that playwright-go will execute the browser at your local path instead of downloading the browser, this means that you can configure to use the browser of your choice.

In addition, playwright-go supports a set of predefined browser types that are not downloadable, this means that if you have already installed that browser and set the --browser-type to it it will use the installed browser.
The list for that is one of the predefined list:

"chrome", "chrome-beta", "chrome-dev", "chrome-canary", "msedge", "msedge-beta", "msedge-dev", "msedge-canary"

I have tested this with chrome, chrome-canary, msedge and verified that it uses the ExecutablePath instead of the default one.

I believe this resolves most of the issues in #729

@mapkon @gliptak please review

mapkon

This looks on. Does that mean that we should close #815?

mapkon

Please checkout the linting errors and fix them.

codecov-commenter · 2023-06-15T03:55:27Z

Codecov Report

Merging #816 (e5d5822) into master (83489ec) will increase coverage by 0.04%.
The diff coverage is 48.57%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
+ Coverage   36.29%   36.33%   +0.04%     
==========================================
  Files          53       53              
  Lines        7941     7970      +29     
==========================================
+ Hits         2882     2896      +14     
- Misses       4686     4696      +10     
- Partials      373      378       +5

Flag	Coverage Δ
unittests	`36.33% <48.57%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/cfg/cfg.go	`43.84% <ø> (ø)`
pkg/flags/flags.go	`35.71% <0.00%> (-3.35%)`	⬇️
pkg/provider/browser/browser.go	`57.14% <58.62%> (+1.14%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eliat123 · 2023-06-15T04:36:42Z

Please checkout the linting errors and fix them.

@mapkon Thank you for noticing that, I updated the golint version to be able to see the ci lint issue and fixed it.

eliat123 · 2023-06-28T04:34:09Z

@mapkon I have made the necessary adjustments as per your request. Hope this will help.

dangymbol · 2023-11-08T13:16:49Z

Long comment:
This looks like a big improvement for any nix environment. I'm using Linux Mint which Playwright doesn't currently support due to incorrect detection of the base linux version. That means running aws2saml with the browser provider gives me a delightful segmentation violation! This PR should fix that by allowing me to use the installed version instead.

I'm also not in favour of downloading binary versions of Chromium, Firefox and Webkit to a cache - since I already have the current ones installed through the standard package manager.

Short comment: +1 😄

mapkon · 2023-11-09T02:05:54Z

@dangymbol Would you mind taking on this PR and updating it?

eliat123 · 2023-11-09T05:40:23Z

@mapkon I have updated this PR.
@dangymbol FYI.

gliptak · 2023-11-09T23:17:01Z

pkg/provider/browser/browser.go

@@ -56,10 +72,37 @@ func (cl *Client) Authenticate(loginDetails *creds.LoginDetails) (string, error)
 		Headless: playwright.Bool(cl.Headless),
 	}

-	// currently using Chromium as it is widely supported for Identity providers
+	validBrowserTypes := []string{"chromium", "firefox", "webkit", "chrome", "chrome-beta", "chrome-dev", "chrome-canary", "msedge", "msedge-beta", "msedge-dev", "msedge-canary"}


why differentiate all these browsertypes if playwright only supports firefox, chrome and webkit?

I see #816 (comment)

maybe the if statement below is to be updated?

Hi @gliptak, Thank you for your review.

Playwright does support all of those browser types, see:
https://playwright.dev/docs/browsers and https://github.com/playwright-community/playwright-go/blob/main/generated-structs.go#L691C4-L695

The main idea behind this PR is that the user can choose a browser type which is already installed on their system.

If you choose a browser you do not have on your system, you will get a relevant error message.

For example, I tried to set --browser-type=msedge. Since I did not have msedge installed I got the following notification:

INFO[0000] Setting browser type: msedge provider=browser Error authenticating to IdP.: could not send message: Chromium distribution 'msedge' is not found at /Applications/Microsoft Edge.app/Contents/MacOS/Microsoft Edge Run "npx playwright install msedge"

After I installed msedge, it used the installed one:

INFO[0000] Setting browser type: msedge provider=browser INFO[0001] opening browser URL="[IDP_URL]" provider=browser INFO[0021] waiting ... provider=browser INFO[0049] clean up browser provider=browser

Furthermore, If your browser is installed on a non-default location, you can utilize --browser-executable-path, and it will start the browser from that path.

Hope this explains it.

gliptak · 2023-11-10T02:26:57Z

pkg/provider/browser/browser.go

+	// Default browser is Chromium as it is widely supported for Identity providers,
+	// It can also be set to the other playwright browsers: Firefox and WebKit
+	browserType := pw.Chromium
+	if cl.BrowserType == "firefox" {


@eliat123 would there be more browsertypes mapped here?

https://playwright.dev/docs/browsers

@gliptak Nope. Only these browsertypes are supported.

Ref: https://github.com/playwright-community/playwright-go/blob/ddc7abdd06db016d63664de1ab08870195085ddb/playwright.go#L21-L23

According to https://playwright.dev/docs/browsers :

Chromium is for "chromium", "chrome", "msedge", "chrome-beta", "msedge-beta" or "msedge-dev".

Firefox is for the recent Firefox Stable build.

Webkit is for the recent WebKit build.

Chromium will be the default option, but if "webkit" or "firefox" are selected, they will be used instead. This ensures the most available options.

will be browserType be pw.Chromium when cl.BrowserType == 'msedge' and is this as expected?

@gliptak The answer is yes.

Here is the section from Playwright docs about Google Chrome & Microsoft Edge:

While Playwright can download and use the recent Chromium build, it can operate against the branded Google Chrome and Microsoft Edge browsers available on the machine (note that Playwright doesn't install them by default). In particular, the current Playwright version will support Stable and Beta channels of these browsers. Available channels are 'chrome', 'msedge', 'chrome-beta', 'msedge-beta' or 'msedge-dev'.

ref: https://playwright.dev/docs/browsers#google-chrome--microsoft-edge

mikex99 · 2023-11-15T16:17:15Z

Can I get a link to this build for testing?

gliptak · 2023-11-15T16:55:29Z

@mikex99 bottom of https://github.com/Versent/saml2aws/actions/runs/6807857396

merusso · 2023-11-18T07:13:37Z

Does this implementation allow use of an system's existing, default browser (#729)? It seems like it enables use of other Playwright browsers, which are still managed separately.

kbarlowgw · 2023-12-08T17:19:39Z

I am trying to use this to run a local browser not in incognito mode but, while I can run a local chromium install, it still seems that playwright wants to open in it incognito mode. How do we prevent this?

For settings, I am trying setting SAML2AWS_BROWSER_EXECUTABLE_PATH="chromium" and running --browser-type=chrome but it still opens in incognito mode.

kbarlowgw · 2023-12-08T18:17:51Z

Following up on this to see if there was a workaround or playwright switch I could use, it seems this ability is still in the works with playwright and we may have to update to use browser.launchPersistentContext() instead of launch() when complete:

microsoft/playwright#11833

If someone knows of another option, please let me know.

Thanks.

gliptak · 2023-12-09T13:25:08Z

@merusso consider opening an issue for finding/running with default browser

@kbarlowgw consider opening an issue on updating to browser.launchPersistentContext() are there cases when launch() is applicable? would this be a switch and what is the default?

@eliat123 for awareness

merusso · 2023-12-09T19:15:07Z

@gliptak I believe this was already created, #729. I believe the issue was mistakenly closed when this PR was merged.

added support for browser-type

4786882

eliat123 changed the title ~~added support for browser-type~~ Added support for browser-type May 3, 2022

MatteoJoliveau mentioned this pull request Jul 5, 2022

[Browser Provider] Support browser type #815

Closed

Merge branch 'master' into add_browser_type

18bb93f

elicopilot added 2 commits January 9, 2023 12:28

Merge branch 'master' into add_browser_type

5429e6b

Merge branch 'add_browser_type' of https://github.com/eliat123/saml2aws…

f45c5f8

… into add_browser_type

TwiN reviewed Jan 10, 2023

View reviewed changes

added support for Firefox and Webkit

6620c45

eliat123 requested review from TwiN and zemberdotnet and removed request for TwiN and zemberdotnet January 10, 2023 22:16

Set BrowserType without Idp-provider restriction

62fabce

eliat123 requested review from zemberdotnet and TwiN and removed request for TwiN and zemberdotnet January 11, 2023 20:33

Merge branch 'master' into add_browser_type

50f0a14

added README

e5d5822

mapkon reviewed Jun 15, 2023

View reviewed changes

mapkon linked an issue Jun 15, 2023 that may be closed by this pull request

Using locally installed browser rather than outdated sanboxed chromium from "playwright-go" #729

Closed

mapkon suggested changes Jun 15, 2023

View reviewed changes

update GOLANGCI_VERSION to 1.53.2 and fix lint error

935a519

eliat123 requested a review from mapkon June 15, 2023 04:34

eliat123 requested a review from zemberdotnet June 15, 2023 23:52

eliat123 changed the title ~~Added support for browser-type~~ Added support for browser-type and browser-executable-path Jun 15, 2023

Merge branch 'master' into add_browser_type

fa5d8e7

Updated with upstream/master

86cd2fd

mapkon approved these changes Nov 9, 2023

View reviewed changes

gliptak reviewed Nov 9, 2023

View reviewed changes

gliptak reviewed Nov 10, 2023

View reviewed changes

eliat123 requested a review from gliptak November 10, 2023 07:34

gliptak approved these changes Nov 10, 2023

View reviewed changes

mapkon merged commit 0b61e6e into Versent:master Nov 11, 2023
8 checks passed

	loginDetails, err := resolveLoginDetails(account, loginFlags)
	if err != nil {
	log.Printf("%+v", err)
	os.Exit(1)
	}

	logger.WithField("idpAccount", account).Debug("building provider")

	provider, err := saml2aws.NewSAMLClient(account)

	account, err := buildIdpAccount(loginFlags)
	if err != nil {
	return errors.Wrap(err, "Error building login details.")
	}

Added support for browser-type and browser-executable-path #816

Added support for browser-type and browser-executable-path #816

Conversation

eliat123 commented May 3, 2022

Improvement details:

Usage example:

eliat123 commented May 11, 2022

efernandes-dev-ops commented May 23, 2022

MatteoJoliveau commented Jul 5, 2022

mlai commented Jul 11, 2022

efernandes-dev-ops commented Jul 13, 2022

eliat123 commented Jul 25, 2022

TwiN commented Jan 9, 2023

eliat123 commented Jan 9, 2023

Choose a reason for hiding this comment

zemberdotnet Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zemberdotnet Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merusso commented Jan 22, 2023

MadOx710 commented Feb 6, 2023

eliat123 commented Mar 3, 2023

eliat123 commented Jun 15, 2023

mapkon left a comment

Choose a reason for hiding this comment

mapkon left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 15, 2023

Codecov Report

eliat123 commented Jun 15, 2023

eliat123 commented Jun 28, 2023

dangymbol commented Nov 8, 2023

mapkon commented Nov 9, 2023

eliat123 commented Nov 9, 2023

gliptak Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikex99 commented Nov 15, 2023

gliptak commented Nov 15, 2023

merusso commented Nov 18, 2023

kbarlowgw commented Dec 8, 2023 • edited Loading

kbarlowgw commented Dec 8, 2023

gliptak commented Dec 9, 2023

merusso commented Dec 9, 2023

zemberdotnet Jan 10, 2023 •

edited

Loading

zemberdotnet Jan 11, 2023 •

edited

Loading

gliptak Nov 9, 2023 •

edited

Loading

kbarlowgw commented Dec 8, 2023 •

edited

Loading