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

Web UI Modern Auth #2538

Merged
merged 6 commits into from
Jan 11, 2024
Merged

Conversation

andrewbaldwin44
Copy link
Collaborator

Fixes #2517

image

Proposal

  • Add a new page to the modern UI that allows for users to authenticate with username / password or with a provider
  • Remove flask_basicauth in favor of flask_login. The proposal is to move the responsibility of security and maintainability onto the user. Locust will simply specify which routes need to be protected, but the user will be the one responsible for implementing the protection. This has the added benefit of providing flexibility as to how users authenticate to the app
  • Add an example of how authentication can be implemented by the user
  • Add documentation on how to implement authentication
  • Add tests

examples/auth.py Outdated Show resolved Hide resolved
examples/auth.py Outdated Show resolved Hide resolved
@cyberw
Copy link
Collaborator

cyberw commented Jan 10, 2024

Cool stuff. See comments. Can you have an extra look and ensure there are no remnants (documentation etc) of the old stuff? Pretty sure we can nuke the AuthCredentialsError class for example.

@andrewbaldwin44 andrewbaldwin44 force-pushed the feature/webui-auth branch 4 times, most recently from 3c9e288 to 0520dbf Compare January 10, 2024 15:49
@cyberw
Copy link
Collaborator

cyberw commented Jan 10, 2024

Looking good! Only that one last comment about --web-auth :)

@andrewbaldwin44 andrewbaldwin44 force-pushed the feature/webui-auth branch 3 times, most recently from 47cb865 to 099ac03 Compare January 10, 2024 20:06
@cyberw
Copy link
Collaborator

cyberw commented Jan 11, 2024

Given that the package we use is called flask-login, perhaps it makes more sense to name the argument --login instead of --auth (and the function parameter name etc. sorry if i'm nit-picking but argument names are really hard to change after the fact. this is my last comment, I swear :)

@andrewbaldwin44
Copy link
Collaborator Author

andrewbaldwin44 commented Jan 11, 2024

Yup that makes sense! What do you think about --web-login, that way it is clear it is only for the web UI? (I pushed it so if you like it you can merge it, otherwise I can change it again :) )

locust/web.py Outdated
def __init__(
self,
environment: "Environment",
host: str,
port: int,
auth_credentials: Optional[str] = None,
use_auth: bool = False,
Copy link
Collaborator

@cyberw cyberw Jan 11, 2024

Choose a reason for hiding this comment

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

rename this to web_login? or maybe just login...

@@ -355,9 +355,16 @@ def setup_parser_arguments(parser):
dest="web_auth",
metavar="<username:password>",
default=None,
help="DEPRECATED. See https://github.com/locustio/locust/issues/2517 Turn on Basic Auth for the web interface. Should be supplied in the following format: username:password ",
help="DEPRECATED: use --web-login",
Copy link
Collaborator

@cyberw cyberw Jan 11, 2024

Choose a reason for hiding this comment

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

Change this to configargparse.SUPPRESS to hide it from the help text and let the error message speak for itself :)

@cyberw
Copy link
Collaborator

cyberw commented Jan 11, 2024

I made two more comments :)

And also... I have another thought. Maybe we dont even need a command line argument to enable it?

Perhaps we can just check whether someone has registered a user_loader callback (by calling environment.web_ui.login_manager.user_loader(load_user))? That should already be 1:1 with our users wanting to enable login, right?

@andrewbaldwin44
Copy link
Collaborator Author

Unfortunately I don't think there's a clean way to get around having an argument. The issue is that without the argument we need to initialize the LoginManager. And we can't initialize the LoginManager without a user loader without getting an error Missing user_loader or request_loader.... And we can't add a "placeholder" user_loader because then all users will be seeing the login page

@andrewbaldwin44
Copy link
Collaborator Author

I guess we could have the users set their user_loader on the web_ui (as in environment.web_ui.user_loader = my_user_loader). Then we would have to initialize the login_manager in the auth_required_if_enabled and enable auth if a user_loader is provided. My issue with doing it this way is we could be limiting a lot of flexibility by not exposing the login_manager to the user

@cyberw
Copy link
Collaborator

cyberw commented Jan 11, 2024

I see! Sounds messy. Lets keep the parameter then. Ok to merge?

@andrewbaldwin44
Copy link
Collaborator Author

Let's do it :)

@cyberw cyberw merged commit 43979d9 into locustio:master Jan 11, 2024
11 of 12 checks passed
@cyberw
Copy link
Collaborator

cyberw commented Jan 11, 2024

Agh. I think I made a mistake when merging (or something was wrong with the changes themselves)

Never mind, it is fine, I just made a mistake when testing it :)

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.

Update dependency or remove support for Basic Auth for the Web UI
2 participants