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

PDE-5319 feat(cli): invoke auth start subcommand #872

Merged
merged 16 commits into from
Oct 9, 2024

Conversation

eliangcs
Copy link
Member

@eliangcs eliangcs commented Sep 26, 2024

Changes:

  • add zapier invoke auth start subcommand
  • add support for --non-interactive flag to zapier invoke command
  • document zapier invoke better
  • remove Node.js 16 from CI tests and add Node.js 20
    • did it because the open package added in this PR requires Node.js 18
    • had to upgrade mock-fs along the way because the old version wasn't working on Node.js 20 😠

The auth start subcommand works with the following authentication types:

  • oauth2
  • basic
  • custom
  • session

Authentication types oauth1 and digest are rare, so I'm leaving them out of this PR for now.

For oauth2, auth start does following:

  1. Check if CLIENT_ID and CLIENT_SECRET are defined in .env. If not, prompt for them.
  2. Start a local HTTP server waiting for the redirect request
  3. Open the browser with authentication.oauth2Config.authorizeUrl
  4. Get the code parameter in the redirect request and call authentication.oauth2Config.getAccessToken to get the access token
  5. Save the access token to .env

For basic and custom, auth start simply prompts for the auth fields.

For session, auth start prompts for the required auth fields and calls authentication.sessionConfig.perform to get the computed auth field(s) (e.g. a session key).

@eliangcs eliangcs changed the title PDE-5319 feat(cli): invoke auth subcommand PDE-5319 feat(cli): invoke auth start subcommand Sep 26, 2024
@eliangcs eliangcs marked this pull request as ready for review October 4, 2024 10:28
@eliangcs eliangcs requested a review from a team as a code owner October 4, 2024 10:28
standielpls
standielpls previously approved these changes Oct 7, 2024
Copy link
Contributor

@standielpls standielpls left a comment

Choose a reason for hiding this comment

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

I tested custom and oauth2, and they work! Nice job! Excited for this.
I have a pair of non-blocking comments

}

async startBasicAuth(authFields) {
if (this.nonInteractive) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does basic auth need interactive mode?
Is it for censoring the password?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically we want to make sure the CLI doesn't ask interactive questions in non-interactive mode (nonInteractive === true). For basic auth, the only thing that auth start does is ask for username and password interactively. auth start with basic auth doesn't really make sense in non-interactive mode, which is why we throw an error here.

Does that make sense?

Copy link
Member Author

@eliangcs eliangcs Oct 8, 2024

Choose a reason for hiding this comment

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

In case you're asking why we're not allowing this non-interactive use case for basic auth:

echo '{"username":"user","password":"secret"}' | zapier invoke auth start

We could but I thought that would be useless, since it'd be easier for the user to edit .env or append the auth fields to .env:

echo 'authData_username=user' >> .env
echo 'authData_password=secret' >> .env

});

endSpinner();
startSpinner('Opening browser to authorize');
Copy link
Contributor

Choose a reason for hiding this comment

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

I got stuck at authorizing bc of bad params, I reckon it's hard to kill this process if the browser request errors?
Do you think it's valuable to set a timer for the code promise or provide 'ctrl/cmd + c' to abort helper?

Copy link
Member Author

@eliangcs eliangcs Oct 8, 2024

Choose a reason for hiding this comment

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

If the partner server encounters an error and doesn't redirect to localhost, we won’t be able to detect the error. I prefer not to use a timer for this. I think the best approach would be to let the user know they can press CTRL-C to exit if an error occurs. I've added this in 9929e9c.

CTRL-C already works on my machine. Does it work on yours? @standielpls

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it does, thank you for adding the hint text

@eliangcs eliangcs merged commit 9f159fb into main Oct 9, 2024
14 checks passed
@eliangcs eliangcs deleted the PDE-5319-invoke-auth-start branch October 9, 2024 03:56
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