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

Implement wasmer whoami #3316

Merged
merged 17 commits into from
Nov 21, 2022
Merged

Implement wasmer whoami #3316

merged 17 commits into from
Nov 21, 2022

Conversation

fschutt
Copy link
Contributor

@fschutt fschutt commented Nov 17, 2022

Note: has to be rebased on the wasmer login branch, otherwise the integration test won't work

@fschutt fschutt requested a review from syrusakbary as a code owner November 17, 2022 10:35
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

Is there a different way to check whether the whoami command succeeds without requiring access to the ciuser token?

Developers shouldn't have access to the ciuser token because it means they can masquerade as a system process (which screws up the audit trail), and that token has admin rights on WAPM so we'd be screwed if the wrong people got their hands on it.

lib/cli/src/cli.rs Outdated Show resolved Hide resolved
lib/cli/src/commands/whoami.rs Show resolved Hide resolved
lib/registry/src/lib.rs Outdated Show resolved Hide resolved
lib/registry/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 84 to 129
#[test]
fn run_whoami_works() -> anyhow::Result<()> {
let ciuser_token = std::env::var("WAPM_DEV_TOKEN").expect("no CIUSER / WAPM_DEV_TOKEN token");

let output = Command::new(get_wasmer_path())
.arg("login")
.arg("--registry")
.arg("wapm.dev")
.arg(ciuser_token)
.output()?;

if !output.status.success() {
bail!(
"wasmer login failed with: stdout: {}\n\nstderr: {}",
std::str::from_utf8(&output.stdout)
.expect("stdout is not utf8! need to handle arbitrary bytes"),
std::str::from_utf8(&output.stderr)
.expect("stderr is not utf8! need to handle arbitrary bytes")
);
}

let output = Command::new(get_wasmer_path())
.arg("whoami")
.arg("--registry")
.arg("wapm.dev")
.output()?;

let stdout = std::str::from_utf8(&output.stdout)
.expect("stdout is not utf8! need to handle arbitrary bytes");

if !output.status.success() {
bail!(
"linking failed with: stdout: {}\n\nstderr: {}",
stdout,
std::str::from_utf8(&output.stderr)
.expect("stderr is not utf8! need to handle arbitrary bytes")
);
}

assert_eq!(
stdout,
"logged into registry \"https://registry.wapm.dev/graphql\" as user \"ciuser\"\n"
);

Ok(())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this test is a good idea.

It assumes the $WAPM_DEV_TOKEN variable is set and that user's name is ciuser, so the only way for integration tests to pass locally is if you have the ciuser token on your computer, which is a massive no-no (no human developers should have access to CI tokens).

We're also trying to contact wapm.dev, which makes the test slow and flaky (e.g. wapm.dev could be having a bad day and always error out).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah well with network requests, that's kind of a given, otherwise it wouldn't be an integration test. In the worst case the test fails because wapm.dev is down, then we just need to restart the tests on GitHub.

The other part, I think I can require WAPM_DEV_TOKEN if GITHUB_TOKEN is present (true for integration tests in the CI, false for running tests locally).

Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

I think checking for $GITHUB_TOKEN and assuming the test passes is a pretty good trade-off for now 👍

@Michael-F-Bryan
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 21, 2022

Build succeeded:

@bors bors bot merged commit f503052 into master Nov 21, 2022
@bors bors bot deleted the whoami branch November 21, 2022 13:10
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