-
Notifications
You must be signed in to change notification settings - Fork 337
Add credential checking logic when auth params are provided in wrangler config #842
Conversation
src/commands/config/mod.rs
Outdated
GlobalUser::GlobalKeyAuth { .. } => { | ||
match client.request(&GetUserDetails {}) { | ||
Ok(_) => Ok(()), | ||
Err(e) => failure::bail!("Auth check failed. Please make sure your email and global API key pair are correct. \n{}", http::format_error(e, ErrorCodeDetail::None)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we link here (and above) to the docs about finding your credentials? i wouldn't link anywhere in the dashboard because we don't control those, but the docs should be pretty consistent. open to being wrong about this, not blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weirdly enough, the best docs are in the help center (there's not a comprehensive set of API token docs in the v4 API documentation yet...). I can add a link to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have something in the workers wrangler quick start. if it doesn't already exist, we should update it to include api token instructions as part of this project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the above link is the most stable atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link above seems to have outdated documentation, I'm going to open up a workers docs ticket to fix that (as well as add a section for api tokens). I'll omit the link from Wrangler for now, but will add them when the docs are ready!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it is fine to include the link, given the location will not change, only the content, and the content must be updated before we release. one less loose thread to track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm game to change the link to be a general https://developers.cloudflare.com/workers/quickstart/#authentication link (note: this endpoint currently doesn't exist). This link would contain information about both API tokens and API keys... what do you think? In the meantime I'll add the existing link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah that seems like the way to go. yeah add the existing link, write an issue for us here, and an accompanying issue for docs (cc @victoriabernard92 ) and this is good to merge.
suggestion: for PRs that depend on other PRs because they have the same commits, can you pull against the upstream PR? it makes reviewing easier. so in this case, pull this against |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Credential validating looks great! I've wanted this for a long time :) Couple questions about overall refactor
EDIT: I changed the base PR that this merges into because the diff included a lot of unrelated stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing to approved because this PR does not include unrelated code like I thought it did!
An update to this PR: I also add support for a Since this test calls There is a world, however, where we could make a testing account for wrangler and provide its credentials... but we'd be storing those as plaintext in our tests, which is not good from a security practice. |
Flag seems great! FWIW I think we would want to make mocks for our API calls before we would make a testing account. |
yeah i'd be more interested in writing unit tests with mocked api clients; integration tests are important, but i always question including api requests in fully-automated CI test suites. this is an ongoing struggle for me. |
Closes #439
Note: This PR will fail to build until cloudflare/cloudflare-rs#58 and cloudflare/cloudflare-rs#59 are merged into cloudflare-rs and a cloudflare-rs release is cut. This should also only be merged after #841 is merged.
This PR validates credentials given in
wrangler config
by testing them against the Cloudflare API. If credentials are invalid, users are asked to check their token or email/api key pair and make sure they input what they expected.Example output:
^ no emoji because linux :(