Make mapping between (team) permissions and roles more lenient. #1711
Conversation
fisx
changed the title
fisx
force-pushed
the
fix-permission-role-mapping
branch
2 times, most recently
from
f8743c5
to
a912a44
Compare
fisx
force-pushed
the
fix-permission-role-mapping
branch
from
a912a44
to
4e9494f
Compare
|
now it does. i think. |
| -- we never did use @p /= p'@ for anything, fingers crossed that it doesn't occur anywhere | ||
| -- in the wild. but if it does, this implementation prevents privilege escalation. | ||
| let p'' = Set.intersection p p' | ||
| in permissionsRole (Permissions p'' p'') | ||
| permissionsRole (Permissions p _) = permsRole p |
There was a problem hiding this comment.
line 152 and line 158 have overlapping patterns. Will haskell actually fall back when the guard doesn't match? or would we have to move line 158 into an | otherwise = ?
There was a problem hiding this comment.
it does fall back, you've hit on something, this here is actually more clear and robust: f12e85c
| permissionsRole (Permissions p _) = permsRole p | ||
| where | ||
| permsRole :: Set Perm -> Maybe Role | ||
| permsRole perms = | ||
| Maybe.listToMaybe | ||
| [role | role <- [minBound ..], rolePerms role == perms] | ||
| [ role | ||
| | (perms', role) <- reverse . sortBy (compare `on` (length . fst)) $ (\r -> (rolePerms r, r)) <$> [minBound ..], |
There was a problem hiding this comment.
sortBy (compare `on` (length . fst)) -> sortOn (length . fst)
| permissionsRole (Permissions p _) = permsRole p | ||
| where | ||
| permsRole :: Set Perm -> Maybe Role | ||
| permsRole perms = | ||
| Maybe.listToMaybe | ||
| [role | role <- [minBound ..], rolePerms role == perms] | ||
| [ role | ||
| | (perms', role) <- reverse . sortBy (compare `on` (length . fst)) $ (\r -> (rolePerms r, r)) <$> [minBound ..], |
There was a problem hiding this comment.
I'm a bit confused why we're sorting on the length of the permission set? does that really do what we want? We can have two permission sets that are the same size; but we must consider one permission set strictly less permissive than another (Perhaps based on the ordering of Perm ?)
e.g. the rolePerms for RoleAdmin and RoleMember have the same length; they both contain 4 elements; but RoleMember <= RoleAdmin
There was a problem hiding this comment.
Shouldn't we sort by the natural ordering of Role ? e.g. just iterate over [minBound ..]
So:
[ role | role <- [minBound..], rolePerms role `Set.isSubsetOf` perms]
There was a problem hiding this comment.
good idea, in this particular case it's the same, and the tests will show if it changes: c67cd00
shouldn't change behavior for any existing users, but i found a user in our staging env that has an unknown set of permissions, and for that user is should fix https://wearezeta.atlassian.net/browse/SQSERVICES-720.
I've explained things in comments in the code, where we'll be able to find it in the future.
Checklist
make git-add-cassandra-schemato update the cassandra schema documentation.