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: add auth:accesstoken:store command #165

Merged
merged 29 commits into from
Jun 11, 2021
Merged

Conversation

peternhale
Copy link
Contributor

@peternhale peternhale commented May 14, 2021

What does this PR do?

Introduces a new command auth:accesstoken:store

What issues does this PR fix or reference?

@W-8066452@

Command still needs Review Board approval
THIS PR DEPENDS ON sfdx-core PR feat: modify authInfo to return username for access token auth #405

maggiben and others added 6 commits May 14, 2021 12:27
* feat: identify scratch orgs if hub is known during auth

* chore: remove codecov

* chore: clean up github folder

* refactor: don't use prod-like loginUrl to exit

* chore: prune unnecessary ts-comment

* test: temporarily remove scratch-org-id to verify tests

* style: typos

* test: improve logging, temporarily use console

* chore: back to regular logging

* test: stubs for authinfo.hasAuthentications

* style: typos

* test: try single test

* test: return a promise?

* test: check authentications from instance

* test: stub method used by hasAuthentications

* test: run all the tests again

* test: try grant using stubbed listAll

* test: stub listAll for web:login

* test: store uses stub

* style: better error messages

* style: typo in nut name!

* refactor: changes from review comments

* refactor: getFields from authinfo in scratch-id

* Revert "refactor: getFields from authinfo in scratch-id"

This reverts commit c914648.

* style: comment for why we pass in the fields object

* fix: use decrypted fields, not result of getFields
@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @SF-CLI-BOT to sign the Salesforce.com Contributor License Agreement.

src/commands/auth/accesstoken/store.ts Outdated Show resolved Hide resolved
src/commands/auth/accesstoken/store.ts Outdated Show resolved Hide resolved
src/common.ts Show resolved Hide resolved
test/commands/auth/device/login.test.ts Outdated Show resolved Hide resolved
"invalidInstanceUrl": "Invalid instance URL. Specify a Salesforce instance URL using the format <domainname>.salesforce.com",
"accessTokenStdin": "Access token of user to use for authentication",
"noPrompt": "do not prompt for confirmation",
"overwriteAuthFileYesNo": "An authorization file exists for user \"%s\". Are you sure you want to save the file?\n\nSave the user (y/n)?"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little unclear about these two questions. Are you asking whether the user wants to overwrite a file? If so, maybe ask this as the first question:

"Are you sure you want to overwrite the existing file?"

And I don't know what "Save the user" means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jshackell-sfdc I like that better. Will change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jshackell-sfdc I am reusing an existing message here. If I change it, it will affect the other commands as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Let's just leave it. Thx

Copy link
Contributor

Choose a reason for hiding this comment

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

Although, I still think it'll be confusing to the user. But I also don't want to suggest changing a message that applies to other commands without knowing what those commands are. Could you create a new message for just this situation? I don't know how you all deal with these things. I leave it up to you. With the caveat that I'm not thrilled with the message if you leave it as-is :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a new message.

Copy link
Contributor

@jshackell-sfdc jshackell-sfdc left a comment

Choose a reason for hiding this comment

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

See my small edits, one is optional. But I noticed a few misspellings so those need to be addressed for sure. Thx!

const messages = Messages.loadMessages('@salesforce/plugin-auth', 'accesstoken.store');
const commonMessages = Messages.loadMessages('@salesforce/plugin-auth', 'messages');

const ACCESS_TOKEN_FORMAT1 = '"<org id>!<accesstoken>"';
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: is there an ACCESS_TOKEN_FORMAT2? why is there the 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WillieRuemmele, access tokens do follow the above format. Copy/paste error ;)
I'll fix that

@WillieRuemmele WillieRuemmele merged commit 3f7d75a into main Jun 11, 2021
@WillieRuemmele WillieRuemmele deleted the phale/W-8066452 branch June 11, 2021 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants