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

Add pam option to enable Google Authenticator #34728

Merged
merged 1 commit into from Feb 10, 2018
Merged

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Feb 8, 2018

Motivation for this change

I couldn't find this option anywhere, and I used it on my Fedora, that I've replaced with NixOS (yay!), so I want it here as well.

Things done

(Not much yet, I am a total noob, please be gentle).

  • Works on my box
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md. (I think)

@@ -46,6 +46,16 @@ let
'';
};

google2FA = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. Would be googleAuthenticator be more self-explanatory then google2FA?

Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that there are other config settings that we might want to support later, it might make sense to "namespace" the config option like so:

google2FA = {
  enable = mkOption {
    default = false;
    type = types.bool;
    description = ''
      If set, users with enabled Google Authenticator (created
      <filename>~/.google_authenticator</filename>) will be required
      to provide Google Authenticator token to log in.
    '';
  };
};

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The additional namespace is a very good idea.

authenticator seemed like a longish word. I have very little idea about conventions here. I am happy to change it.

Copy link
Member

@Mic92 Mic92 Feb 9, 2018

Choose a reason for hiding this comment

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

For me it was more about being consistent with what upstream calls it.

@dpc
Copy link
Contributor Author

dpc commented Feb 10, 2018

I have amended the change according to your instructions.

@Mic92 Mic92 merged commit 79315b6 into NixOS:master Feb 10, 2018
@Mic92
Copy link
Member

Mic92 commented Feb 10, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants