Skip to content

Commit 2cc690c

Browse files
dannykoppingdannykopping
committed
Review feedback
Signed-off-by: Danny Kopping <danny@coder.com>
1 parent 7ee541b commit 2cc690c

File tree

7 files changed

+5
-42
lines changed

7 files changed

+5
-42
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1483,6 +1483,8 @@ func (q *querier) GetNotificationTemplateByID(ctx context.Context, id uuid.UUID)
14831483

14841484
func (q *querier) GetNotificationTemplatesByKind(ctx context.Context, kind database.NotificationTemplateKind) ([]database.NotificationTemplate, error) {
14851485
// TODO: restrict 'system' kind to admins only?
1486+
// All notification templates share the same rbac.Object, so there is no need
1487+
// to authorize them individually. If this passes, all notification templates can be read.
14861488
if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceNotificationTemplate); err != nil {
14871489
return nil, err
14881490
}

‎coderd/database/dump.sql

Lines changed: 0 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000237_notification_preferences.down.sql renamed to ‎coderd/database/migrations/000238_notification_preferences.down.sql

File renamed without changes.

‎coderd/database/migrations/000237_notification_preferences.up.sql renamed to ‎coderd/database/migrations/000238_notification_preferences.up.sql

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,6 @@ CREATE TABLE notification_preferences
88
PRIMARY KEY (user_id, notification_template_id)
99
);
1010

11-
-- Ensure we cannot insert multiple entries for the same user/template combination.
12-
ALTER TABLE notification_preferences
13-
ADD CONSTRAINT unique_user_notification_template UNIQUE (user_id, notification_template_id);
14-
1511
-- Add a new type (to be expanded upon later) which specifies the kind of notification template.
1612
CREATE TYPE notification_template_kind AS ENUM (
1713
'system'

‎coderd/database/migrations/testdata/fixtures/000237_notifications_preferences.up.sql renamed to ‎coderd/database/migrations/testdata/fixtures/000238_notifications_preferences.up.sql

File renamed without changes.

‎coderd/database/unique_constraint.go

Lines changed: 0 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/rbac/roles_test.go

Lines changed: 3 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -627,12 +627,12 @@ func TestRolePermissions(t *testing.T) {
627627
// Members may not access other members' preferences
628628
Name: "NotificationPreferencesOtherUser",
629629
Actions: []policy.Action{policy.ActionRead, policy.ActionUpdate},
630-
Resource: rbac.ResourceNotificationPreference.InOrg(orgID).WithOwner(uuid.NewString()), // some other user
630+
Resource: rbac.ResourceNotificationPreference.WithOwner(uuid.NewString()), // some other user
631631
AuthorizeMap: map[bool][]hasAuthSubjects{
632-
true: {orgAdmin, owner},
632+
true: {owner},
633633
false: {
634634
memberMe, templateAdmin, orgUserAdmin, userAdmin,
635-
orgAuditor, orgTemplateAdmin,
635+
orgAdmin, orgAuditor, orgTemplateAdmin,
636636
otherOrgMember, otherOrgAuditor, otherOrgUserAdmin, otherOrgTemplateAdmin,
637637
otherOrgAdmin, orgMemberMe,
638638
},
@@ -678,37 +678,6 @@ func TestRolePermissions(t *testing.T) {
678678
},
679679
},
680680
},
681-
{
682-
// Notification preferences are currently not organization-scoped
683-
// Any owner/admin across any organization may access any users' preferences
684-
// Members may access their own preferences
685-
Name: "NotificationPreferencesAnyOrg",
686-
Actions: []policy.Action{policy.ActionRead, policy.ActionUpdate},
687-
Resource: rbac.ResourceNotificationPreference.AnyOrganization().WithOwner(currentUser.String()),
688-
AuthorizeMap: map[bool][]hasAuthSubjects{
689-
true: {orgMemberMe, orgAdmin, otherOrgAdmin, owner},
690-
false: {
691-
memberMe, templateAdmin, otherOrgUserAdmin, userAdmin, orgUserAdmin,
692-
orgAuditor, orgTemplateAdmin,
693-
otherOrgMember, otherOrgAuditor, otherOrgTemplateAdmin,
694-
},
695-
},
696-
},
697-
{
698-
// Notification templates are currently not organization-scoped
699-
// Any owner/admin across any organization may access notification templates
700-
Name: "NotificationTemplateAnyOrg",
701-
Actions: []policy.Action{policy.ActionRead, policy.ActionUpdate},
702-
Resource: rbac.ResourceNotificationPreference.AnyOrganization(),
703-
AuthorizeMap: map[bool][]hasAuthSubjects{
704-
true: {orgAdmin, otherOrgAdmin, owner},
705-
false: {
706-
orgMemberMe, memberMe, templateAdmin, orgUserAdmin, userAdmin,
707-
orgAuditor, orgTemplateAdmin,
708-
otherOrgMember, otherOrgAuditor, otherOrgUserAdmin, otherOrgTemplateAdmin,
709-
},
710-
},
711-
},
712681
}
713682

714683
// We expect every permission to be tested above.

0 commit comments

Comments
 (0)