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: make access key and allowed ips OR conditional #345

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

rhell4
Copy link
Contributor

@rhell4 rhell4 commented Sep 29, 2024

Access keys and allowed IPs should be indpendant of each other and when at least one of them is satisfied the user should be allowed in.

The implementation of #340 made it if both access key and allowed IP was used if one of them failed the user was blocked. The correct use case was allowed IP was still to be used but an access key could be used so someone outside of that IP range could still get in with the provided key.

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. Inside of 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 Not blocked ✅ Not blocked
Access key setup and IP range setup. Outside of IP range, access key correct Not blocked ✅ Not blocked
Access key setup and IP range setup. Inside of IP range, access key correct Not blocked ✅ Not blocked
Access key setup and IP range setup. Outside of IP range, access key incorrect Blocked ✅ Blocked

Access keys and allowed IPs should be indpendant of each other and when
at least one of them is satisfied the user should be allowed in.
Copy link
Contributor

@matthewhilton matthewhilton 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 and tests are passing - happy to merge 👍

@matthewhilton matthewhilton merged commit a40e198 into MOODLE_39_STABLE Sep 30, 2024
52 checks passed
@matthewhilton matthewhilton deleted the access-key-fix branch September 30, 2024 00:44
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.

2 participants