From 4603ebaad5ab751ae0ff26f0fc8eede1dd081802 Mon Sep 17 00:00:00 2001 From: Aidin Niavarani Date: Wed, 13 Dec 2017 16:38:02 -0800 Subject: [PATCH] Allow admins to view and delete LTI users and third party users associated with each user - Change 'Courses' link on Manage Users screen for each user to Courses & Accounts, linking to new page - Change User Courses page to User Courses & Accounts page - Add two sections to list user's LTI and their third party account links to the User Courses & Accounts page - Add uuid to the lti_user and third_party_user tables - Add functionality to delete third party users and unlink lti users - Add relevant backend and frontend acceptance tests - Remove Manage LTI Links button from course page - Change X icon on Manage LTI Links page (Unlink context) to trash icon for consistency --- compair/api/__init__.py | 9 +- compair/api/dataformat/__init__.py | 17 ++ compair/api/users.py | 115 +++++++++- compair/models/lti_models/lti_user.py | 13 ++ compair/models/third_party_user.py | 12 +- compair/static/compair-config.js | 30 ++- compair/static/less/compair.less | 4 +- compair/static/less/lti_manage.less | 4 +- .../{user_courses.less => user_manage.less} | 6 +- .../course/course-assignments-partial.html | 7 - .../lti-contexts-list-partial.html | 2 +- .../modules/route/route-provider_spec.js | 20 +- .../modules/user/user-list-partial.html | 2 +- ...-partial.html => user-manage-partial.html} | 86 ++++++- compair/static/modules/user/user-module.js | 62 +++++- .../test/factories/http_backend_mocks.js | 95 ++++++++ .../static/test/factories/lti_user_factory.js | 23 ++ .../factories/third_party_user_factory.js | 24 ++ .../test/features/step_definitions/common.js | 2 +- .../step_definitions/view_user_courses.js | 71 ------ .../step_definitions/view_user_manage.js | 135 +++++++++++ .../features/step_definitions/view_users.js | 11 +- .../test/features/view_user_courses.feature | 49 ---- .../test/features/view_user_manage.feature | 89 ++++++++ .../test/fixtures/admin/default_fixture.js | 52 ++++- .../static/test/page_objects/user_courses.js | 7 - .../static/test/page_objects/user_manage.js | 7 + compair/tests/api/test_users.py | 209 +++++++++++++++++- 28 files changed, 989 insertions(+), 174 deletions(-) rename compair/static/less/{user_courses.less => user_manage.less} (58%) rename compair/static/modules/user/{user-course-partial.html => user-manage-partial.html} (62%) create mode 100644 compair/static/test/factories/lti_user_factory.js create mode 100644 compair/static/test/factories/third_party_user_factory.js delete mode 100644 compair/static/test/features/step_definitions/view_user_courses.js create mode 100644 compair/static/test/features/step_definitions/view_user_manage.js delete mode 100644 compair/static/test/features/view_user_courses.feature create mode 100644 compair/static/test/features/view_user_manage.feature delete mode 100644 compair/static/test/page_objects/user_courses.js create mode 100644 compair/static/test/page_objects/user_manage.js diff --git a/compair/api/__init__.py b/compair/api/__init__.py index 28d79ee41..4e807d84f 100644 --- a/compair/api/__init__.py +++ b/compair/api/__init__.py @@ -236,7 +236,9 @@ def register_demo_api_blueprints(app): def log_events(log): # user events from .users import on_user_modified, on_user_get, on_user_list_get, on_user_create, on_user_course_get, \ - on_user_password_update, on_user_edit_button_get, on_teaching_course_get, on_user_notifications_update + on_user_password_update, on_user_edit_button_get, on_teaching_course_get, on_user_notifications_update, \ + on_user_course_status_get, on_user_lti_users_get, on_user_lti_user_unlink, on_user_third_party_users_get, \ + on_user_third_party_user_delete on_user_modified.connect(log) on_user_get.connect(log) on_user_list_get.connect(log) @@ -246,6 +248,11 @@ def log_events(log): on_user_edit_button_get.connect(log) on_user_notifications_update.connect(log) on_user_password_update.connect(log) + on_user_course_status_get.connect(log) + on_user_lti_users_get.connect(log) + on_user_lti_user_unlink.connect(log) + on_user_third_party_users_get.connect(log) + on_user_third_party_user_delete.connect(log) # course events from .course import on_course_modified, on_course_get, on_course_list_get, on_course_create, \ diff --git a/compair/api/dataformat/__init__.py b/compair/api/dataformat/__init__.py index ca3eb8d67..53e0e8339 100644 --- a/compair/api/dataformat/__init__.py +++ b/compair/api/dataformat/__init__.py @@ -81,6 +81,23 @@ def get_users_in_course(restrict_user=True): return users +def get_lti_user(): + return { + 'id': fields.String(attribute="uuid"), + 'lti_consumer_id': fields.String(attribute="lti_consumer_uuid"), + 'lti_user_id': fields.String(attribute="user_id"), + 'compair_user_id': fields.String(attribute="compair_user_uuid"), + 'oauth_consumer_key': fields.String, + 'lis_person_name_full': fields.String + } + +def get_third_party_user(): + return { + 'id': fields.String(attribute="uuid"), + 'third_party_type': UnwrapEnum(attribute="third_party_type"), + 'unique_identifier': fields.String(attribute="unique_identifier"), + 'compair_user_id': fields.String(attribute="compair_user_uuid") + } def get_course(): return { diff --git a/compair/api/users.py b/compair/api/users.py index 1989029d4..a1c1503e3 100644 --- a/compair/api/users.py +++ b/compair/api/users.py @@ -11,8 +11,8 @@ from compair.authorization import is_user_access_restricted, require, allow from compair.core import db, event, abort from .util import new_restful_api, get_model_changes, pagination_parser -from compair.models import User, SystemRole, Course, UserCourse, CourseRole, \ - Assignment, LTIUser, LTIUserResourceLink, LTIContext, ThirdPartyUser, ThirdPartyType, \ +from compair.models import User, SystemRole, Course, UserCourse, CourseRole, Assignment, \ + LTIConsumer, LTIUser, LTIUserResourceLink, LTIContext, ThirdPartyUser, ThirdPartyType, \ Answer, Comparison, AnswerComment, AnswerCommentType, EmailNotificationMethod from compair.api.login import authenticate from distutils.util import strtobool @@ -90,6 +90,10 @@ def string_to_bool(value): on_user_edit_button_get = event.signal('USER_EDIT_BUTTON_GET') on_user_notifications_update = event.signal('USER_NOTIFICATIONS_UPDATE') on_user_password_update = event.signal('USER_PASSWORD_UPDATE') +on_user_lti_users_get = event.signal('USER_LTI_USERS_GET') +on_user_lti_user_unlink = event.signal('USER_LTI_USER_UNLINK') +on_user_third_party_users_get = event.signal('USER_THIRD_PARTY_USERS_GET') +on_user_third_party_user_delete = event.signal('USER_THIRD_PARTY_USER_DELETE') def check_valid_system_role(system_role): system_roles = [ @@ -597,6 +601,109 @@ def get(self): return {"statuses": statuses} +# /id/lti/users +class UserLTIListAPI(Resource): + @login_required + def get(self, user_uuid): + + user = User.get_by_uuid_or_404(user_uuid) + + require(MANAGE, User, + title="User's LTI Account Links Unavailable", + message="Sorry, your system role does not allow you to view LTI account links for this user.") + + lti_users = user.lti_user_links \ + .order_by( + LTIUser.lti_consumer_id, + LTIUser.user_id + ) \ + .all() + + on_user_lti_users_get.send( + self, + event_name=on_user_lti_users_get.name, + user=current_user, + data={'user_id': user.id}) + + return {"objects": marshal(lti_users, dataformat.get_lti_user())} + +# /id/lti/users/uuid +class UserLTIAPI(Resource): + @login_required + def delete(self, user_uuid, lti_user_uuid): + """ + unlink lti user from compair user + """ + + user = User.get_by_uuid_or_404(user_uuid) + lti_user = LTIUser.get_by_uuid_or_404(lti_user_uuid) + + require(MANAGE, User, + title="User's LTI Account Links Unavailable", + message="Sorry, your system role does not allow you to remove LTI account links for this user.") + + lti_user.compair_user_id = None + db.session.commit() + + on_user_lti_user_unlink.send( + self, + event_name=on_user_lti_user_unlink.name, + user=current_user, + data={'user_id': user.id, 'lti_user_id': lti_user.id}) + + return { 'success': True } + +# /id/third_party/users +class UserThirdPartyUserListAPI(Resource): + @login_required + def get(self, user_uuid): + + user = User.get_by_uuid_or_404(user_uuid) + + require(MANAGE, User, + title="User's Third Party Logins Unavailable", + message="Sorry, your system role does not allow you to view third party logins for this user.") + + third_party_users = ThirdPartyUser.query \ + .filter(ThirdPartyUser.user_id == user.id) \ + .order_by( + ThirdPartyUser.third_party_type, + ThirdPartyUser.unique_identifier + ) \ + .all() + + on_user_third_party_users_get.send( + self, + event_name=on_user_third_party_users_get.name, + user=current_user, + data={'user_id': user.id}) + + return {"objects": marshal(third_party_users, dataformat.get_third_party_user())} + +#/id/third_party/users/uuid +class UserThirdPartyUserAPI(Resource): + @login_required + def delete(self, user_uuid, third_party_user_uuid): + + user = User.get_by_uuid_or_404(user_uuid) + third_party_user = ThirdPartyUser.get_by_uuid_or_404(third_party_user_uuid) + + require(MANAGE, User, + title="User's Third Party Logins Unavailable", + message="Sorry, your system role does not allow you to delete third party connections for this user.") + + on_user_third_party_user_delete.send( + self, + event_name=on_user_third_party_user_delete.name, + user=current_user, + data={'user_id': user.id, 'third_party_type': third_party_user.third_party_type, 'unique_identifier': third_party_user.unique_identifier}) + + # TODO: consider adding soft delete to thrid_party_user in the future + ThirdPartyUser.query.filter(ThirdPartyUser.uuid == third_party_user_uuid).delete() + db.session.commit() + + return { 'success': True } + # courses/teaching class TeachingUserCourseListAPI(Resource): @login_required @@ -697,6 +804,10 @@ def post(self, user_uuid): api.add_resource(UserAPI, '/') api.add_resource(UserListAPI, '') api.add_resource(UserCourseListAPI, '//courses') +api.add_resource(UserThirdPartyUserAPI, '//third_party/users/') +api.add_resource(UserThirdPartyUserListAPI, '//third_party/users') +api.add_resource(UserLTIAPI, '//lti/users/') +api.add_resource(UserLTIListAPI, '//lti/users') api.add_resource(CurrentUserCourseListAPI, '/courses') api.add_resource(UserCourseStatusListAPI, '/courses/status') api.add_resource(TeachingUserCourseListAPI, '/courses/teaching') diff --git a/compair/models/lti_models/lti_user.py b/compair/models/lti_models/lti_user.py index 9df402a1b..41bcb6883 100644 --- a/compair/models/lti_models/lti_user.py +++ b/compair/models/lti_models/lti_user.py @@ -1,6 +1,7 @@ # sqlalchemy from sqlalchemy import func, select, and_, or_ from sqlalchemy.ext.hybrid import hybrid_property +from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy_enum34 import EnumType from . import * @@ -29,6 +30,10 @@ class LTIUser(DefaultTableMixin, UUIDMixin, WriteTrackingMixin): lti_user_resource_links = db.relationship("LTIUserResourceLink", backref="lti_user", lazy="dynamic") # hyprid and other functions + lti_consumer_uuid = association_proxy('lti_consumer', 'uuid') + oauth_consumer_key = association_proxy('lti_consumer', 'oauth_consumer_key') + compair_user_uuid = association_proxy('compair_user', 'uuid') + def is_linked_to_user(self): return self.compair_user_id != None @@ -75,6 +80,14 @@ def get_by_tool_provider(cls, lti_consumer, tool_provider): return lti_user + @classmethod + def get_by_uuid_or_404(cls, model_uuid, joinedloads=[], title=None, message=None): + if not title: + title = "LTI User Unavailable" + if not message: + message = "Sorry, this LTI user was deleted or is no longer accessible." + return super(cls, cls).get_by_uuid_or_404(model_uuid, joinedloads, title, message) + def upgrade_system_role(self): # upgrade system role is needed if self.is_linked_to_user(): diff --git a/compair/models/third_party_user.py b/compair/models/third_party_user.py index 98abd4f7b..2a0b8b6d3 100644 --- a/compair/models/third_party_user.py +++ b/compair/models/third_party_user.py @@ -9,6 +9,7 @@ from sqlalchemy.orm import synonym from sqlalchemy import func, select, and_, or_ from sqlalchemy.ext.hybrid import hybrid_property +from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy_enum34 import EnumType from . import * @@ -28,6 +29,7 @@ class ThirdPartyUser(DefaultTableMixin, UUIDMixin, WriteTrackingMixin): # relationships # user via User Model + compair_user_uuid = association_proxy('user', 'uuid') # hyprid and other functions @@ -47,4 +49,12 @@ def __declare_last__(cls): # prevent duplicate user in course db.UniqueConstraint('third_party_type', 'unique_identifier', name='_unique_third_party_type_and_unique_identifier'), DefaultTableMixin.default_table_args - ) \ No newline at end of file + ) + + @classmethod + def get_by_uuid_or_404(cls, model_uuid, joinedloads=[], title=None, message=None): + if not title: + title = "Third Party User Unavailable" + if not message: + message = "Sorry, this third party user was deleted or is no longer accessible." + return super(cls, cls).get_by_uuid_or_404(model_uuid, joinedloads, title, message) \ No newline at end of file diff --git a/compair/static/compair-config.js b/compair/static/compair-config.js index a0ebcf866..11b59773d 100644 --- a/compair/static/compair-config.js +++ b/compair/static/compair-config.js @@ -135,10 +135,12 @@ myApp.factory('RouteResolves', "CourseResource", "AssignmentResource", "ClassListResource", "GroupResource", "UserResource", "ComparisonExampleResource", "CriterionResource", "AssignmentCommentResource", "AnswerResource", "TimerResource", "GradebookResource", "LTIConsumerResource", + "UserLTIUsersResource", "UserThirdPartyUsersResource", "AuthTypesEnabled", function($q, $route, Toaster, Authorize, Session, LTI, CourseResource, AssignmentResource, ClassListResource, GroupResource, UserResource, ComparisonExampleResource, CriterionResource, AssignmentCommentResource, - AnswerResource, TimerResource, GradebookResource, LTIConsumerResource) + AnswerResource, TimerResource, GradebookResource, LTIConsumerResource, + UserLTIUsersResource, UserThirdPartyUsersResource, AuthTypesEnabled) { return { @@ -245,6 +247,20 @@ myApp.factory('RouteResolves', var assignmentId = $route.current.params.assignmentId; return AnswerResource.userUnsaved({'courseId': courseId, 'assignmentId': assignmentId}).$promise; }, + userLTIs: function() { + if (AuthTypesEnabled.lti) { + var userId = $route.current.params.userId; + return UserLTIUsersResource.get({'id': userId}).$promise; + } + return $q.when({'objects':[]}); + }, + userThirdPartyUsers: function() { + if (AuthTypesEnabled.cas) { + var userId = $route.current.params.userId; + return UserThirdPartyUsersResource.get({'id': userId}).$promise; + } + return $q.when({'objects':[]}); + }, //other timer: function() { @@ -641,17 +657,19 @@ myApp.config( } } }) - .when('/users/:userId/course', + .when('/users/:userId/manage', { - title: "Manage User Courses", - templateUrl: 'modules/user/user-course-partial.html', - label: "Manage User Courses", - controller: 'UserCourseController', + title: "User Courses & Accounts", + templateUrl: 'modules/user/user-manage-partial.html', + label: "User Courses & Accounts", + controller: 'UserManageController', resolve: { resolvedData: function() { return ResolveDeferredRouteData({ user: RouteResolves.user(), canManageUsers: RouteResolves.canManageUsers(), + userLTIs: RouteResolves.userLTIs(), + userThirdPartyUsers: RouteResolves.userThirdPartyUsers() }, ['user']); } } diff --git a/compair/static/less/compair.less b/compair/static/less/compair.less index 6c43f52c3..c304b381d 100644 --- a/compair/static/less/compair.less +++ b/compair/static/less/compair.less @@ -18,8 +18,8 @@ // USERS SCREEN @import 'users.less'; -// USER COURSES SCREEN -@import 'user_courses.less'; +// USER MANAGE SCREEN +@import 'user_manage.less'; // COURSE SCREEN @import 'course.less'; diff --git a/compair/static/less/lti_manage.less b/compair/static/less/lti_manage.less index bcf283bff..5b0b2a48b 100644 --- a/compair/static/less/lti_manage.less +++ b/compair/static/less/lti_manage.less @@ -1,9 +1,9 @@ -/***** USER COURSES SCREEN *****/ +/***** USER MANAGE SCREEN *****/ .lti-contexts-screen { .search-contexts { margin-top: 1.7em; } -}//closes user-courses-screen +}//closes user-manage-screen diff --git a/compair/static/less/user_courses.less b/compair/static/less/user_manage.less similarity index 58% rename from compair/static/less/user_courses.less rename to compair/static/less/user_manage.less index 80988a5c1..22b2d0231 100644 --- a/compair/static/less/user_courses.less +++ b/compair/static/less/user_manage.less @@ -1,7 +1,7 @@ -/***** USER COURSES SCREEN *****/ +/***** USER MANAGE SCREEN *****/ -.user-courses-screen { +.user-manage-screen { .search-courses { margin-top: 1.7em; @@ -12,5 +12,5 @@ } -}//closes user-courses-screen +}//closes user-manage-screen diff --git a/compair/static/modules/course/course-assignments-partial.html b/compair/static/modules/course/course-assignments-partial.html index b79c1b503..0e340d577 100644 --- a/compair/static/modules/course/course-assignments-partial.html +++ b/compair/static/modules/course/course-assignments-partial.html @@ -24,13 +24,6 @@

{{course.name}} ({{course.year}} {{course.term}}) Edit Course - - - - Manage LTI Links - - diff --git a/compair/static/modules/lti_context/lti-contexts-list-partial.html b/compair/static/modules/lti_context/lti-contexts-list-partial.html index 795490fc7..55b983410 100644 --- a/compair/static/modules/lti_context/lti-contexts-list-partial.html +++ b/compair/static/modules/lti_context/lti-contexts-list-partial.html @@ -32,7 +32,7 @@

LTI links

- + diff --git a/compair/static/modules/route/route-provider_spec.js b/compair/static/modules/route/route-provider_spec.js index 1ee462bd5..0fc6c670b 100644 --- a/compair/static/modules/route/route-provider_spec.js +++ b/compair/static/modules/route/route-provider_spec.js @@ -887,12 +887,14 @@ describe('user-module', function () { }); }); - describe('"/users/:userId/course"', function() { - var path = '/users/'+mockUserId+'/course'; + describe('"/users/:userId/manage"', function() { + var path = '/users/'+mockUserId+'/manage'; it('should handle pre-loading errors', function() { $httpBackend.expectGET('/api/users/'+mockUserId).respond(404, ''); - $httpBackend.expectGET('modules/user/user-course-partial.html').respond(''); + $httpBackend.expectGET('/api/users/'+mockUserId+'/lti/users').respond({}); + $httpBackend.expectGET('/api/users/'+mockUserId+'/third_party/users').respond({}); + $httpBackend.expectGET('modules/user/user-manage-partial.html').respond(''); expect($route.current).toBeUndefined(); $location.path(path); @@ -905,13 +907,15 @@ describe('user-module', function () { expect(toaster.error).toHaveBeenCalled(); expect($rootScope.routeResolveLoadError).not.toBeUndefined(); - expect($route.current.templateUrl).toBe('modules/user/user-course-partial.html'); - expect($route.current.controller).toBe('UserCourseController'); + expect($route.current.templateUrl).toBe('modules/user/user-manage-partial.html'); + expect($route.current.controller).toBe('UserManageController'); }); it('should load correctly', function() { $httpBackend.expectGET('/api/users/'+mockUserId).respond({}); - $httpBackend.expectGET('modules/user/user-course-partial.html').respond(''); + $httpBackend.expectGET('/api/users/'+mockUserId+'/lti/users').respond({}); + $httpBackend.expectGET('/api/users/'+mockUserId+'/third_party/users').respond({}); + $httpBackend.expectGET('modules/user/user-manage-partial.html').respond(''); expect($route.current).toBeUndefined(); $location.path(path); @@ -923,8 +927,8 @@ describe('user-module', function () { expect(toaster.error).not.toHaveBeenCalled(); expect($rootScope.routeResolveLoadError).toBeUndefined(); - expect($route.current.templateUrl).toBe('modules/user/user-course-partial.html'); - expect($route.current.controller).toBe('UserCourseController'); + expect($route.current.templateUrl).toBe('modules/user/user-manage-partial.html'); + expect($route.current.controller).toBe('UserManageController'); }); }); diff --git a/compair/static/modules/user/user-list-partial.html b/compair/static/modules/user/user-list-partial.html index 87e6b843e..6f543126e 100644 --- a/compair/static/modules/user/user-list-partial.html +++ b/compair/static/modules/user/user-list-partial.html @@ -40,7 +40,7 @@

Manage Users

Edit  |  - Manage Courses + Courses & Accounts {{user.displayname}} {{user.firstname}} diff --git a/compair/static/modules/user/user-course-partial.html b/compair/static/modules/user/user-manage-partial.html similarity index 62% rename from compair/static/modules/user/user-course-partial.html rename to compair/static/modules/user/user-manage-partial.html index c5b42aeb1..4a82cebd1 100644 --- a/compair/static/modules/user/user-course-partial.html +++ b/compair/static/modules/user/user-manage-partial.html @@ -1,8 +1,12 @@ -
+
+ +

{{ user.displayname }}'s Courses & Accounts

+ +

-

Manage {{ user.displayname }}'s Courses

+

Courses