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

feat: Enable custom scripts #2

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Enable custom scripts #2

wants to merge 3 commits into from

Conversation

mgagliardo91
Copy link
Owner

This PR adds more functionality to this fork against vlaci/openconnect-sso (open PR at vlaci#76) to enable custom user scripts and screenshots on failures.

Thanks @YoungElPaso for suggesting the enhancements.

From a comment:

Could I suggest loading a JS file to append to or substitute for the scripts for autofill? I think the differences in SSO flows requires some ability to fine tune what selector get's clicked when, what get's filled out or checked etc and I don't think the rules in the config.toml are sufficient to handle all of the cases.

Since you're proposing using Selenium (which I think is a great idea!) I think it would be useful to be able to execute user scripts more directly, since the audience of people who can implement those for Selenium is bound to be higher than for Qt.

I should add, that I suggest this because my org's SSO flow is overly complicated and cannot be accomodated via the rules.

Oh, also, perhaps consider a screenshot dump for failures via Selenium? That would be super useful for debugging headless environments!

Changes:

  • Allow a custom javascript script to be passed which will override the auto_fill_rules via the CLI argument --override-script or config property override_script. The script will be expanded with environment variables including USERNAME and PASSWORD to allow generic script creation. An example of a working custom script (same functionality as the default rules) is available at example/defaultRules.js

  • Screenshot failures using selenium during an exchange when in headless mode. Screenshots will be saved to the current working directory.

  • Allow specifying a timeout value for waiting on the SSO exchange in the browser using the CLI argument --authenticate_timeout or config property authenticate_timeout (value in seconds - defaults to 10).

Example:

openconnect-sso --server https://my.server --username mgagliardo91 --browser-display-mode hidden --override-script example/defaultRules.js

@YoungElPaso
Copy link

YoungElPaso commented Feb 10, 2022

Hi @mgagliardo91 this works quite well, and the screenshot on error is great! However, I can't quite get it to work, but I'm sure that's down to our setup.

I'm 90% sure the problem here is that our SSO flow has an interstitial page that includes a client-side redirect. So while I can get the custom script to fill out that page fine (and I can debug by assigning inline CSS styles - which again, is great thanks to the custom script!) but the redirect fools the authenticator into thinking that it's already at the final authentication page and it starts checking for the cookie even though in our flow, the next page is actually the final step.

So, on our last page, the scripts don't run, the lasts submit doesn't get clicked meanwhile the authenticator is trying to fetch the cookie until it times out.

The only solution I can think of is if the cookie checking loop was combined with the autoFill() so in my case, while it is checking for the cookie, it could submit that final form. So maybe that loop could be closed? I dunno. Maybe that makes sense - no cookie, so attempt autofill? Some sort of recursion combined with the proper selectors would probably work for anyone like me who has more hoops to jump through.

I think this improvements here in this PR are great though and the solution for me is probably to fork this branch and then hack away.

So, for what it's worth, your changes look great to me and work well - I only wish I could provide a more comprehensive and obviously successfull test!

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