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

Limit Sysadmin Updating (no self modify, no system user modify) #8155

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

JVickery-TBS
Copy link
Contributor

feat(validator): sysadmin update;

  • Added extra validator to updating a user's sysadmin value.

Proposed fixes:

Adds an extra validator to the user schema which limits the capabilities of modifying a User's sysadmin value. Limits it so you cannot modify your own sysadmin value, or the system user's sysadmin value.

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply

- Added extra validator to updating a user's sysadmin value.
- Added change log file.
- Minor fix to new test.
- Flake8 syntax fixes.
- Pyrigth fixes.
- Fixed logic in new validator.
- Using ignore_auth with fake user should be allowed.
- Using ignore_auth with fake user should be allowed.
- Better logic for no value provided.
- Return values for new validator.
@JVickery-TBS
Copy link
Contributor Author

Wow okay, finally got all the tests working.

Copy link
Member

@amercader amercader left a comment

Choose a reason for hiding this comment

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

This looks great @JVickery-TBS , just some comments

ckan/logic/validators.py Outdated Show resolved Hide resolved
ckan/logic/validators.py Outdated Show resolved Hide resolved
ckan/tests/logic/action/test_patch.py Show resolved Hide resolved
JVickery-TBS and others added 3 commits April 10, 2024 10:49
Co-authored-by: Adrià Mercader <amercadero@gmail.com>
- More test coverage.
- Updated some logic and errors.
- flake8 fixes.
@JVickery-TBS
Copy link
Contributor Author

ooohh @amercader I am suddenly recalling another reason why I did the ignore_auth check thing. It is because the tests use a fake username 127.0.0.1 with ignore auth

So should I check the ignore_auth thing still if the provided user does not exist in the DB? Or is there a better way to do that?

- Use site id instead of loopback.
@JVickery-TBS
Copy link
Contributor Author

@amercader I tried changing the 127.0.0.1 user name in the test helper to config.get('ckan.site_id') but that has seemed to cause a lot more issues in the tests :/

@amercader
Copy link
Member

no worries @JVickery-TBS this is on my list to have a closer look. I'll figure out what's the best approach

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.

None yet

2 participants