Skip to content
Draft

Acl #1628

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion tests/test_25_environments.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import pytest
from fixtures.utils import retry_fast
from selenium.webdriver.common.by import By
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait


def test_create(prod, browser):
Expand Down Expand Up @@ -44,7 +46,11 @@ def test_add_member(alice_member, browse_prod_members):
def test_remove_member(alice_member, browse_prod_members):
browser = browse_prod_members
browser.select("tbody tr:nth-child(1) button").click()
username = browser.select(".modal-body strong").text
wait = WebDriverWait(browser, 10)
element = wait.until(
EC.visibility_of_element_located((By.CSS_SELECTOR, ".modal-body strong"))
)
username = element.text
assert username.startswith("a") # admin or alice
browser.select("#buttonDelete").click()
browser.absent("tbody tr:nth-child(2)")
Expand Down
2 changes: 0 additions & 2 deletions ui/.husky/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,3 @@

cd ui
npx lint-staged
ruff check
ruff format --check
6 changes: 5 additions & 1 deletion ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@
"vite": "^8.1.5"
},
"lint-staged": {
"*.{js,css,vue}": "prettier --write"
"*.{js,css,vue}": "prettier --write",
"*.py": [
"ruff check",
"ruff format --check"
]
},
"allowScripts": {
"vue-demi@0.14.8": true,
Expand Down
5 changes: 5 additions & 0 deletions ui/share/sql/dev-fixture.sql
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ VALUES
('admin', (SELECT id FROM application.groups WHERE name = 'stable/dba')),
('admin', (SELECT id FROM application.groups WHERE name = 'mass/dba'));

INSERT INTO application.acl (role, action, resource)
VALUES
('trn:temboard:core:group:mass/dba', '*', 'trn:temboard:core:instance:mass'),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
('trn:temboard:core:group:mass/dba', '*', 'trn:temboard:core:instance:mass'),
('trn:temboard:core:group:mass/dba', '*', 'trn:temboard:core:instance:mass/*'),

('trn:temboard:core:group:stable/dba', '*', 'trn:temboard:core:instance:stable');

-- Pre-register agents

INSERT INTO application.instances
Expand Down
91 changes: 91 additions & 0 deletions ui/temboardui/acl.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import logging

from flask import abort

logger = logging.getLogger(__name__)


class TRN:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to add a short docstring explain what TRN is, what it looks like.

def __init__(self, scope, type, name):
self.scope = scope
self.type = type
self.name = name

@staticmethod
def parse(trn):
elems = str.split(trn, ":")
if len(elems) < 5:
raise Exception("Malformed TRN")
return TRN(elems[2], elems[3], elems[4])
Comment on lines +7 to +12

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would use a class method here:

    @classmethod
    def parse(cls, trn):
        elems = str.split(trn, ":")
        if len(elems) < 5:
            raise Exception("Malformed TRN")
        return cls(elems[2], elems[3], elems[4])

You avoid repeating the name of the class. It prevents errors if the class is renamed. And it works better with inheritance.


def __str__(self):
return f"trn:temboard:{self.scope}:{self.type}:{self.name}"

def parent(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This would IMO be a good candidate for @property.

Then it would be called like this:
parent = TRN.parse("trn:temboard:core:user:alice").parent

parent = TRN(self.scope, self.type, self.name)

if self.name != "*":
parent.name = "*"
if "/" in self.name:
names = str.split(self.name, "/")
parent.name = "/".join(names[:-1])
return parent
if self.type != "*":
parent.type = "*"
return parent
parent.scope = "*"
return parent

def expand(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about having a property called parents instead?

trns = ["*"]
trn = TRN(self.scope, self.type, self.name)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

self is already a TRN. You shouldn't have to recreate one.

while str(trn) != "trn:temboard:*:*:*":
if str(trn) not in trns:
trns.append(str(trn))
trn = trn.parent()
trns.append(str(trn))
return trns

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of returning a list of strings, why not returning a list of TRN instances?



class ACLResult:
def __init__(self, role, action, resource, decision="allowed", statements=None):
self.role = role
self.action = action
self.resource = resource
self.decision = decision
self.statements = statements or []

def raise_for_decision(self):
log_prefix = "Access <%s %s on %s> "
log_args = (self.role, self.action, self.resource or "*")

if self.decision == "allowed":
logger.debug(
log_prefix + "allowed by %s",
*log_args,
", ".join(repr(s) for s in self.statements),
)
return True
else:
if self.decision == "implicitDeny":
logger.debug(log_prefix + "implicitly denied.", *log_args)
else:
logger.debug(
log_prefix + "denied by %s",
*log_args,
", ".join(repr(s) for s in self.statements if s.deny),
)
raise abort(403)


def expand_actions(action):
"""Returns the list of pattern relevant for this action."""
actions = ["*"]
if action != "*":
method, _, endpoint = action.partition(":")
if method != "*":
actions.append("*:" + endpoint)
elif endpoint != "*":
actions.append(method + ":*")
actions.append(action)
return actions
2 changes: 2 additions & 0 deletions ui/temboardui/application.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

from flask import current_app as app
from itsdangerous import URLSafeTimedSerializer
from sqlalchemy.orm import selectinload
from sqlalchemy.orm.exc import NoResultFound

from temboardui.errors import TemboardUIError
Expand Down Expand Up @@ -96,6 +97,7 @@ def get_role_by_cookie(session, content):
try:
role = (
session.query(Role)
.options(selectinload(Role.groups))
.filter(Role.role_name == str(c_role_name), Role.is_active.is_(True))
.one()
)
Expand Down
3 changes: 1 addition & 2 deletions ui/temboardui/handlers/settings/metadata.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import logging

from temboardui.web.tornado import admin_required, app, render_template
from temboardui.web.tornado import app, render_template

from ...version import inspect_versions

logger = logging.getLogger(__name__)


@app.route(r"/settings/metadata")
@admin_required
def metadata(request):
versions_info = inspect_versions()
infos = {
Expand Down
99 changes: 97 additions & 2 deletions ui/temboardui/model/orm.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from sqlalchemy.orm import Query, relationship
from temboardtoolkit.utils import utcnow

from ..acl import TRN
from . import QUERIES

Model = declarative_base()
Expand Down Expand Up @@ -62,6 +63,15 @@ def select_secret(cls, secret):
def expired(self):
return self.edate < utcnow()

def trn(self):
return TRN("core", "apikey", str(self.id))

def role_trns(self):
return self.trn().expand()

def resource_trns(self):
return self.trn().expand()
Comment on lines +66 to +73

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could possibly use @property.



class Plugin(Model):
__tablename__ = "plugins"
Expand Down Expand Up @@ -181,8 +191,8 @@ def asdict(self):
phone=self.role_phone,
active=self.is_active,
admin=self.is_admin,
groups=[g.name for g in self.groups],
environments=[g.environment.name for g in self.groups],
groups=[g.name for g in self.groups if g.environment],
environments=[g.environment.name for g in self.groups if g.environment],
Comment on lines +194 to +195

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it related to current commit?

)

def select_environments(self):
Expand All @@ -199,6 +209,19 @@ def select_instances(self):
.columns(Instance.__mapper__.c.values())
)

def trn(self):
return TRN("core", "user", self.role_name)

def role_trns(self):
trns = set()
trns.update(self.trn().expand())
for g in self.groups:
trns.update(g.trn().expand())
return trns

def resource_trns(self):
return self.trn().expand()


class StubRole:
# Fake object for roles not in database.
Expand Down Expand Up @@ -256,6 +279,12 @@ def delete_member(self, name, username):
group=name, role=username
)

def trn(self):
return TRN("core", "group", self.name)

def resource_trns(self):
return self.trn().expand()


class Environment(Model):
__tablename__ = "environments"
Expand Down Expand Up @@ -327,6 +356,12 @@ def asdict(self):
dba_group=self.dba_group.name,
)

