Change login flow (experimental) #223

campionfellin · 2018-06-19T01:33:33Z

This is the flow (more or less):

clasp login: logs you in using the default clasp credentials. Saves .clasprc.json to your ~ directory.
clasp login --creds: logs you in using the file credentials.json in the directory that you are running the command in. Saves .clasprc.json to your current directory.
clasp login --creds other_creds.json: logs you in using the credentials in other_creds.json. Saves .clasprc.json to your current directory.

Your credentials file should look like this:

{"installed":{"client_id":"239267426989-275aglft6htcfsdj7t5d3csdlogchamh.apps.googleusercontent.com","project_id":"project-id-xxxxxxxxx","auth_uri":"https://accounts.google.com/o/oauth2/auth","token_uri":"https://accounts.google.com/o/oauth2/token","auth_provider_x509_cert_url":"https://www.googleapis.com/oauth2/v1/certs","client_secret":"xxxxxxxx","redirect_uris":["urn:ietf:wg:oauth:2.0:oob","http://localhost"]}}

Signed-off-by: campionfellin [email protected]

Related to #204

npm run test succeeds.
npm run lint succeeds.
Appropriate changes to README are included in PR.

Signed-off-by: campionfellin <[email protected]>

campionfellin · 2018-06-19T01:33:48Z

Please test before merging.

coveralls · 2018-06-19T01:37:47Z

Coverage decreased (-0.2%) to 15.507% when pulling 1c5f9fa on campionfellin:new-login into bd177b4 on google:master.

campionfellin · 2018-06-19T03:37:40Z

(also note that npm run docs did not work for some reason to update the docs with this)

marcosscriven · 2018-06-20T13:38:19Z

@campionfellin - would this help with #225? Would like to be able to use clasp to upload script from CI, without any interaction required.

campionfellin · 2018-06-20T14:18:59Z

For now, you still have to use the browser to confirm the login. What this does is allow you to authenticate with your own Oauth client, rather than the default for clasp. This helps with things like clasp run which need more permissions than the default.

campionfellin · 2018-06-21T00:22:54Z

@grant though this is related to #225 I think it belongs more with #204 and should be merged. We can continue discussion on #225 to see what to do next.

grant

Can we update the README too? (Hand editing it is fine)
LGTM after nits and README.

grant · 2018-06-21T17:16:34Z

src/auth.ts

+    oauth2ClientSettings.clientId = credentials.installed.client_id;
+    oauth2ClientSettings.clientSecret = credentials.installed.client_secret;
+    ownCreds = true;
+    console.log('Credentials found, using those to login.');


Put all messages in LOG.

grant · 2018-06-21T17:18:46Z

src/auth.ts

