OAuth integration for Wrangler login #2048

ocsfrank · 2021-08-30T17:29:09Z

Still in staging, but it should be ready for review.

Please also ignore the cargo.toml, I will update it once the service goes to prod. The CI test are failing because of oauth2 library.

…frank/oauth

threepointone · 2021-09-03T17:02:41Z

This appears to be working well! Some feedback:

when it opens the browser, we log the browser url in the terminal. It's super verbose and feels unnecessary. @ocsfrank mentioned how it's for when the browser fails to open, which is a possibility in wsl. As a compromise, I suggest we only log the url in wsl.
I manually edited the token and deleted the characters, then when I run wrangler dev I get this message

but I would've expected the "invalid/corrupt token" messaging we recently added. This should probably be fixed? It shouldn't block landing this PR, it can happen in a followup PR.

Doing just a written approval here for the working, feel free to land after a code review.

src/commands/whoami.rs

…into frank/oauth

…frank/oauth

ocsfrank · 2021-09-03T21:18:28Z

src/login/mod.rs

-
-    rsa.private_decrypt(&encrypted_token, &mut token_bytes, Padding::PKCS1)?;
-    let token = str::from_utf8(&token_bytes)?.trim_matches(char::from(0));
+    if wsl::is_wsl() {


Is there a better way to check if the current process is run on WSL without having to include another dependency?

I wonder if we should just print this URL out despite its verbosity.. I get @threepointone's point for sure and generally agree.. but what about the case where users launch wrangler from within a Docker container? Probably not handled by this WSL crate, but wouldn't be able to open a host browser window either.

nilslice · 2021-09-07T22:22:59Z

src/commands/dev/edge/mod.rs

+        let mut user = user.clone();
+
+        // Check if oauth token is expired
+        let _res = check_update_oauth_token(&mut user).expect("Failed to refresh access token");


While it isn't likely that the user will be able to do anything if the expect is reached from an Err value, we may want to consider letting the dev caller handle this error or print something more useful here. "Failed to refresh access token" isn't as informative as whatever might come back from the API for this call.

@ocsfrank could you dig in a bit and see what other options are available here? The end-goal is providing as much to the wrangler user as possible for debugging.. as this stands, we just print this panic message and maybe a backtrace before suggesting they wrangler report the issue. But, it's unlikely that we can help them, or that they can help themselves without more info.

nilslice · 2021-09-07T22:25:36Z

src/commands/logout.rs

+pub fn run() -> Result<()> {
+    let user_create = GlobalUser::new();
+    let mut has_auth = true;
+    if let Ok(user) = user_create {


can any/all of these be written as if let Ok(user) = GlobalUser::new() { ... } ? Doesn't look like you need the user_create binding anywhere. (I easily could be missing it though)

nilslice · 2021-09-07T22:29:27Z

src/commands/whoami.rs

+    missing_permissions: &mut Vec<String>,
+    token_type: &str,
+) -> Result<String> {
+    let token_auth_email = fetch_auth_token_email(user, missing_permissions)?;


Similarly here, I think could be shortened to if let Some(token_auth_email) = fetch_auth_token_email(user, missing_permissions)? { ... }

nilslice · 2021-09-07T22:31:05Z

src/commands/whoami.rs

-                "an API Token".to_string()
-            }
+        GlobalUser::ApiTokenAuth { .. } => get_token_type(user, &mut missing_permissions, "API")
+            .expect("Failed to get Api token type."),


s/Api/API

Also, I think this and the following match arm should bubble up this error, or print more of the returned error's context if its available. The panic will not be very useful, and an error report via wrangler report isn't likely to be resolved by our team.

nilslice · 2021-09-07T22:53:25Z

src/login/mod.rs

-
-    rsa.private_decrypt(&encrypted_token, &mut token_bytes, Padding::PKCS1)?;
-    let token = str::from_utf8(&token_bytes)?.trim_matches(char::from(0));
+    if wsl::is_wsl() {


I wonder if we should just print this URL out despite its verbosity.. I get @threepointone's point for sure and generally agree.. but what about the case where users launch wrangler from within a Docker container? Probably not handled by this WSL crate, but wouldn't be able to open a host browser window either.

nilslice · 2021-09-07T22:57:07Z

src/login/mod.rs

-    let user = GlobalUser::TokenAuth {
-        api_token: token.to_string(),
+    // Get authorization code and CSRF state from local HTTP server
+    let params_response = match block_on(http_server_get_params()) {


any reason in particular to use the executor from futures here rather than tokio (asking because it looks like you're using tokio elsewhere) ? Not saying it doesn't work, but I think this means we're using different runtimes for various async operations here.

nilslice · 2021-09-07T23:05:18Z

src/login/mod.rs

+            if result.is_err() {
+                log::debug!("Failed to invalidate OAuth token before login.");
+            }
+        }
    }
 }


Generally in this file src/login/mod.rs (besides tests), there is a lot of unwrap and expect usage. I think some of these are in places that will create a lot of noise from wrangler report usage, and could be cleaned up a bit. Consider some ? usage in place of these where returning the error (even if it's printed later on) could give the user some way of debugging an issue.

Many of them might be fine. Just adding a comment here to suggest taking another pass at error handling along this path.

nilslice · 2021-09-07T23:07:37Z

src/settings/global_user.rs

+        // Check if oauth token is expired
+        if let Ok(ref mut oauth_user) = new_user {
+            let _res =
+                check_update_oauth_token(oauth_user).expect("Failed to refresh access token");


Another case to consider more useful error handling / context-sharing with the end user.

nilslice · 2021-09-07T23:13:25Z

src/settings/global_user.rs

+            || ((oauth_token.is_ok() || refresh_token.is_ok() || expiration_time.is_ok())
+                && (email.is_ok() || api_key.is_ok()))
+        {
+            let error_info = "\nMore than one authentication method (e.g. API token and OAuth token, or OAuth token and Global API key) has been found in the configuration file. Please use only one.";


We can probably add more detail to this error:

print the path to the config file in case they want to manually edit it

suggest running wrangler logout (assuming this clears all auth info on disk?)

nilslice · 2021-09-07T23:14:47Z

src/settings/global_user.rs

+        } else if api_token.is_ok() {
+            Ok(Self::ApiTokenAuth {
+                api_token: api_token.expect("Failed to read API token"),
+            })
+        } else if email.is_ok() && api_key.is_ok() {
+            Ok(Self::GlobalKeyAuth {
+                email: email.expect("Failed to read email"),
+                api_key: api_key.expect("Failed to read api_key"),
+            })
+        } else if oauth_token.is_ok() && refresh_token.is_ok() && expiration_time.is_ok() {
+            Ok(Self::OAuthTokenAuth {
+                oauth_token: oauth_token.expect("Failed to read OAuth token"),
+                refresh_token: refresh_token.expect("Failed to read OAuth refresh token"),
+                expiration_time: expiration_time
+                    .expect("Failed to read access token expiration time"),


I think all of these expect calls can just be unwrap, since you've already attested the are Ok in the if checks

nilslice · 2021-09-07T23:15:46Z

src/settings/global_user.rs

+            })
+        } else {
+            // Empty configuration file and no environment variables, or missing variable for global API key
+            let error_info = "\nNo (valid) authentication method has been found.";


Could add more detail here too.. could running wrangler login help? let's offer some suggestions for a user to try.

Yes! That would help, and it's being printed in the next line with the call to Self::show_config_err_info(Some(error_info.to_string()), config).

nilslice · 2021-09-09T22:05:23Z

src/login/http.rs

        }
    });

    // Receive authorization code and csrf state from HTTP server
-    let params = runtime.block_on(async {
+    let params = tokio::task::spawn(async move {
        match rx.recv().await {
            Some(values) => values,
            None => "error".to_string(),


since we have a history of difficult-to-debug channel errors, maybe we could be more descriptive and log this error (unless it is already being logged somewhere in the calling context).

do you know if it is possible that the rx can receive None at any point before it gets Some(values) and should therefore be polled?

Logging is a good idea. The only case that I am aware of is when the sender side is closed for any reason, and rx still tries to read something. In this case, it should receive None.

nilslice · 2021-09-09T22:08:54Z

src/settings/global_user.rs

            Ok(Self::ApiTokenAuth {
-                api_token: api_token.unwrap(),
+                api_token: api_token_value,
            })


Nit: these could be re-written to be a bit more concise:

} else if let Ok(api_token) = api_token { Ok(Self::ApiTokenAuth { api_token}) // ...

the same is true for all the branches below. if the variable binding (in this case inside the if let, shadowing the outer one) is the same as the field name, you can omit the field name and just pass in the variable name in its place. Not major, but might make the code a bit nicer to read.

I see! Will include these changes in the next commit.

nilslice

Nice work!

…frank/oauth

nataliescottdavidson

I have verified that the changes work as expected on my machine.

Mengqi Chen and others added 24 commits August 9, 2021 09:30

initial integration of oauth

f92a8a3

add more error info for config file

332a7b1

update cargos for dev

8bee708

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

8685f82

…frank/oauth

update global_user tests: replace panic with failure assertions

d4ab5db

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

c7bd354

…frank/oauth

add more comments and tests

ac4b5b0

update comments

c7aaa5b

add redirection and consent denied case

f3604c6

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

3ca2683

…frank/oauth

allow user to define scopes and add redirection

19d4f24

testing add scopes info, redirect success/failure pages

b0e12d3

refactor code and small updates

eb12f53

fmt

273f8f7

prepare for refresh tokens

d93a9d7

update whoami with refresh tokens

e74eae9

change refresh token method

e8fb459

add refresh tokens for all commands that need a user

4275eae

refactor

5662034

add refresh token to dev edge

29d4e6f

update readme

bd1fc28

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

3f9924b

…frank/oauth

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

64ceb95

…frank/oauth

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

f8065a8

…frank/oauth

ocsfrank requested review from nilslice, nataliescottdavidson and caass August 30, 2021 17:29

ocsfrank requested a review from a team as a code owner August 30, 2021 17:29

update endpoint_url to be configurable within wrangler

25e0309

nilslice mentioned this pull request Aug 31, 2021

Wrangler login not working when logged out of dashboard #2043

Closed

threepointone mentioned this pull request Sep 2, 2021

Improve wrangler dev error message for invalid tokens. #2060

Merged

Merge branch 'master' into frank/oauth

02866b4

nataliescottdavidson suggested changes Sep 3, 2021

View reviewed changes

src/commands/whoami.rs Outdated Show resolved Hide resolved

ocsfrank added 4 commits September 3, 2021 14:04

invalidate oauth tokens at each login, hide url for login

e68bbe1

Merge branch 'frank/oauth' of https://github.com/cloudflare/wrangler …

a31b338

…into frank/oauth

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

83d1a1d

…frank/oauth

remove old todo

ca85ed2

ocsfrank commented Sep 3, 2021

View reviewed changes

nilslice reviewed Sep 7, 2021

View reviewed changes

nilslice suggested changes Sep 7, 2021

View reviewed changes

ocsfrank added 6 commits September 8, 2021 16:40

WIP: apply some of the feedback

0ea035e

improve error handling in login module

948004b

resolve merge conflict

fa20b54

add more error handling in dev and global_user

fd29f2d

update login to use tokio, fix clippy, update error msg

0c0ecf2

update error message and handling from refreshing tokens

32fb954

nilslice reviewed Sep 9, 2021

View reviewed changes

ocsfrank added 2 commits September 9, 2021 16:04

update global_user syntax and log channel error in login

fe6fe20

Merge branch 'master' into frank/oauth

1f2fb12

nilslice approved these changes Sep 10, 2021

View reviewed changes

1.19.3-rc.1

6e2702d

threepointone approved these changes Sep 13, 2021

View reviewed changes

ocsfrank added 2 commits September 13, 2021 12:33

small refactor and invalidate oauth tokens before wrangler config

95d88de

Merge branch 'master' of https://github.com/cloudflare/wrangler into …

c3b74de

…frank/oauth

nataliescottdavidson approved these changes Sep 13, 2021

View reviewed changes

update readme authorize

d7255d2

ocsfrank merged commit 2416307 into master Sep 14, 2021

delete-merged-branch bot deleted the frank/oauth branch September 14, 2021 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAuth integration for Wrangler login #2048

OAuth integration for Wrangler login #2048

ocsfrank commented Aug 30, 2021 •

edited

threepointone commented Sep 3, 2021

ocsfrank Sep 3, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

nilslice Sep 7, 2021

ocsfrank Sep 8, 2021

nilslice Sep 9, 2021

ocsfrank Sep 9, 2021

nilslice Sep 9, 2021

ocsfrank Sep 9, 2021

nilslice left a comment

nataliescottdavidson left a comment

OAuth integration for Wrangler login #2048

OAuth integration for Wrangler login #2048

Conversation

ocsfrank commented Aug 30, 2021 • edited

threepointone commented Sep 3, 2021

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilslice left a comment

Choose a reason for hiding this comment

nataliescottdavidson left a comment

Choose a reason for hiding this comment

ocsfrank commented Aug 30, 2021 •

edited