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

[#340] Access key exclusion method #341

Merged
merged 1 commit into from
Aug 20, 2024
Merged

[#340] Access key exclusion method #341

merged 1 commit into from
Aug 20, 2024

Conversation

matthewhilton
Copy link
Contributor

@matthewhilton matthewhilton commented Aug 5, 2024

Closes #340

Changes

  • Adds access key based exclusion. Allowing optional setup of access key, which must be provided by testers via a url param to access.
  • This is added in addition to the current IP restriction, i.e. if both are enabled user needs to pass both.

Note: cookie must be used since at the point where outage checks the conditions, nothing has been setup yet i.e. no $USER, $SESSION, etc...

Test plan

Test Expected Result
No access key setup, no IP range setup Not blocked ever ✅ Not blocked
Access key setup, no IP range setup. Access key incorrect Blocked ✅ Blocked
Access key setup, no IP range setup. Access key correct Not blocked ✅ Not blocked
No Access key setup, IP range setup. Within ip range. Not blocked ✅ Not blocked
No Access key setup, IP range setup. Outside of ip range. Blocked ✅ Blocked
Access key setup and IP range setup. inside of ip range, access key incorrect Blocked ✅ Blocked
Access key setup and IP range setup. inside of ip range, access key correct Not blocked ✅ Not blocked

@matthewhilton matthewhilton force-pushed the access-key branch 2 times, most recently from 8697eb4 to cdba7a1 Compare August 5, 2024 23:18
@matthewhilton matthewhilton changed the base branch from MOODLE_39_STABLE to fixup-ci-and-codingstandards August 5, 2024 23:25
@matthewhilton matthewhilton marked this pull request as ready for review August 5, 2024 23:33
@matthewhilton matthewhilton force-pushed the access-key branch 3 times, most recently from 59bea1e to e75c046 Compare August 6, 2024 00:17
Base automatically changed from fixup-ci-and-codingstandards to MOODLE_39_STABLE August 9, 2024 00:59
@rhell4 rhell4 self-requested a review August 9, 2024 01:00
Copy link
Contributor

@rhell4 rhell4 left a comment

Choose a reason for hiding this comment

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

Hey @matthewhilton what are your thoughts on adding some extra unit tests to test how the climaintenance.php file resolves with the test plan?

We would probably have to add a few if (PHPUNIT_TEST) throughout the code here so that it doesn't exit or die, and maybe echo some more stuff for testing when $blocked is false. Capture the output with ob_get_contents and then compare it with what is expected (checking the comment tags, i.e. '<!-- Your access key is allowed: ...', etc.)?

@matthewhilton
Copy link
Contributor Author

Thanks @rhell4 good idea.

I did think of it briefly but got a bit put off of doing it because of needing to eval the php. But I ended up giving it a shot and it actually wasn't as bad as I thought. Although I did have to do some hacks to get the ip address and url params in there, will see if the CI likes it 😃

Copy link
Contributor

@rhell4 rhell4 left a comment

Choose a reason for hiding this comment

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

Looks good, all tested okay, new unit tests added to test our new scenario and old tests amended, CI is happy, I'm happy for this change to go in.

@rhell4 rhell4 merged commit cba55cd into MOODLE_39_STABLE Aug 20, 2024
30 checks passed
@rhell4 rhell4 deleted the access-key branch August 20, 2024 00:26
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.

Add option for key to let users in
2 participants