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

Add support for autoclosing oauth flow window #1618

Merged
merged 1 commit into from Feb 20, 2024

Conversation

stgarf
Copy link
Contributor

@stgarf stgarf commented Feb 3, 2024

Summary

This solves a small UX friction point. It can be tested by using something that makes use of the interactive flow like gitsign and I've integrated those changes here to test with.

image

Update go.mod to contain:
replace github.com/sigstore/sigstore => github.com/stgarf/sigstore 510ce9e

and then run go mod tidy. You should be able to test out the changes. Of note is that the page will not close automatically if no connectorid (for gitsign for example) is set due to the fact that the window.close API call will only succeed if the page is the first item in the window's history.

git config --local commit.gpgsign true  # Sign all commits
git config --local tag.gpgsign true  # Sign all tags
git config --local gpg.x509.program gitsign  # Use gitsign for signing
git config --local gpg.format x509  # gitsign expects x509 args
git config --local gitsign.autoclose true
git config --local gitsign.autoclosetimeout 5
git config --local gitsign.connectorid https://accounts.google.com

Release Note

Updated the interactiveIDTokenSource struct to have autoclose and autocloseTimeout fields which default to false and 0 respectively. The original InteractiveSuccessHTML returned in flow.go has been changed from a static string to a dynamic go template which can be returned with a call to GetInteractiveSuccessHTML(autoclose bool, timeout int).

Documentation

This may require an update to docs but I am not sure where. I've only seen gitsign using this so far and I've already made some documentation updates in my fork of it.

@kommendorkapten
Copy link
Member

Code looks good, but I haven't tested it.

pkg/oauth/interactive.go Outdated Show resolved Hide resolved
@haydentherapper
Copy link
Contributor

@wlynch @imjasonh pinging you here too, @bobcallaway was wondering if you had tried this out and run into an issue

Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

I tried to get it working awhile back and had trouble, but my javascript is also very weak. (@stgarf If you happened know why my last attempt to get this to work wasn't successful, I'd love to learn why! 🙏 )

@stgarf made a similar change in chainctl and we confirmed it works, so I'm all for this! 🚀

pkg/oauth/interactive.go Outdated Show resolved Hide resolved
@stgarf stgarf requested a review from wlynch February 14, 2024 07:12
@stgarf
Copy link
Contributor Author

stgarf commented Feb 14, 2024

@wlynch to answer your question for posterity the page will not close automatically if it was redirected to from another page, e.g. no social login was set, due to the fact that the window.close API call will only succeed if the page is the first item in the window's history.

Introduce the ability to automatically close the browser window after a
successful OAuth authentication flow in Sigstore. The HTML returned by
`GetInteractiveSuccessHTML` varies based on the autoclose flag and a
specified timeout. This change improves the ux by eliminating the need
to close the page manually after successful authentication.

Resolves: #1617

Signed-off-by: Steve Garf <garf@chainguard.dev>
@stgarf
Copy link
Contributor Author

stgarf commented Feb 16, 2024

Thanks folks! Commits have been squashed.

@bobcallaway bobcallaway merged commit 0e830c5 into sigstore:main Feb 20, 2024
9 checks passed
@stgarf stgarf deleted the feature/autoclose-auth-page branch February 20, 2024 18:02
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.

None yet

6 participants