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

Add ADMIN roles through config file #3475

Merged
merged 2 commits into from Jul 5, 2023

Conversation

dbutenhof
Copy link
Member

PBENCH-1197

With the change to SSO, we've lost our own private realm and the ability to manage roles within it. We may be able to restore roles through Keycloak and LDAP groups, but this provides a temporary "quick and dirty" mechanism to define ADMIN roles for a server based on the username provided and cached from OIDC tokens.

We define a simple admin-role config variable which can be defined to a comma-separated list of usernames to be granted ADMIN role. This value is processed by the authorization code when it caches local User objects for validation.

I've added a functional test for the audit API, which requires ADMIN role, both to prove that it works and to provide a long-delayed minimal validation of auditing. (I decided not to merge this into the overloaded "datasets" test file, which means it can't easily be run last: this makes it a less rigorous "audit test", but that can be addressed later and it provides an "ADMIN role test" that's necessary now.)

@dbutenhof dbutenhof added Server API Of and relating to application programming interfaces to services and functions Users Of and relating to working with users. Audit Of and relating to server side changes to data labels Jun 30, 2023
@dbutenhof dbutenhof self-assigned this Jun 30, 2023
webbnh
webbnh previously approved these changes Jul 1, 2023
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks generally good (and nicely low-impact, more or less). I just have a bunch of nits.

jenkins/run-server-func-tests Outdated Show resolved Hide resolved
lib/pbench/server/auth/auth.py Outdated Show resolved Hide resolved
lib/pbench/test/functional/server/conftest.py Outdated Show resolved Hide resolved
lib/pbench/test/functional/server/conftest.py Show resolved Hide resolved
lib/pbench/test/unit/server/auth/test_auth.py Outdated Show resolved Hide resolved
lib/pbench/test/unit/server/auth/test_auth.py Outdated Show resolved Hide resolved
server/lib/config/pbench-server-default.cfg Outdated Show resolved Hide resolved
server/lib/config/pbench-server-default.cfg Show resolved Hide resolved
server/pbenchinacan/run-pbench-in-a-can Show resolved Hide resolved
@webbnh
Copy link
Member

webbnh commented Jul 1, 2023

We really need to do something about the legacy tests...it took seven tries to get this PR past Area-51. 🥴

PBENCH-1197

With the change to SSO, we've lost our own private realm and the ability to
manage roles within it. We may be able to restore roles through Keycloak and
LDAP groups, but this provides a temporary "quick and dirty" mechanism to
define ADMIN roles for a server based on the username provided and cached
from OIDC tokens.

We define a simple `admin-role` config variable which can be defined to a
comma-separated list of usernames to be granted ADMIN role. This value is
processed by the authorization code when it caches local `User` objects
for validation.

I've added a functional test for the `audit` API, which requires ADMIN role,
both to prove that it works and to provide a long-delayed minimal validation
of auditing. (I decided not to merge this into the overloaded "datasets" test
file, which means it can't easily be run last: this makes it a less rigorous
"audit test", but that can be addressed later and it provides an "ADMIN role
test" that's necessary now.)
Copy link
Member

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

👍

server/lib/config/pbench-server-default.cfg Show resolved Hide resolved
server/pbenchinacan/run-pbench-in-a-can Show resolved Hide resolved
lib/pbench/test/unit/server/auth/test_auth.py Outdated Show resolved Hide resolved
@dbutenhof dbutenhof merged commit 9008b1e into distributed-system-analysis:main Jul 5, 2023
4 checks passed
@dbutenhof dbutenhof deleted the admin branch July 5, 2023 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Of and relating to application programming interfaces to services and functions Audit Of and relating to server side changes to data Server Users Of and relating to working with users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants