test: add tests for unprotected behavior#2203
Conversation
Added tests targeting high-risk unprotected logic around application permissions: - Tested group permissions `hasPermission` and `isUserAdminOrOwnerOfGroup` in `group_permission.dart`. - Tested timetable schedule permissions `hasPermissionToManageLessons` and `hasPermissionToManageEvents` in `timetable_permissions.dart`. - Tested submission visibility permissions `isAllowedToViewSubmittedPermissions` in `submission_permissions.dart` to prevent students from accessing other users' homework data. Co-authored-by: nilsreichardt <24459435+nilsreichardt@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request adds a valuable set of tests for permission-related logic, improving the reliability of the application. The tests are well-structured and cover the intended scenarios correctly. I've suggested some refactorings to reduce boilerplate code in the test files, which will make them more concise and easier to maintain in the future.
| group('hasPermission for administration', () { | ||
| test('should be true for owner', () { | ||
| expect(MemberRole.owner.hasPermission(GroupPermission.administration), | ||
| isTrue); | ||
| }); | ||
|
|
||
| test('should be true for admin', () { | ||
| expect(MemberRole.admin.hasPermission(GroupPermission.administration), | ||
| isTrue); | ||
| }); | ||
|
|
||
| test('should be false for creator', () { | ||
| expect(MemberRole.creator.hasPermission(GroupPermission.administration), | ||
| isFalse); | ||
| }); | ||
|
|
||
| test('should be false for standard', () { | ||
| expect( | ||
| MemberRole.standard.hasPermission(GroupPermission.administration), | ||
| isFalse); | ||
| }); | ||
| }); | ||
|
|
||
| group('hasPermission for contentCreation', () { | ||
| test('should be true for owner', () { | ||
| expect(MemberRole.owner.hasPermission(GroupPermission.contentCreation), | ||
| isTrue); | ||
| }); | ||
|
|
||
| test('should be true for admin', () { | ||
| expect(MemberRole.admin.hasPermission(GroupPermission.contentCreation), | ||
| isTrue); | ||
| }); | ||
|
|
||
| test('should be true for creator', () { | ||
| expect( | ||
| MemberRole.creator.hasPermission(GroupPermission.contentCreation), | ||
| isTrue); | ||
| }); | ||
|
|
||
| test('should be false for standard', () { | ||
| expect( | ||
| MemberRole.standard.hasPermission(GroupPermission.contentCreation), | ||
| isFalse); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests in these groups are repetitive. You can use a data-driven approach with a map of test cases to make the code more concise and maintainable. This pattern can also be applied to the isUserAdminOrOwnerOfGroup test group.
group('hasPermission for administration', () {
final testCases = <MemberRole, bool>{
MemberRole.owner: true,
MemberRole.admin: true,
MemberRole.creator: false,
MemberRole.standard: false,
};
for (final entry in testCases.entries) {
test('should be ${entry.value} for ${entry.key.name}', () {
expect(
entry.key.hasPermission(GroupPermission.administration),
entry.value,
);
});
}
});
group('hasPermission for contentCreation', () {
final testCases = <MemberRole, bool>{
MemberRole.owner: true,
MemberRole.admin: true,
MemberRole.creator: true,
MemberRole.standard: false,
};
for (final entry in testCases.entries) {
test('should be ${entry.value} for ${entry.key.name}', () {
expect(
entry.key.hasPermission(GroupPermission.contentCreation),
entry.value,
);
});
}
});| group('SubmissionPermissions.isAllowedToViewSubmittedPermissions', () { | ||
| test('should allow when user is teacher and is admin in course', () async { | ||
| when(mockCourseGateway.getRoleFromCourseNoSync(any)) | ||
| .thenReturn(MemberRole.admin); | ||
| final stream = Stream.value(TypeOfUser.teacher); | ||
| final permissions = SubmissionPermissions(stream, mockCourseGateway); | ||
| final homework = HomeworkDto.create(courseID: 'course1'); | ||
|
|
||
| final result = | ||
| await permissions.isAllowedToViewSubmittedPermissions(homework); | ||
|
|
||
| expect(result, isTrue); | ||
| }); | ||
|
|
||
| test('should allow when user is teacher and is owner in course', () async { | ||
| when(mockCourseGateway.getRoleFromCourseNoSync(any)) | ||
| .thenReturn(MemberRole.owner); | ||
| final stream = Stream.value(TypeOfUser.teacher); | ||
| final permissions = SubmissionPermissions(stream, mockCourseGateway); | ||
| final homework = HomeworkDto.create(courseID: 'course1'); | ||
|
|
||
| final result = | ||
| await permissions.isAllowedToViewSubmittedPermissions(homework); | ||
|
|
||
| expect(result, isTrue); | ||
| }); | ||
|
|
||
| test('should deny when user is teacher but only standard in course', () async { | ||
| when(mockCourseGateway.getRoleFromCourseNoSync(any)) | ||
| .thenReturn(MemberRole.standard); | ||
| final stream = Stream.value(TypeOfUser.teacher); | ||
| final permissions = SubmissionPermissions(stream, mockCourseGateway); | ||
| final homework = HomeworkDto.create(courseID: 'course1'); | ||
|
|
||
| final result = | ||
| await permissions.isAllowedToViewSubmittedPermissions(homework); | ||
|
|
||
| expect(result, isFalse); | ||
| }); | ||
|
|
||
| test('should deny when user is student even if admin in course', () async { | ||
| when(mockCourseGateway.getRoleFromCourseNoSync(any)) | ||
| .thenReturn(MemberRole.admin); | ||
| final stream = Stream.value(TypeOfUser.student); | ||
| final permissions = SubmissionPermissions(stream, mockCourseGateway); | ||
| final homework = HomeworkDto.create(courseID: 'course1'); | ||
|
|
||
| final result = | ||
| await permissions.isAllowedToViewSubmittedPermissions(homework); | ||
|
|
||
| expect(result, isFalse); | ||
| }); | ||
|
|
||
| test('should deny when course role is null (defaults to standard)', () async { | ||
| when(mockCourseGateway.getRoleFromCourseNoSync(any)).thenReturn(null); | ||
| final stream = Stream.value(TypeOfUser.teacher); | ||
| final permissions = SubmissionPermissions(stream, mockCourseGateway); | ||
| final homework = HomeworkDto.create(courseID: 'course1'); | ||
|
|
||
| final result = | ||
| await permissions.isAllowedToViewSubmittedPermissions(homework); | ||
|
|
||
| expect(result, isFalse); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests in this group are very similar and contain duplicated code. You can refactor this using a data-driven approach to make the tests more concise and easier to maintain and extend.
group('SubmissionPermissions.isAllowedToViewSubmittedPermissions', () {
final homework = HomeworkDto.create(courseID: 'course1');
final testCases = <String, ({TypeOfUser userType, MemberRole? role, bool expected})>{
'allow when user is teacher and is admin in course': (
userType: TypeOfUser.teacher,
role: MemberRole.admin,
expected: true,
),
'allow when user is teacher and is owner in course': (
userType: TypeOfUser.teacher,
role: MemberRole.owner,
expected: true,
),
'deny when user is teacher but only standard in course': (
userType: TypeOfUser.teacher,
role: MemberRole.standard,
expected: false,
),
'deny when user is student even if admin in course': (
userType: TypeOfUser.student,
role: MemberRole.admin,
expected: false,
),
'deny when course role is null (defaults to standard)': (
userType: TypeOfUser.teacher,
role: null,
expected: false,
),
};
for (final entry in testCases.entries) {
test('should ${entry.key}', () async {
when(mockCourseGateway.getRoleFromCourseNoSync(any))
.thenReturn(entry.value.role);
final stream = Stream.value(entry.value.userType);
final permissions = SubmissionPermissions(stream, mockCourseGateway);
final result =
await permissions.isAllowedToViewSubmittedPermissions(homework);
expect(result, entry.value.expected);
});
}
});| group('hasPermissionToManageLessons', () { | ||
| test('should be true for owner', () { | ||
| expect(hasPermissionToManageLessons(MemberRole.owner), isTrue); | ||
| }); | ||
|
|
||
| test('should be true for admin', () { | ||
| expect(hasPermissionToManageLessons(MemberRole.admin), isTrue); | ||
| }); | ||
|
|
||
| test('should be true for creator', () { | ||
| expect(hasPermissionToManageLessons(MemberRole.creator), isTrue); | ||
| }); | ||
|
|
||
| test('should be false for standard', () { | ||
| expect(hasPermissionToManageLessons(MemberRole.standard), isFalse); | ||
| }); | ||
| }); | ||
|
|
||
| group('hasPermissionToManageEvents', () { | ||
| group('when user is author', () { | ||
| test('should be true for owner', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.owner, true), isTrue); | ||
| }); | ||
|
|
||
| test('should be true for admin', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.admin, true), isTrue); | ||
| }); | ||
|
|
||
| test('should be true for creator', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.creator, true), isTrue); | ||
| }); | ||
|
|
||
| test('should be true for standard', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.standard, true), isTrue); | ||
| }); | ||
| }); | ||
|
|
||
| group('when user is not author', () { | ||
| test('should be true for owner', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.owner, false), isTrue); | ||
| }); | ||
|
|
||
| test('should be true for admin', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.admin, false), isTrue); | ||
| }); | ||
|
|
||
| test('should be false for creator', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.creator, false), isFalse); | ||
| }); | ||
|
|
||
| test('should be false for standard', () { | ||
| expect(hasPermissionToManageEvents(MemberRole.standard, false), isFalse); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test groups in this file contain repetitive test cases. You can use a data-driven approach to make them more concise and maintainable, reducing boilerplate code.
group('hasPermissionToManageLessons', () {
final testCases = <MemberRole, bool>{
MemberRole.owner: true,
MemberRole.admin: true,
MemberRole.creator: true,
MemberRole.standard: false,
};
for (final entry in testCases.entries) {
test('should be ${entry.value} for ${entry.key.name}', () {
expect(hasPermissionToManageLessons(entry.key), entry.value);
});
}
});
group('hasPermissionToManageEvents', () {
group('when user is author', () {
final testCases = <MemberRole, bool>{
MemberRole.owner: true,
MemberRole.admin: true,
MemberRole.creator: true,
MemberRole.standard: true,
};
for (final entry in testCases.entries) {
test('should be ${entry.value} for role ${entry.key.name}', () {
expect(hasPermissionToManageEvents(entry.key, true), entry.value);
});
}
});
group('when user is not author', () {
final testCases = <MemberRole, bool>{
MemberRole.owner: true,
MemberRole.admin: true,
MemberRole.creator: false,
MemberRole.standard: false,
};
for (final entry in testCases.entries) {
test('should be ${entry.value} for role ${entry.key.name}', () {
expect(hasPermissionToManageEvents(entry.key, false), entry.value);
});
}
});
});|
Visit the preview URL for this PR (updated for commit d11e2ad): https://sharezone-test--pr2203-add-unprotected-beha-2fzs6qpr.web.app (expires Wed, 04 Mar 2026 16:43:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b |
Added tests targeting high-risk unprotected logic around application permissions: - Tested group permissions `hasPermission` and `isUserAdminOrOwnerOfGroup` in `group_permission.dart`. - Tested timetable schedule permissions `hasPermissionToManageLessons` and `hasPermissionToManageEvents` in `timetable_permissions.dart`. - Tested submission visibility permissions `isAllowedToViewSubmittedPermissions` in `submission_permissions.dart` to prevent students from accessing other users' homework data. Also added missing license headers. Co-authored-by: nilsreichardt <24459435+nilsreichardt@users.noreply.github.com>
Objective
Continuously improve reliability by adding high-value tests for unprotected behaviors that could cause incorrect permissions or visibility.
What is now protected
hasPermissionextension onMemberRoleappropriately verifies content creation and administration rights. VerifiedisUserAdminOrOwnerOfGroupallows only owner/admin roles.hasPermissionToManageLessonschecks content creation andhasPermissionToManageEventsadequately checks both role and authorship, allowing proper management of schedules.SubmissionPermissionsclass correctly blocks standard students from viewing submissions while allowing teachers holding an admin/owner course role.What bugs it prevents
PR created automatically by Jules for task 13501131065073978994 started by @nilsreichardt