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.

Sign up for GitHub

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

Jump to bottom

factorio: download using token, not password #46586

Merged
merged 2 commits into from Sep 22, 2018
Merged

factorio: download using token, not password #46586

merged 2 commits into from Sep 22, 2018

Conversation

elitak
Copy link
Contributor

@elitak elitak commented Sep 12, 2018 •
edited

Downloads were broken by upstream devs' addition of CAPTCHA to the login form.

Changed all references of password to token and used the new GET request + query string to fetch the files.

Fixes #46267

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@elitak elitak mentioned this pull request Sep 12, 2018
Closed
nbp
nbp reviewed Sep 12, 2018
View reviewed changes
pkgs/games/factorio/fetch.sh Outdated
{
packageOverrides = pkgs: rec {
factorio = pkgs.factorio.override {
username = "<username or email address>";
Copy link
Member

Choose a reason for hiding this comment

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

nit: Insert a comment saying that this is confidential data and that it should not be shared.
follow-up: Use overlays instead of packageOverrides.

brendan-hall
brendan-hall reviewed Sep 14, 2018
View reviewed changes
pkgs/games/factorio/fetch.sh Outdated
echo <<EOF
Fetch failed.
Please ensure you have set the username and token with config.nix,'
or /etc/nix/nixpkgs-config.nix if on NixOS.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we point out that your token can be found at https://www.factorio.com/profile

@elitak
Copy link
Contributor Author

elitak commented Sep 15, 2018

We can do without the customized fetchurl now, but I would like to find a way to inject the error message. The only method I can think of is to nest overrideDerivation alongside override to add a postBuild hook, but that gets pretty messy.

@elitak
Copy link
Contributor Author

elitak commented Sep 15, 2018 •
edited

I figured out I should use preHook and failureHook. I added the info about factorio.com/profile. I will squash these commits after review, but they are close to final, now.

note to self: separate version bump into its own commit

@elitak
Copy link
Contributor Author

elitak commented Sep 15, 2018 •
edited

fetchurl happily downloads the redirected login page on bad credentials, and the derivation then fails on the bad hash. I don't know how to avoid this easily, so I just put a shorter WARNING message in.

I will ask upstream to return 401 instead.

@Mic92
Copy link
Member

Mic92 commented Sep 18, 2018

@GrahamcOfBorg eval

@elitak
factorio: download using token, not password
ff22306 
Downloads were broken by upstream devs' addition of CAPTCHA to the login
form. We now need only a slightly modified fetchurl to retrieve the
binary distribution.
@elitak
factorio: experimental=true includes stable builds, when newer
8ed49d7
@elitak
Copy link
Contributor Author

elitak commented Sep 20, 2018

I removed the warning now that 403 is returned, and changed the semantics of the experimental package function option to include stable builds.

@GrahamcOfBorg eval

@joachifm
Copy link
Contributor

Is this ready?

@elitak
Copy link
Contributor Author

elitak commented Sep 22, 2018

Yes.

@joachifm joachifm merged commit 9fee74d into NixOS:master Sep 22, 2018
@elitak elitak deleted the factorio branch December 29, 2018 08:41
@elitak elitak restored the factorio branch December 29, 2018 08:41
@elitak elitak deleted the factorio branch December 29, 2018 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbp nbp nbp left review comments

@brendan-hall brendan-hall brendan-hall left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@elitak @Mic92 @joachifm @nbp @brendan-hall @GrahamcOfBorg