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

FIX Allow logged in user with permissions to bypass basic auth #119

Merged

Conversation

emteknetnz
Copy link
Member

Issue #116

@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 15, 2025

This PR seems to be based on this assertion from the issue:

Expected behaviour: Logged in users with sufficient permissions should be able to see the /dev/check page without triggering Basic Auth regardless whether the Basic Auth option is turned on or not.

I'm not convinced that's necessarily the case. There's an explicit username and password. Surely if you don't know those credentials, it means you're not allowed to access the site (even though there is an account set up for you)?
Some projects may be relying on this current behaviour e.g. in UAT to ensure only specific testers can access UAT even if a prod snapshot (with prod users) is loaded in.

Perhaps there should be a configuration property to toggle between allowing any authenticated user vs only those with the basic auth creds?

@emteknetnz
Copy link
Member Author

emteknetnz commented Jan 15, 2025

The env variables only adds basic-auth for the dev/check endpoint, not the whole site. This PR is to get things into a state where the existing documentation for authentication is correct.

Perhaps there should be a configuration property to toggle between allowing any authenticated user vs only those with the basic auth creds?

According to the docs it's supposed to be admin only, and since it's on a dev/ endpoint this makes sense to me. Basic-auth can be used enabled to essentially bypass the admin requirement to allow machine-machine checks. There's a separate public facing health/check endpoint according to the docs that does a basic health check.

@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 15, 2025

This should target 3.0 so it can be released as a patch.
This isn't targeting 3.0 because it needs to bump the dependencies in order to be tested correctly.

@GuySartorelli
Copy link
Member

With this PR if I'm not logged in it automatically redirects me to the login screen when I'm not logged in - the basic auth challenge therefore isn't even made so I can't use it to bypass user authentication.

@sig-peggy
Copy link

This fix changes the behaviour described in the documentation:

Now if you access dev/check in a browser it will pop up a basic auth popup, and if the submitted username and password match the ones defined the username and password defined in the environment, access will be granted to the page.

The documentation should be updated if this is the preferred behaviour.

@emteknetnz emteknetnz force-pushed the pulls/3/admin-bypass branch from 5224d65 to 484e719 Compare January 15, 2025 22:47
@emteknetnz
Copy link
Member Author

emteknetnz commented Jan 15, 2025

With this PR if I'm not logged in it automatically redirects me to the login screen when I'm not logged in

Should be fixed now

Note you'll also need silverstripe/cwp-core#134 if you're using silverstripe/recipe-kitchen-sink and using a "test" environment type

Also you may need to test using a new private window as basic auth has a weird way to cache itself in a browser

@GuySartorelli
Copy link
Member

@sig-peggy

This fix changes the behaviour described in the documentation
The documentation should be updated if this is the preferred behaviour.

That was a bug - the intention of this PR is to make the behaviour follow what is documented.

@sig-peggy
Copy link

When I try this version of the fix I get the second problem noted in issue #73 again. You can see my approach to fixing that part of the problem here: 092d296

@GuySartorelli
Copy link
Member

If I an not logged in, and I click "cancel" when the basic auth prompt appears, I get this a blank error screen:
image
It should instead throw me to the login screen due to a failed auth check.

@emteknetnz emteknetnz force-pushed the pulls/3/admin-bypass branch from 484e719 to 3f81bb1 Compare January 16, 2025 00:15
@emteknetnz emteknetnz force-pushed the pulls/3/admin-bypass branch 2 times, most recently from d95cb3d to 532cdf4 Compare January 16, 2025 00:27
@emteknetnz
Copy link
Member Author

When I try this version of the fix I get the second problem noted

Have updated, now getting user/pw from $request rather than $_SERVER

It should instead throw me to the login screen due to a failed auth check.

Have updated to behave the same was as BasicAuth.php does i.e. show some text on failure instead of the dev template

@sig-peggy
Copy link

This one works for my needs, thanks 👍

@emteknetnz emteknetnz force-pushed the pulls/3/admin-bypass branch from 532cdf4 to b6aa2a2 Compare January 16, 2025 21:10
@GuySartorelli
Copy link
Member

GuySartorelli commented Jan 16, 2025

Now I'm getting a basic auth prompt when I haven't even set ENVCHECK_BASICAUTH_USERNAME and ENVCHECK_BASICAUTH_PASSWORD

This hints that something about the tests isn't quite right as well most likely. Maybe need to check the response from a FunctionalTest?

@emteknetnz emteknetnz force-pushed the pulls/3/admin-bypass branch from b6aa2a2 to fb905f2 Compare January 16, 2025 21:48
@emteknetnz
Copy link
Member Author

My bad, should be fixed now

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM, works as expected locally.

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.

3 participants