Skip to content

Oauth for mozilla#323

Merged
luser merged 4 commits intomozilla:masterfrom
aidanhs:aphs-dist-sccache-oauth-mozilla
Oct 25, 2018
Merged

Oauth for mozilla#323
luser merged 4 commits intomozilla:masterfrom
aidanhs:aphs-dist-sccache-oauth-mozilla

Conversation

@aidanhs
Copy link
Copy Markdown
Contributor

@aidanhs aidanhs commented Oct 23, 2018

If you test with the dist-tests feature on Linux, it will do the selenium tests in docker.

I also rolled in the suggested extraction of token checking into different functions.

@luser
Copy link
Copy Markdown
Contributor

luser commented Oct 23, 2018

The Travis build failure looks like a missing crate declaration in the dist code:

error[E0432]: unresolved import `crypto`
  --> src/dist/client_auth.rs:70:9
   |
70 |     use crypto;
   |         ^^^^^^ no `crypto` in the root
error[E0433]: failed to resolve. Maybe a missing `extern crate crypto;`?

@aidanhs
Copy link
Copy Markdown
Contributor Author

aidanhs commented Oct 23, 2018

Never tested with --no-default-features before! Fixed now

where T: CommandCreatorSync
{
trace!("detect_compiler");
trace!("detect_compiler: {}", executable.display());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You know, I was just looking at a log file from our CI the other day and realizing that this log message was not very informative. :)

extern crate assert_cmd;
extern crate chrono;
extern crate env_logger;
extern crate escargot;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious--what does escargot buy us over the existing assert_cmd usage?

access_information: CISProfileV2AccessInformation,
}
let profile: CISProfileV2 = res.json().chain_err(|| "Cannot decode CIS V2 profile from person API")?;
let is_allowed = profile.access_information.ldap.value.into_iter().any(|item| item.name == "hris_dept_firefox");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not 100% confident that this is the check we want here, but maybe it's close enough for now? I don't know if "is a MoCo employee" would be considered too broad? We should probably ask the security folks for clarification.

@luser
Copy link
Copy Markdown
Contributor

luser commented Oct 25, 2018

I won't claim that I've understood the meat of the authentication code here, but I've given everything a once-over and don't see anything that worries me so I'm going to merge this. If we discover problems in testing we can land fixes in separate PRs.

@luser luser merged commit 06b9999 into mozilla:master Oct 25, 2018
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