def trn(self):
return TRN("core", "environment", self.name)

def resource_trn(self):
return self.trn().expand()


class Instance(Model):
__tablename__ = "instances"
Expand Down Expand Up @@ -522,3 +557,63 @@ def enable_plugin(self, plugin):

def disable_plugin(self, plugin):
return Plugin.delete(self, plugin)

def trn(self):
return TRN(
"core",
"instance",
f"{self.environment.name}/{self.agent_address}:{self.agent_port}",
)

def resource_trns(self):
return self.trn().expand()


class ACLRule(Model):
__tablename__ = "acl"
__table_args__ = {"schema": "application"}

id = Column(types.BigInteger, primary_key=True)
role = Column(types.UnicodeText)
action = Column(types.UnicodeText)
resource = Column(types.UnicodeText)
deny = Column(types.Boolean)
cdate = Column(types.TIMESTAMP(timezone=True))
origin = Column(types.UnicodeText)

@classmethod
def insert(cls, role, action, resource, deny=False):
return Query(cls).from_statement(
text(QUERIES["acl-insert"]).bindparams(
role=role, action=action, resource=resource, deny=deny
)
)

@classmethod
def delete(cls, role, action, resource):
return Query(cls).from_statement(
text(QUERIES["acl-delete"]).bindparams(
role=role, action=action, resource=resource
)
)

@classmethod
def match(cls, roles, actions, resources):
return Query(cls).from_statement(
text(QUERIES["acl-get"]).bindparams(
roles=roles, actions=actions, resources=resources
)
)

def __repr__(self):
return f"<ACL stmt deny={self.deny} {self.role} for {self.action} on {self.resource}>"


class Anonymous:
@staticmethod
def trn():
return TRN("*", "*", "*")

@staticmethod
def role_trns():
return [Anonymous.trn()]
6 changes: 6 additions & 0 deletions ui/temboardui/model/queries/acl-delete.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
DELETE FROM
application.acl
WHERE
role = :role
AND action = :action
AND resource = :resource RETURNING *;
8 changes: 8 additions & 0 deletions ui/temboardui/model/queries/acl-get.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
SELECT
*
FROM
application.acl
WHERE
role = ANY(:roles)
AND ACTION = ANY(:actions)
Comment thread
pirlgon marked this conversation as resolved.
AND resource = ANY(:resources);
5 changes: 5 additions & 0 deletions ui/temboardui/model/queries/acl-insert.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
INSERT INTO
application.acl(role, action, resource, deny)
VALUES
(:role, :action, :resource, :deny)
RETURNING *;
Loading