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: frontdoor via post #913

Merged
merged 33 commits into from
Jan 10, 2024
Merged

feat: frontdoor via post #913

merged 33 commits into from
Jan 10, 2024

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented Jan 2, 2024

What does this PR do?

  • keep tokens out of urls for human open
  • create a small html file locally that redirects to frontdoor.jsp via post
  • open that local file in the browser

What issues does this PR fix or reference?

@W-10025523@

also

What does this PR do?
add browserPrivate as an option for --browser

What issues does this PR fix or reference?
@W-14764106@
forcedotcom/cli#2262

forcedotcom/cli#1852

forcedotcom/cli#636

Notes

it is valid to do --json. That'll still have a AT-param in the url for machine use AND do the new open behavior for humans

QA challenges

  • does it work correctly on windows (that is, fs and tempDir are working correctly) and across various browsers and shells?
  • can you find a way to leave these temporary files on the filesystem with valid AT in them?

Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

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

Looks good, just left 1 suggestion and 2 requested changes.

@mshanemc mshanemc requested a review from a team as a code owner January 8, 2024 21:43
@cristiand391
Copy link
Member

QA notes:

testing done on win10 vm:

❌ tmp file gets deleted too quickly, tried ~20 times with firefox/edge, with them open/closed but by the time they try to access the file it is gone:
Screenshot 2024-01-09 at 14 23 10
Screenshot 2024-01-09 at 14 23 24
Screenshot 2024-01-09 at 14 23 52
Screenshot 2024-01-09 at 14 24 05

NOTE

edited the code to print the tmp file path.
sleeping for 3s works sometimes, but found 5s to be most consistent.

having the browser closed didn't seem to make any difference (I guess b/c the child procecss gets returned once it's opened so the time starts to run after that happens)

If I. delete the await rm line it works on firefox/edge and pwsh/git-bash/cmd

@cristiand391
Copy link
Member

cristiand391 commented Jan 9, 2024

QA update:

frontdoor file

windows

✅ browsers can open the file and do auth

sf org open -o cd --browser <browser>, without browser flag it still opens the default.
tested with firefox/edge and latest chrome.
Added a console.log to get the tmp filepath, always get deleted after the sleep (more details below)

✅ file gets deleted even if you specify a browser that isn't installed
uninstalled chrome and ran sf org open -o cd --browser chrome, the tmp file was created and deleted after 7s, exit code 0.

✅ file gets deleted if I kill the browser before the 7s wait

--json withouth --url-only
sf opens the tmp file for auth but still includes the frontdoor URL + AT in json.
json doesn't include path to tmp file
file gets deleted

--json with --url-only
doesn't open browser/doesn't create tmp file. frontdoor URL + AT in json

macos/linux

✅ chrome works (only tried on macos)

update:

✅ firefox works on mac and linux
firefox doesn't open the tmp file

linux: logout, then sf org open -o cd -b firefox opens the org but ask for credentials so it doesn't do the auth.
if I manually copy/paste the file:/// URL it works (redirect -> auth).

macos: logout, then sf org open -o cd -b firefox opens an empty tab.

on linux, if If run sf org open -o cd -b edge without having Edge installed, open crashes and the tmp file doesn't get deleted. Maybe put this in a try/catch block? doesn't happen on mac and win.
UPDATE:
try/catch/finally didn't work because the error is thrown by the child process instance after it gets out of the try block, I still can find the file in /tmp.
What worked:
listen on error even, delete the file there and re-throw the error.

✅ fixed, file gets deleted after spawn error.

--private flag

✅ macos
set chrome/firefox as default, then running the command open both in new incognito window.
not tested: edge

🟡 windows

chrome/edge as default work, new incognito window open.

with firefox it throws this error:
Screenshot 2024-01-10 at 10 29 39

there's an open issue in default-browser:
sindresorhus/default-browser#11

NOTE
that last change to delete the file in the finally block covers this, it gets deleted on this failure.

🟡 linux

can't get chrome/edge installed.
with firefox I get this error:
Error: Firefox-P9tn31 is not supported as a default browser

similar to this:
sindresorhus/open#329

not sure if the fix there was related to chrome only, I'll try to get more info a log a new issue there.

messages/open.md Outdated Show resolved Hide resolved
messages/open.md Outdated Show resolved Hide resolved
Co-authored-by: Juliet Shackell <63259011+jshackell-sfdc@users.noreply.github.com>
@mshanemc mshanemc mentioned this pull request Jan 9, 2024
Copy link
Member

@cristiand391 cristiand391 left a comment

Choose a reason for hiding this comment

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

1 suggestion about flags and 1 requested change for open's incognito API

@cristiand391 cristiand391 merged commit 3bb3176 into main Jan 10, 2024
12 checks passed
@cristiand391 cristiand391 deleted the sm/frontdoor-post branch January 10, 2024 15:22
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.

3 participants