Skip to content
Draft

Acl #1628

Show file tree
Hide file tree
Changes from 2 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
40 changes: 40 additions & 0 deletions ui/temboardui/acl.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
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?

119 changes: 119 additions & 0 deletions ui/temboardui/model/versions/014_acl.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
------------------------------------------------
-- ACL MANAGEMENT --
------------------------------------------------
CREATE TABLE application.acl (
"id" BIGSERIAL PRIMARY KEY,
"role" TEXT NOT NULL,
"action" TEXT NOT NULL,
"resource" TEXT NOT NULL,
"deny" BOOLEAN DEFAULT FALSE,
"cdate" TIMESTAMP WITH TIME ZONE DEFAULT NOW(),
"origin" TEXT,
UNIQUE("role", "action", "resource")
);

INSERT INTO
application.acl (role, action, resource)
VALUES
-- All users can access root
(
Comment thread
pirlgon marked this conversation as resolved.
'trn:temboard:*:*:*',
'GET:/',
'*'
Comment thread
pgiraud marked this conversation as resolved.
),
-- All users can access login page
(
'trn:temboard:*:*:*',
'*:/login',
'*'
),
-- All users can submit login form
(
'trn:temboard:*:*:*',
'*:/json/login',
'*'
),
-- All users can access reset password page
(
'trn:temboard:*:*:*',
'*:/reset-password',
'*'
),
-- All users can submit reset-password form
(
'trn:temboard:*:*:*',
'POST:/json/reset-password',
'*'
),
-- All identified users can access logout
(
'trn:temboard:core:user:*',
'*:/logout',
'*'
),
-- All identified users can access home
(
'trn:temboard:core:user:*',
'*:/home',
'*'
),
-- All identified users can retrieve instances list
(
'trn:temboard:core:user:*',
'GET:/json/instances/home',
'*'
),
-- All identified user can access about page
(
'trn:temboard:core:user:*',
'*:/about',
'*'
),
-- All users from admins group have access to ALL requests
(
'trn:temboard:core:group:admins',
'*',
'*'
),
-- ApiKey have access to open metrics
(
'trn:temboard:core:apikey:*',
'GET:/proxy/<address>/<port>/monitoring/metrics',
'*'
);

-- Insert ACL for all existing dba groups
-- e.g. : mass/dba => "trn:temboard:core:group:mass/dba" "*" "trn:temboard:core:instance:mass"
INSERT INTO
application.acl (role, action, resource)
SELECT
'trn:temboard:core:group:' || g.name AS group,
'*' AS action,
'trn:temboard:core:instance:' || e.name AS instance
FROM
application.groups g
JOIN application.environments e ON g.id = e.dba_group_id;

-- Create group admins
INSERT INTO
application.groups (name, description)
VALUES
('admins', 'Admin');

--Add every user having is_admin to true in admins group
INSERT INTO
application.memberships (role_name, group_id)
SELECT
r.role_name,
(
SELECT
id
FROM
application.groups
WHERE
name = 'admins'
)
FROM
application.roles r
WHERE
r.is_admin;
56 changes: 56 additions & 0 deletions ui/tests/unit/test_acl.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import pytest
from temboardui.acl import TRN, expand_actions

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.

expand_actions doesn't exist in this commit. It seems to be added in a future commit.



def test_trn_parse():
with pytest.raises(Exception) as e:
TRN.parse("malformed:trn")
assert str(e.value) == "Malformed TRN"

trn = TRN.parse("trn:temboard:core:user:alice")

assert trn.scope == "core"
assert trn.type == "user"
assert trn.name == "alice"

assert str(trn) == "trn:temboard:core:user:alice"

trn = TRN.parse("trn:temboard:core:instance:prod/pg001.bridoulou.fr")

assert trn.scope == "core"
assert trn.type == "instance"
assert trn.name == "prod/pg001.bridoulou.fr"


def test_trn_parent():
trn = TRN.parse("trn:temboard:core:user:alice")
assert str(trn.parent()) == "trn:temboard:core:user:*"

trn = TRN.parse("trn:temboard:core:group:prod/dba")
assert str(trn.parent()) == "trn:temboard:core:group:prod"

trn = TRN.parse("trn:temboard:core:group:prod/dba/indus")
assert str(trn.parent()) == "trn:temboard:core:group:prod/dba"

trn = TRN.parse("trn:temboard:*:*:*")
assert str(trn.parent()) == "trn:temboard:*:*:*"


def test_trn_expand():
trn = TRN.parse("trn:temboard:core:user:alice")
parents = TRN.expand(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.

Here you use expand just like if it was a classmethod. In the function self isn't really what it is supposed to be.

The following would be more straightforward:

parents = trn.expand()

assert len(parents) == 5
assert str(parents[0]) == "*"
assert str(parents[1]) == "trn:temboard:core:user:alice"
assert str(parents[2]) == "trn:temboard:core:user:*"
assert str(parents[3]) == "trn:temboard:core:*:*"
assert str(parents[4]) == "trn:temboard:*:*:*"
Comment on lines +43 to +47

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 feels like the order of parents is reversed, starting from index 1.

This would look more natural:

    assert str(parents[0]) == "*"
    assert str(parents[4]) == "trn:temboard:*:*:*"
    assert str(parents[3]) == "trn:temboard:core:*:*"
    assert str(parents[2]) == "trn:temboard:core:user:*"
    assert str(parents[1]) == "trn:temboard:core:user:alice"



def test_expand_action():
action = "POST:/login"
actions = expand_actions(action)
assert len(actions) == 3
assert actions[0] == "*"
assert actions[1] == "*:/login"
assert actions[2] == "POST:/login"