List the valid computer roles for devices.csv - #34
Open
TomlDev wants to merge 1 commit into
Open
Conversation
The roles allowed in the sophomorixRole column come from the
[computerrole.*] sections of sophomorix.ini, so they are configurable
per installation. Nothing exposed them: /v1/roles reports the user roles
present in LDAP, which is a different set for a different purpose. A
client that needs to offer the choice has had to hardcode a list, which
is wrong on any server that defines its own roles.
GET /v1/devices/roles returns them sorted, read per request so an edit
to sophomorix.ini takes effect without restarting the API.
Registered before /devices/{device}, which would otherwise answer for it
and return the details of a device named 'roles'.
ConfigParser.read() ignores a missing or unreadable file, and the
SophomorixIni constructor then reads a section this endpoint never asked
for, so an unreadable ini surfaces as KeyError('ROLE_USER') -- an error
about user roles from the computer roles endpoint. Both that and
configparser's own errors are reported as a 500 that names the file and
keeps the original error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The roles that are valid in the
sophomorixRolecolumn ofdevices.csvcome from the[computerrole.*]sections ofsophomorix.ini:They are therefore configurable per installation, and nothing exposes them.
/v1/rolesreports the user roles present in LDAP, which is a different set for a different purpose. A client that wants to offer the choice has to hardcode a list, and that list is wrong on any server that defines its own roles — ours currently carries seven, while this server reports fifteen.RoleChecker("GS"), matching the rest ofdevices.py, since a school-administrator maintainsdevices.csvtoo. No school parameter: the ini is server-wide.Details worth reviewing
Registered before
/devices/{device}, which would otherwise answer for it and return the details of a device namedroles.Read per request, so editing
sophomorix.initakes effect without restarting the API.Error handling.
ConfigParser.read()ignores a missing or unreadable file, and theSophomorixIniconstructor then reads a section this endpoint never asked for, so an unreadable ini surfaces asKeyError('ROLE_USER'). Without handling, an administrator asking for computer roles would get an error about user roles. Both that andconfigparser's own errors become a 500 that names the file and keeps the original error.A naming question
This lands as
/v1/devices/roles, one path segment away from the existing/v1/roles, which returns user roles. Two endpoints named "roles" meaning different things is not ideal even with the docstring spelling out the difference./v1/devices/computer-rolesor a/v1/sophomorix/...path would read better — happy to move it wherever you prefer, it is a one-line change.Testing
pytests/test_devices_roles.py, 14 tests, run on a 7.4.9 server againstlinuxmusterTools@lmn74. The full suite shows the same failures before and after this branch, compared as sortedFAILED/ERRORid lists rather than counts.The permission tests override only
check_authentication_header, soRoleCheckeritself still runs: both administrator roles get 200, and teacher, student, parent and staff get 401. The four equivalents added totest_devices.pyfollow that file's convention and go through the live server, so they need a provisioned machine and did not execute here — the ones above are what actually verifies the access rule.Seven mutations were each confirmed to fail at least one test: dropping
sorted(), hardcoding the role list instead of reading the ini, widening and narrowing theRoleChecker, dropping theKeyErrorbranch, dropping the original error from the message, and registering the route after/devices/{device}.Verified against the real
sophomorix.inion a 7.4 server, which returns the fifteen roles fromaddctowlan.SophomorixIniis present in the 7.4.5 tools package as well, so this does not depend on the newestlinuxmuster-tools.