Skip to content

Commit

Permalink
chore: fix a missing check for global permissions in jq (determined-a…
Browse files Browse the repository at this point in the history
…i#874)

we had missed a check for global permissions when determining
if a user had access to a job.
  • Loading branch information
hamidzr authored and determined-ci committed Jun 21, 2023
1 parent 0572c44 commit 58308ca
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 7 deletions.
7 changes: 7 additions & 0 deletions e2e_tests/tests/cluster/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ def logged_in_user(credentials: authentication.Credentials) -> Generator:
log_out_user()


@pytest.mark.e2e_cpu
def test_logged_in_user() -> None:
with logged_in_user(ADMIN_CREDENTIALS):
output = det_run(["user", "whoami"])
assert f"You are logged in as user '{ADMIN_CREDENTIALS.username}'" in output


def get_random_string() -> str:
return str(uuid.uuid4())

Expand Down
15 changes: 14 additions & 1 deletion e2e_tests/tests/job/test_rbac.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from tests import api_utils
from tests import experiment as exp
from tests.cluster.test_rbac import create_workspaces_with_users, rbac_disabled
from tests.cluster.test_users import ADMIN_CREDENTIALS, logged_in_user
from tests.cluster.test_users import ADMIN_CREDENTIALS, det_run, logged_in_user


def seed_workspace(ws: bindings.v1Workspace) -> None:
Expand All @@ -33,6 +33,19 @@ def seed_workspace(ws: bindings.v1Workspace) -> None:
api_utils.launch_ntsc(admin_session, workspace_id=ws.id, typ=ntsc, exp_id=experiment_id)


@pytest.mark.e2e_cpu_rbac
@pytest.mark.skipif(rbac_disabled(), reason="ee rbac is required for this test")
def test_job_global_perm() -> None:
with logged_in_user(ADMIN_CREDENTIALS):
experiment_id = exp.create_experiment(
conf.fixtures_path("no_op/single.yaml"),
conf.fixtures_path("no_op"),
["--project_id", str(1)],
)
output = det_run(["job", "ls"])
assert str(experiment_id) in str(output)


@pytest.mark.e2e_cpu_rbac
@pytest.mark.skipif(rbac_disabled(), reason="ee rbac is required for this test")
def test_job_filtering() -> None:
Expand Down
1 change: 1 addition & 0 deletions master/internal/db/postgres_rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ func DoesPermissionMatchAll(ctx context.Context, curUserID model.UserID,
}

// GetNonGlobalWorkspacesWithPermission returns all workspaces the user has permissionID on.
// This does not check for permissions granted on scopes higher than workspace level eg cluster.
func GetNonGlobalWorkspacesWithPermission(ctx context.Context, curUserID model.UserID,
permissionID rbacv1.PermissionType,
) ([]int, error) {
Expand Down
37 changes: 34 additions & 3 deletions master/internal/job/authz_rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

log "github.com/sirupsen/logrus"

"github.com/determined-ai/determined/master/internal/authz"
"github.com/determined-ai/determined/master/internal/command"
"github.com/determined-ai/determined/master/internal/db"
"github.com/determined-ai/determined/master/pkg/model"
Expand All @@ -23,6 +24,8 @@ func (a *JobAuthZRBAC) FilterJobs(
var viewableNtscWorkspaces map[model.AccessScopeID]bool
hasNTSC := false
hasExperiment := false
userHasGlobalExpViewPerm := false
userHasGlobalNTSCViewPerm := false
for _, job := range jobs {
switch job.Type {
case jobv1.Type_TYPE_EXPERIMENT:
Expand All @@ -36,15 +39,41 @@ func (a *JobAuthZRBAC) FilterJobs(
}
}

if hasExperiment {
err = db.DoesPermissionMatch(ctx, curUser.ID, nil,
rbacv1.PermissionType_PERMISSION_TYPE_VIEW_EXPERIMENT_METADATA)
switch err.(type) {
case nil:
userHasGlobalExpViewPerm = true
case authz.PermissionDeniedError:
break
default:
return nil, err
}
}

if hasNTSC {
err = db.DoesPermissionMatch(ctx, curUser.ID, nil,
rbacv1.PermissionType_PERMISSION_TYPE_VIEW_NSC)
switch err.(type) {
case nil:
userHasGlobalNTSCViewPerm = true
case authz.PermissionDeniedError:
break
default:
return nil, err
}
}

if hasNTSC && !userHasGlobalNTSCViewPerm {
viewableNtscWorkspaces, err = command.AuthZProvider.Get().
AccessibleScopes(ctx, curUser, model.AccessScopeID(0))
if err != nil {
return nil, err
}
}

if hasExperiment {
if hasExperiment && !userHasGlobalExpViewPerm {
viewableExpWorkspacesList, err := db.GetNonGlobalWorkspacesWithPermission(
ctx, curUser.ID, rbacv1.PermissionType_PERMISSION_TYPE_VIEW_EXPERIMENT_METADATA,
)
Expand All @@ -60,12 +89,14 @@ func (a *JobAuthZRBAC) FilterJobs(
for _, job := range jobs {
switch job.Type {
case jobv1.Type_TYPE_EXPERIMENT:
if _, ok := viewableExpWorkspaces[int(job.WorkspaceId)]; ok {
viewable, _ := viewableExpWorkspaces[int(job.WorkspaceId)]
if userHasGlobalExpViewPerm || viewable {
viewableJobs = append(viewableJobs, job)
}
case jobv1.Type_TYPE_NOTEBOOK, jobv1.Type_TYPE_TENSORBOARD, jobv1.Type_TYPE_SHELL,
jobv1.Type_TYPE_COMMAND:
if _, ok := viewableNtscWorkspaces[model.AccessScopeID(job.WorkspaceId)]; ok {
viewable, _ := viewableNtscWorkspaces[model.AccessScopeID(job.WorkspaceId)]
if userHasGlobalNTSCViewPerm || viewable {
viewableJobs = append(viewableJobs, job)
}
// TODO: special case for tensorboard.
Expand Down
3 changes: 2 additions & 1 deletion master/internal/rm/dispatcherrm/dispatcher_monitor.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ func newDispatchWatcher(apiClient *launcherAPIClient) *launcherMonitor {
// When launchPending is true, a later call to notifyJobLaunched is required once the
// launcher has initiated the launch and the dispatchID will be valid for status checks.
func (m *launcherMonitor) monitorJob(
user string, dispatchID string, payloadName string, launchPending bool) {
user string, dispatchID string, payloadName string, launchPending bool,
) {
m.newLauncherJob <- &launcherJob{
user: user,
dispatcherID: dispatchID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1044,7 +1044,6 @@ func (m *dispatcherResourceManager) startLauncherJob(
m.rmConfig.LauncherContainerRunType, m.wlmType == pbsSchedulerType,
m.rmConfig.JobProjectSource, m.dbState.DisabledAgents,
)

if err != nil {
sendResourceStateChangedErrorResponse(ctx, err, msg,
"unable to launch job")
Expand Down
4 changes: 3 additions & 1 deletion master/internal/workspace/postgres_workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package workspace
import (
"context"

"github.com/pkg/errors"

"github.com/determined-ai/determined/master/internal/db"
"github.com/determined-ai/determined/master/pkg/model"
)
Expand Down Expand Up @@ -35,7 +37,7 @@ func WorkspaceByProjectID(ctx context.Context, projectID int) (*model.Workspace,
"id = (SELECT workspace_id FROM projects WHERE id = ?)",
projectID).Scan(ctx)
if err != nil {
return nil, err
return nil, errors.Wrapf(err, "failed to get workspace for project %d", projectID)
}
return &w, nil
}

0 comments on commit 58308ca

Please sign in to comment.