Proposal: Flatten groups

[nevivurn]

현제 그룹-permission 구조가 너무 복잡하다고 생각합니다. 조금 더 쉽게 관리를 하기 위해 다음과 같이 그룹 구조를 바꾸는 것을 제안합니다.

Proposal

1. group-group 관계 삭제. (DROP TABLE group_relations)
2. permission 관계 inversion
   • 그룹은 여러 permission을 가지고
   • 사용자는 속한 모든 그룹의 permission의 union을 가진다

좋은 점

• group_reachable_cache를 없애도 된다
  • 그룹 relation을 변경한 후 재시작할 필요가 없어진다
• 그룹 관리 페이지 등에서 사용자가 어느 그룹에 직접 들어가 있는지 혹은 상위 그룹에 들어가 있는지 확인하지 않아도 된다
• 그룹 간 관계가 없어짐으로써 관리하고 이해하기 편해진다
• 앞으로 만들 관리자 페이지에 그룹 간 관계 설정을 추가하지 않아도 된다
  • 지금 상태로 이걸 추가한다면 그룹을 변경할 때마다 group_reachable_cache를 다시 생성해야 한다
• 몇 그룹들은 이미 이 제안의 permission 역할을 하고 있다
  • 현제 모든 permission들은 하나의 permission requirement을 가진다 ("permission groups")
    • 예: "000 실습실 사용자"
  • 이 "permission group" 외에 다른 그룹을 subgroup으로 가지는 그룹은 없다
  • 이 제안은 "permission group"들을 permission으로 바꾸는 것으로 생각할 수 있다

좋지 않은 점

• 개발을 해야 한다
  • 대부분 core, 프론트는 그룹 관리 페이지에 있는 is_direct_member 관련 코드를 없애면 된다
  • 그룹 목록 API 외에는 외부적으로 나타나는 차이는 없다
• 비슷한 그룹이 많을 경우 모든 그룹에 똑같은 permission을 주어야 한다
• contributions welcome

Changes

user permission check:

before:

• build user-reachable groups table in runtime (built and cached on startup)
• list all permission requirements
• list all groups reachable by user
• check if all permission requirements are satisfied
• currently requires two queries and a loop, doable in a single query with a CTE.

id/core/src/model/permissions.ts

Lines 49 to 62 in 437ff3a

	public async checkUserHavePermission(tr: Transaction, userIdx: number, permissionIdx: number): Promise<boolean> {
	const permissionRequirements = await this.getAllPermissionRequirements(tr, permissionIdx)
	if (permissionRequirements.length === 0) {
	return true
	}
	const userReachableGroups = Array.from(await this.model.users.getUserReachableGroups(tr, userIdx))
	for (const pr of permissionRequirements) {
	if (!userReachableGroups.includes(pr)) {
	return false
	}
	}
	return true
	}

after:

• list all groups the user is in
• list all permissions each group has
• check if permission is in said list
• efficient, doable in a single query with 3 joins

SELECT EXISTS (
  SELECT 1 FROM users u 
    JOIN user_memberships um ON um.user_idx = u.idx 
    JOIN group_permissions gp ON gp.group_idx = um.group_idx
    JOIN permissions p ON p.idx = gp.permission_idx
  WHERE u.name = $1 AND p.name = $2
)

user membership list

before:

• if checking groups the user is directly in, query user_memberships
• if checking all reachable groups, query cache
• if both, see below

id/core/src/model/groups.ts

Lines 80 to 99 in 437ff3a

	WITH
	umem AS (SELECT user_idx, group_idx FROM user_memberships WHERE user_idx = $1),
	pend_umem AS (SELECT user_idx, group_idx FROM pending_user_memberships WHERE user_idx = $1)
	SELECT DISTINCT ON (g.idx)
	g.idx,
	g.name_ko,
	g.name_en,
	g.description_ko,
	g.description_en,
	(umem.user_idx IS NOT NULL) AS is_member,
	(dir.user_idx IS NOT NULL) AS is_direct_member,
	(pend_umem.user_idx IS NOT NULL) AS is_pending,
	(EXISTS (SELECT 1 FROM umem WHERE umem.group_idx = g.owner_group_idx)) AS is_owner
	FROM umem
	RIGHT JOIN group_reachable_cache gr ON umem.group_idx = gr.supergroup_idx
	RIGHT JOIN groups g ON g.idx = gr.subgroup_idx
	LEFT JOIN umem dir ON dir.group_idx = g.idx
	LEFT JOIN pend_umem ON pend_umem.group_idx = g.idx
	WHERE g.owner_group_idx IS NOT NULL
	ORDER BY g.idx, umem.user_idx

after

• similar to above, but we don't need joins anymore, no more indirect groups
• owner checking is no longer confusing

WITH
  umem AS (SELECT group_idx FROM user_memberships WHERE user_idx = $1),
  pend_umem AS (SELECT group_idx FROM pending_user_memberships WHERE user_idx = $1)
SELECT 
  g.idx,
  g.name_ko,
  g.name_en,
  g.description_ko,
  g.description_en,
  (EXISTS (SELECT 1 FROM umem WHERE umem.group_idx = g.idx)) AS is_member,
  (EXISTS (SELECT 1 FROM pend_umem WHERE pend_umem.group_idx = g.idx)) AS is_pending,
  (EXISTS (SELECT 1 FROM umem WHERE umem.group_idx = g.owner_group_idx)) AS is_owner
FROM groups g ORDER BY g.idx;

restrict permission to a single group

before:

• set the new group to be a supergroup of all other permission requirements
• add the new group as a permission requirement
• to restore:
  • remove the new group, relations are removed automatically

after:

• set the new group to have the permission
• remove the permission from all other groups
• to restore:
  • remove the new group
  • restore permission to other groups

adding a temp group

before:

• create a temp group
• make the temp group a supergroup of appropriate groups

after:

• create a temp group
• add permissions to temp group

organization

before:

• we have groups like "server users" and "computer users" with no semantic meaning ("permission groups")

after:

• remove all "permission groups"
• remaining groups will have meaningful names, like "staff", "majors"

[seojangho]

Indirect membership을 없애는 제안으로 이해했습니다. 찬성합니다.

@skystar-p @moduhary @81887821 어떠세유?

[81887821]

해당 제안은 잘 읽어봤습니다. ID 프로젝트의 첫 개발이 끝난지 약 1년이 지났고, 우리 코드를 재검토해보기에 적절한 시기인 것 같아 반가운 제안입니다.

해당 구조를 설계할 때 많은 논의가 오갔던 것으로 기억하는데 생각보다 현재 남아있는 문서가 적어 아쉽네요. 우선 group간의 포함관계와 permission 구조는 1. 과거 Active directory 시절의 그룹 구조를 그대로 가져올 수 있으며, 2. 미래에 개발할 수도 있을 여러 기능에 사용 가능하도록, 모든 경우에 대응할 수 있게끔 설계되었던 것으로 기억합니다.

1번은 현재 필요하지 않게 되었습니다. Active directory & SNUCSE 2.0 시절에는 학사, 석사, 박사 그룹을 따로 관리했었고, 학번별 그룹도 만들어 사용했으나 현재는 컴퓨터공학부 구성원 그룹으로 통합되었으니까요.

2번의 경우 개인정보 처리방침에 동의한 사용자들의 그룹이나 이용 약관에 동의한 사람들의 그룹을 만들어 실습서버나 실습실 이용 권한과 and 관계를 만들자는 제안이 있었던 것으로 기억합니다. 이 외의 group 활용 방안들(sudoer group을 만드는 등)은 제안하신 구조로도 구현 가능해보입니다.

적어주신 장단점에 대해 간단히 리뷰를 하자면,

• group_reachable_cache를 없애도 된다
  • 그룹 relation을 변경한 후 재시작할 필요가 없어진다

이 부분은 원래 ID Server에서 그룹에 대한 변경이 이루어져야 하는데 해당 부분이 개발되지 않아 DB에 직접 쿼리를 날리고 있어서 발생하는 문제입니다. 현재 구조 자체의 문제라기 보다는 개발이 덜 된 것이 문제로 보입니다.

• 그룹 관리 페이지 등에서 사용자가 어느 그룹에 직접 들어가 있는지 혹은 상위 그룹에 들어가 있는지 확인하지 않아도 된다
• 앞으로 만들 관리자 페이지에 그룹 간 관계 설정을 추가하지 않아도 된다
  • 지금 상태로 이걸 추가한다면 그룹을 변경할 때마다 group_reachable_cache를 다시 생성해야 한다

개발 인력이 부족한 현 바쿠스 상황에서 이 부분은 큰 이점으로 작용할 것 같습니다.

• 그룹 간 관계가 없어짐으로써 관리하고 이해하기 편해진다

지극히 개인적인 생각이지만, 현 구조도 지금 DB 쿼리를 날리고 있어서 보기 힘들 뿐이지, 적절한 인터페이스가 갖추어진다면 이해하기 불편하진 않을 것이라 생각합니다.

• 몇 그룹들은 이미 이 제안의 permission 역할을 하고 있다
  • 현제 모든 permission들은 하나의 permission requirement을 가진다 ("permission groups")
    • 예: "000 실습실 사용자"
  • 이 "permission group" 외에 다른 그룹을 subgroup으로 가지는 그룹은 없다
  • 이 제안은 "permission group"들을 permission으로 바꾸는 것으로 생각할 수 있다

모든 permission group을 permission으로 대체하는 것은 불가능합니다. 실습실 사용자 그룹, 실습 서버 사용자 그룹, SNUCSE 사용자 그룹 등은 새 구조로 바꿀 시 없앨 수 있지만 GPU 사용자 그룹 등은 여전히 필요합니다. 나중에 the lounge 사용자 그룹을 id에서 관리하게 된다거나, 다른 서비스를 개시하여 해당 사용자들을 id로 관리하게 될 경우 그 사용자들을 위한 그룹은 여전히 필요해 보입니다.

• 비슷한 그룹이 많을 경우 모든 그룹에 똑같은 permission을 주어야 한다

비슷한 하위 그룹들을 더 큰 그룹으로 묶어주는 과정은 여전히 필요하므로 큰 단점은 아닐 것으로 생각됩니다.

새 구조로 바꾸는 데에 들어가는 시간도 많이 들고, 한번 바꾸면 되돌리기도 어려우니 다른 사람들의 의견도 충분히 듣고 결정하면 좋겠습니다.