@@ -143,15 +154,15 @@ async function authorizeWithoutLocalhost() {
 * Logs the user in. Saves the client credentials to an rc file.
 * @param options the localhost and ownkey options from commander


It would be cool if we documented the options.

* @param {object} options Login options * @param {boolean} options.localhost ... * @param {string} options.creds ...

grant · 2018-06-21T17:23:25Z

src/auth.ts

@@ -53,10 +54,20 @@ export const drive = google.drive({
 * @param {boolean} useLocalhost True if a local HTTP server should be run
 *     to handle the auth response. False if manual entry used.
 */
-async function authorize(useLocalhost: boolean, writeToOwnKey: boolean) {
+async function authorize(useLocalhost: boolean, creds: string) {


I think we forgot to document @param {string} creds in a previous PR. Can we add it here?

grant · 2018-06-21T17:23:29Z

src/auth.ts

+  let ownCreds = false;
+  try {
+    const credentials = JSON.parse(fs.readFileSync(creds, 'utf8'));
+    oauth2ClientSettings.clientId = credentials.installed.client_id;


We should probably have some error handling to check if our credentials file is in the right format and has the right keys.

If credentials, credentials. installed, credentials. installed.client_id, credentials. installed.client_secret, then save them.

Otherwise, log an error and exit.

Signed-off-by: campionfellin <[email protected]>

campionfellin · 2018-06-21T21:21:38Z

Will update README in separate PR.

grant · 2018-06-21T21:24:21Z

src/auth.ts

 */
-async function authorize(useLocalhost: boolean, writeToOwnKey: boolean) {
+async function authorize(useLocalhost: boolean, creds: any) {


Can we never have any?
string|boolean?

The main reason for doing this is this line:

creds = creds === true ? 'credentials.json' : creds;

and the reason for that was because if you put a default value in commander, it sets options.creds to that value whether or not the user entered in the --creds flag.

So I removed the default value from the commander side of things and use this line to set it to credentials.json by default.

Can you just do string and default creds outside the body (in the caller)?

There has to be some way to not use any.
This is an internal method too, so we have flexibility to our method signature.

grant

We can't have any more any.
There's got some way to fix that, with stricter options, union, a generic or something.

grant · 2018-06-22T06:07:11Z

src/auth.ts

-async function authorize(useLocalhost: boolean, writeToOwnKey: boolean) {
+async function authorize(useLocalhost: boolean, creds: any) {
+  let ownCreds = false;
+  creds = creds === true ? 'credentials.json' : creds;


Never write === true
Just creds ? creds : '...';

Strings evaluate to true so when they enter --creds otherCredsFile.json then we would rename it to credentials.json unfortunately. I see the other comments and there is probably a way to do this without using this or any so I'll look closer at the commander documentation.

grant · 2018-06-22T06:09:27Z

src/auth.ts

 */
-async function authorize(useLocalhost: boolean, writeToOwnKey: boolean) {
+async function authorize(useLocalhost: boolean, creds: any) {


Can you just do string and default creds outside the body (in the caller)?

There has to be some way to not use any.
This is an internal method too, so we have flexibility to our method signature.

campionfellin · 2018-06-22T17:00:03Z

Ok, after some testing with it, I think the best solution is to remove the default value and make it so if the user uses --creds they must supply a credentials file. The reasoning is this:

If we default it to ./credentials.json, or anything really, when the user uses clasp login without specifying --creds, the options.creds variable is still set to ./credentials.json. Thus, if they have that file in their directory, but want to login globally with the default (old) clasp credentials, they can't.

@grant what do you think?

grant · 2018-06-22T17:28:32Z

Agreed, that makes sense. Let's not default.

Signed-off-by: campionfellin <[email protected]>

campionfellin · 2018-06-22T17:52:04Z

Ok, now here is what happens:

clasp login : works as it used to, using the default clasp credentials.
clasp login --creds : gives error error: option '--creds <file>' argument missing
clasp login --creds creds.json (when creds.json exists and is formatted properly) : logs in user with those credentials, saves ./.clasprc.json
clasp login --creds creds.json (when creds.json doesn't exist): Credentials file not found.
clasp login --creds creds.json (when creds.json exists, but not right format) : Incorrect credentials file format.

campionfellin · 2018-06-22T17:52:48Z

Important to note when we update the readme:

If you use your own credentials from GCP, you must enable both the Apps Script API and Drive API.

grant · 2018-06-22T17:53:38Z

It would be cool if you added @example with the comment you wrote above, so it lasts (here).

Signed-off-by: campionfellin <[email protected]>

campionfellin · 2018-06-22T17:56:32Z

Added simple examples, I think the more detailed "what happens if I do this..." should be in the README under troubleshooting.

grant · 2018-06-25T05:41:49Z

I'll try to release this week with these changes.

Change login flow (experimental)

64b5301

Signed-off-by: campionfellin <[email protected]>

googlebot added the cla: yes label Jun 19, 2018

campionfellin added the do not merge label Jun 19, 2018

campionfellin mentioned this pull request Jun 20, 2018

How to use a service account for CI deployments #225

Closed

campionfellin removed the do not merge label Jun 21, 2018

grant reviewed Jun 21, 2018

View reviewed changes

Fix nits

319ab8b

Signed-off-by: campionfellin <[email protected]>

grant approved these changes Jun 21, 2018

View reviewed changes

grant reviewed Jun 21, 2018

View reviewed changes

grant reviewed Jun 22, 2018

View reviewed changes

campionfellin added the do not merge label Jun 22, 2018

Fix nits, remove default credentials

1627b82

Signed-off-by: campionfellin <[email protected]>

grant approved these changes Jun 22, 2018

View reviewed changes

campionfellin removed the do not merge label Jun 22, 2018

Add example

1c5f9fa

Signed-off-by: campionfellin <[email protected]>

grant merged commit 0ce9d5d into google:master Jun 25, 2018

campionfellin deleted the new-login branch August 15, 2018 04:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change login flow (experimental) #223

Change login flow (experimental) #223

campionfellin commented Jun 19, 2018

campionfellin commented Jun 19, 2018

coveralls commented Jun 19, 2018 •

edited

Loading

campionfellin commented Jun 19, 2018

marcosscriven commented Jun 20, 2018 •

edited

Loading

campionfellin commented Jun 20, 2018

campionfellin commented Jun 21, 2018

grant left a comment

grant Jun 21, 2018

grant Jun 21, 2018

grant Jun 21, 2018

grant Jun 21, 2018

campionfellin commented Jun 21, 2018

grant Jun 21, 2018

campionfellin Jun 21, 2018

campionfellin Jun 21, 2018

grant Jun 22, 2018

grant left a comment

grant Jun 22, 2018

campionfellin Jun 22, 2018

grant Jun 22, 2018

campionfellin commented Jun 22, 2018

grant commented Jun 22, 2018

campionfellin commented Jun 22, 2018

campionfellin commented Jun 22, 2018

grant commented Jun 22, 2018 •

edited

Loading

campionfellin commented Jun 22, 2018

grant commented Jun 25, 2018

		@@ -143,15 +154,15 @@ async function authorizeWithoutLocalhost() {
		* Logs the user in. Saves the client credentials to an rc file.
		* @param options the localhost and ownkey options from commander

Change login flow (experimental) #223

Change login flow (experimental) #223

Conversation

campionfellin commented Jun 19, 2018

campionfellin commented Jun 19, 2018

coveralls commented Jun 19, 2018 • edited Loading

campionfellin commented Jun 19, 2018

marcosscriven commented Jun 20, 2018 • edited Loading

campionfellin commented Jun 20, 2018

campionfellin commented Jun 21, 2018

grant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

campionfellin commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

campionfellin commented Jun 22, 2018

grant commented Jun 22, 2018

campionfellin commented Jun 22, 2018

campionfellin commented Jun 22, 2018

grant commented Jun 22, 2018 • edited Loading

campionfellin commented Jun 22, 2018

grant commented Jun 25, 2018

coveralls commented Jun 19, 2018 •

edited

Loading

marcosscriven commented Jun 20, 2018 •

edited

Loading

grant commented Jun 22, 2018 •

edited

Loading