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

Permissions and restrictions #93

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

jhoxhaa
Copy link
Collaborator

@jhoxhaa jhoxhaa commented Oct 30, 2024

No description provided.

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Oct 30, 2024
@jhoxhaa jhoxhaa force-pushed the permissions-and-restrictions branch 6 times, most recently from b8fc41c to 8003fbd Compare October 31, 2024 07:06
@jhoxhaa jhoxhaa requested a review from lippserd October 31, 2024 09:02
@jhoxhaa jhoxhaa self-assigned this Oct 31, 2024
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

Haven't looked at Auth yet.

application/controllers/ConfigmapsController.php Outdated Show resolved Hide resolved
return Event::on(Database::connection());
$events = Event::on(Database::connection());

foreach ($events as $event) {
Copy link
Member

Choose a reason for hiding this comment

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

Overriding getQuery() as opposed to the other overrides makes sense here, but not the way it is implemented. Since you want to ensure that only events that match the permissions are listed, I would implement something that goes through the show permissions and translates them into an OR filter that is applied to the query, e.g. WHERE reference_kind IN ('Pod', 'CronJob', ...).

Copy link
Member

Choose a reason for hiding this comment

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

This seems unmodified. Create an array of valid reference kinds from the given permissions and then use Filter::equal() as it supports arrays, i.e.

$allowedKinds = [...];

$events->filter(Filter::equal('reference_kind', $allowedKinds));

Copy link
Member

Choose a reason for hiding this comment

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

Why are you still iterating over the events?

library/Kubernetes/Web/CronJobDetail.php Outdated Show resolved Hide resolved
library/Kubernetes/Web/JobDetail.php Outdated Show resolved Hide resolved
library/Kubernetes/Web/NodeDetail.php Outdated Show resolved Hide resolved
library/Kubernetes/Web/PodDetail.php Outdated Show resolved Hide resolved
@Icinga Icinga deleted a comment from lippserd Oct 31, 2024
@Icinga Icinga deleted a comment from lippserd Oct 31, 2024
@jhoxhaa jhoxhaa force-pushed the permissions-and-restrictions branch 3 times, most recently from b20a37c to 90f3815 Compare November 4, 2024 08:39
@jhoxhaa jhoxhaa requested a review from lippserd November 4, 2024 08:40
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

Please double check your changes before requesting a review. There are some minor issues that could have been easily spotted by looking through the code again.

application/controllers/ConfigmapController.php Outdated Show resolved Hide resolved
return Event::on(Database::connection());
$events = Event::on(Database::connection());

foreach ($events as $event) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems unmodified. Create an array of valid reference kinds from the given permissions and then use Filter::equal() as it supports arrays, i.e.

$allowedKinds = [...];

$events->filter(Filter::equal('reference_kind', $allowedKinds));

application/controllers/ReplicasetsController.php Outdated Show resolved Hide resolved
application/controllers/NamespaceController.php Outdated Show resolved Hide resolved
library/Kubernetes/Web/CronJobDetail.php Outdated Show resolved Hide resolved
library/Kubernetes/Web/StatefulSetDetail.php Outdated Show resolved Hide resolved
doc/04-Security.md Show resolved Hide resolved
application/controllers/EventController.php Show resolved Hide resolved
doc/04-Security.md Outdated Show resolved Hide resolved
doc/04-Security.md Outdated Show resolved Hide resolved
@jhoxhaa jhoxhaa force-pushed the permissions-and-restrictions branch 2 times, most recently from c3f7af2 to d98589f Compare November 12, 2024 08:25
$configMap = ConfigMap::on(Database::connection())
->filter(Filter::equal('uuid', $uuidBytes))
->first();
$q = ConfigMap::on(Database::connection())
Copy link
Member

Choose a reason for hiding this comment

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

Since the last review, withRestrictions() was introduced, which can be used fluently. Why is it not used in the same way here? Is there a reason for so many code blocks? If not, please adjust all controllers accordingly:

$q = Auth::getInstance()->withRestrictions(Auth::SHOW_CONFIG_MAPS, ConfigMap::on(Database::connection()))
    ->filter(Filter::equal('uuid', $uuidBytes))
    ->first();

@@ -19,7 +21,21 @@ protected function getContentClass(): string

protected function getQuery(): Query
{
return Event::on(Database::connection());
$events = Event::on(Database::connection());
$events = Auth::getInstance()->withRestrictions(Auth::SHOW_EVENTS, $events);
Copy link
Member

Choose a reason for hiding this comment

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

No need to have $events twice.

return Event::on(Database::connection());
$events = Event::on(Database::connection());

foreach ($events as $event) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you still iterating over the events?


## Restricted Permissions:

If a user has permission to show one resource but lacks permissions for another resource that is dependent on or related
Copy link
Member

Choose a reason for hiding this comment

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

Restricted permissions is the combination of permission(s) and restriction(s), i.e. if I have two roles where role a allows to list pods and has defined a certain restriction and role b allows to list deployments and has specified a certain restriction, I see pods that are restricted by role a and I see deployments that are restricted by role b. We have introduced something similar in Icinga DB. Previously, with the functionality as it exists in the monitoring module for example, I would see pods and deployments that match both role a and role b restrictions.

Though, I think the explanation of the permissions is good and would keep it, but move it to the section on permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants