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 a way to configure basic auth without storing passwords in plaintext in settings #2

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Rework gendigest script into fastapi_security.cli
  • Loading branch information
immerrr committed Dec 4, 2021
commit 6b791da2c95528bf5812e898fb3f485627fb42a2
93 changes: 93 additions & 0 deletions fastapi_security/cli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
"""fastapi_security command-line interface"""

import argparse
import sys
import textwrap
from getpass import getpass
from typing import Optional, Sequence, Text

from fastapi_security.basic import generate_digest


def _wrap_paragraphs(s):
paragraphs = s.strip().split("\n\n")
wrapped_paragraphs = [
"\n".join(textwrap.wrap(paragraph)) for paragraph in paragraphs
]
return "\n\n".join(wrapped_paragraphs)


main_parser = argparse.ArgumentParser(
description=_wrap_paragraphs(__doc__),
formatter_class=argparse.RawDescriptionHelpFormatter,
)
Comment on lines +20 to +23
Copy link
Owner

Choose a reason for hiding this comment

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

prog needs to be added for unit test change suggestion I'm making below to work.

Suggested change
main_parser = argparse.ArgumentParser(
description=_wrap_paragraphs(__doc__),
formatter_class=argparse.RawDescriptionHelpFormatter,
)
main_parser = argparse.ArgumentParser(
prog="fastapi-security",
description=_wrap_paragraphs(__doc__),
formatter_class=argparse.RawDescriptionHelpFormatter,
)

subcommand_parsers = main_parser.add_subparsers(
help="Specify a sub-command",
dest="subcommand",
required=True,
)

gendigest_description = """
Generate digest for basic_auth_with_digest credentials.

Example:

$ fastapi-security gendigest --salt=very-strong-salt
Password:
Confirm password:

Here is your digest:
0jFS-cNapwQf_lpyULF7_hEelbl_zreNVHbxqKwKIFmPRQ09bYTEDQLrr_UEWZc9fdYFiU5F3il3rovJQ_UEpg==

$ cat fastapi_security_conf.py
from fastapi_security import FastAPISecurity

security = FastAPISecurity()
security.init_basic_auth_with_digest(
[
{'user': 'me', 'password': '0jFS-cNapwQf_lpyULF7_hEelbl_zreNVHbxqKwKIFmPRQ09bYTEDQLrr_UEWZc9fdYFiU5F3il3rovJQ_UEpg=='}
],
salt='very-strong-salt',
)
"""

gendigest_parser = subcommand_parsers.add_parser(
"gendigest",
description=gendigest_description,
formatter_class=argparse.RawDescriptionHelpFormatter,
)
gendigest_parser.add_argument(
"--salt",
help="Salt value used in fastapi_security configuration.",
required=True,
)


def gendigest(parsed_args):
# if not parsed_args.salt:
# print("Cannot generate digest: --salt must be non-empty",
# file=sys.stderr)
# sys.exit(1)

password = getpass(prompt="Password: ")
password_confirmation = getpass(prompt="Confirm password: ")

if password != password_confirmation:
print("Cannot generate digest: passwords don't match", file=sys.stderr)
sys.exit(1)

print("\nHere is your digest:", file=sys.stderr)
print(generate_digest(password, salt=parsed_args.salt))


def main(args: Optional[Sequence[Text]] = None):
parsed_args = main_parser.parse_args(args)
if parsed_args.subcommand == "gendigest":
return gendigest(parsed_args)

main_parser.print_usage(file=sys.stderr)
sys.exit(2) # invalid usage: missing subcommand


if __name__ == "__main__":
main()
133 changes: 0 additions & 133 deletions fastapi_security/gendigest.py

This file was deleted.

3 changes: 3 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ classifiers = [
"Typing :: Typed",
]

[tool.poetry.scripts]
fastapi-security = 'fastapi_security.cli:main'

[tool.poetry.dependencies]
python = "^3.6"
aiohttp = "^3"
Expand Down
55 changes: 55 additions & 0 deletions tests/integration/test_cli.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import subprocess
from unittest import mock

from fastapi_security import cli


def test_usage_output_without_params():
result = subprocess.run(["fastapi-security"], capture_output=True)
assert result.returncode == 2
assert result.stdout.decode().splitlines() == []
assert result.stderr.decode().splitlines() == [
"usage: fastapi-security [-h] {gendigest} ...",
"fastapi-security: error: the following arguments are required: subcommand",
]

Comment on lines +7 to +15
Copy link
Owner

Choose a reason for hiding this comment

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

To fix failing tests you could call cli.main directly as I've suggested below. Makes for faster tests as well.

Copy link
Author

Choose a reason for hiding this comment

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

The failure was intentional, so to speak, but honestly I forgot about the pending change I intended to make. It is fixed now.

The problem is not in calling cli.main directly or not, but rather in that add_subparsers function only added required= kwarg in Python 3.7, so I wanted a test suite that would work with required=True in 3.7+ to then mimic the same behaviour (message and exit-code) for 3.6. See my most recent commit.

The reasons I went for subprocess approach in the first place are as follows:

  • it does not require hard-coding the prog= kwarg
  • it doubles as an integration test for the entire script (also testing that the entrypoint configuration is correct)
  • it does not require catching a SystemExit exception to ensure tests continue working

If it were up to me, I'd probably keep the existing tests as is, and use cli.main as soon as there are more unit tests to add. Although if you insist I can change go for cli.main in this PR as well.

Copy link
Owner

@jacobsvante jacobsvante Jan 5, 2022

Choose a reason for hiding this comment

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

@immerrr Gotcha. Let's drop support for 3.6, it's reached end-of-life. You can include that change in your PR. Or if you'd rather I remove it and you rebase on top of my commit?

Copy link
Author

Choose a reason for hiding this comment

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

Done (and reverted the change that enabled support for py3.6)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @immerrr. Tests are still failing on Windows. And they also fail locally on my Mac OS. Fails to find command fastapi-security. I would suggest that you go the cli.main route.

Copy link
Author

Choose a reason for hiding this comment

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

ok, will do


def test_usage_with_help_param():
result = subprocess.run(["fastapi-security", "-h"], capture_output=True)
assert result.returncode == 0
assert result.stdout.decode().splitlines() == [
"usage: fastapi-security [-h] {gendigest} ...",
"",
"fastapi_security command-line interface",
"",
"positional arguments:",
" {gendigest} Specify a sub-command",
"",
"optional arguments:",
" -h, --help show this help message and exit",
]
assert result.stderr.decode().splitlines() == []
Comment on lines +17 to +31
Copy link
Owner

Choose a reason for hiding this comment

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

Same with this one.



def test_gendigest_without_params():
result = subprocess.run(["fastapi-security", "gendigest"], capture_output=True)
assert result.returncode == 2
assert result.stdout.decode().splitlines() == []
assert result.stderr.decode().splitlines() == [
"usage: fastapi-security gendigest [-h] --salt SALT",
"fastapi-security gendigest: error: the following arguments are required: --salt",
]
Comment on lines +34 to +41
Copy link
Owner

Choose a reason for hiding this comment

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

fastapi-security command is not found when I run poetry run pytest. Perhaps call cli.main directly like you've done in test_gendigest_smoke_test? E.g:

Suggested change
def test_gendigest_without_params():
result = subprocess.run(["fastapi-security", "gendigest"], capture_output=True)
assert result.returncode == 2
assert result.stdout.decode().splitlines() == []
assert result.stderr.decode().splitlines() == [
"usage: fastapi-security gendigest [-h] --salt SALT",
"fastapi-security gendigest: error: the following arguments are required: --salt",
]
def test_gendigest_without_params(capsys):
with pytest.raises(SystemExit, match="2"):
cli.main(["gendigest"])
captured = capsys.readouterr()
assert captured.out == ""
assert captured.err.splitlines() == [
"usage: fastapi-security gendigest [-h] --salt SALT",
"fastapi-security gendigest: error: the following arguments are required: --salt",
]



def test_gendigest_smoke_test(capsys, monkeypatch):
# gendigest smoke test is performed not in a subprocess, because getpass
# uses /dev/tty instead of stdin/stdout for security reasons, and it is
# much more tricky to intercept it, so instead go for a simple monkeypatch.
monkeypatch.setattr(cli, "getpass", mock.Mock(return_value="hello"))
cli.main(["gendigest", "--salt=very-strong-salt"])
captured = capsys.readouterr()
assert (
captured.out
== "xRPfDaQHwpcXlzfWeR_uqOBTytcjEAUMv98SDnbHmpajmT_AxeJTHX6FyeM8H1T4otOe81PMWAOqAD5_tO4gYg==\n"
)
assert captured.err == "\nHere is your digest:\n"