From 61a658b9ef605b26c010cf43d106eaa28ad187f9 Mon Sep 17 00:00:00 2001 From: Jon Shipley Date: Fri, 9 Dec 2022 10:08:14 +0000 Subject: [PATCH] Feature/54223 adminlogonevent school id col (#2331) * add migration * add local strategy * Add index to fk col * Update local strategy tests * Mod dfe strategy * Remove lint * admin logon event tests * Update sign_in_steps.rb Co-authored-by: Guy Harwood Co-authored-by: Mohsen Qureshi Co-authored-by: Mohsen Qureshi --- admin/authentication/local-strategy.js | 8 ++++- admin/services/dfe-signin.service.js | 5 ++++ .../authentication/local-strategy.spec.js | 5 +++- ...55.do.add-school-id-to-adminlogonevent.sql | 29 +++++++++++++++++++ ....undo.add-school-id-to-adminlogonevent.sql | 19 ++++++++++++ test/admin-hpa/features/sign_in.feature | 15 +++++++++- .../step_definitions/sign_in_steps.rb | 12 +++++++- .../features/support/sql_db_helper.rb | 6 ++++ 8 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 db/migrations/schema-objects/20221027152055.do.add-school-id-to-adminlogonevent.sql create mode 100644 db/migrations/schema-objects/20221027152055.undo.add-school-id-to-adminlogonevent.sql diff --git a/admin/authentication/local-strategy.js b/admin/authentication/local-strategy.js index 547792113f..e9dca87894 100644 --- a/admin/authentication/local-strategy.js +++ b/admin/authentication/local-strategy.js @@ -6,6 +6,7 @@ const logger = require('../services/log.service').getLogger() const userDataService = require('../services/data-access/user.data.service') const adminLogonEventDataService = require('../services/data-access/admin-logon-event.data.service') +const { isNumber, isNotNil, isInteger } = require('ramda-adjunct') module.exports = async function (req, userIdentifier, password, done) { /** @@ -16,7 +17,8 @@ module.exports = async function (req, userIdentifier, password, done) { body: JSON.stringify(R.omit(['password'], R.prop('body', req))), remoteIp: req.headers['x-forwarded-for'] || req.connection.remoteAddress, userAgent: req.headers['user-agent'], - loginMethod: 'local' + loginMethod: 'local', + school_id: null } try { @@ -48,6 +50,10 @@ module.exports = async function (req, userIdentifier, password, done) { id: user.id } + if (isNotNil(user.schoolId) && isNumber(user.schoolId) && isInteger(user.schoolId)) { + logonEvent.school_id = user.schoolId + } + // Success - valid login logonEvent.user_id = user.id await saveValidLogonEvent(logonEvent, sessionData) diff --git a/admin/services/dfe-signin.service.js b/admin/services/dfe-signin.service.js index 3e029a82e2..94a8c0b39b 100644 --- a/admin/services/dfe-signin.service.js +++ b/admin/services/dfe-signin.service.js @@ -92,6 +92,11 @@ const service = { isAuthenticated: true, authProviderSessionToken: dfeUser.id_token } + + if (schoolRecord) { + logonEvent.school_id = schoolRecord.id + } + await adminLogonEventDataService.sqlCreate(logonEvent) // set id to sql record id diff --git a/admin/spec/back-end/authentication/local-strategy.spec.js b/admin/spec/back-end/authentication/local-strategy.spec.js index 5f71e4ea60..554256161f 100644 --- a/admin/spec/back-end/authentication/local-strategy.spec.js +++ b/admin/spec/back-end/authentication/local-strategy.spec.js @@ -21,6 +21,7 @@ describe('localStrategy', () => { } } const req = httpMocks.createRequest(reqParams) + test('validates and saves a valid helpdesk user', async () => { const user = { id: 1, @@ -48,6 +49,7 @@ describe('localStrategy', () => { id: user.id }) }) + test('validates and saves a valid teacher user', async () => { const user = { id: 1, @@ -74,13 +76,14 @@ describe('localStrategy', () => { id: user.id }) }) + test('registers an invalid logon event if user is not found', async () => { jest.spyOn(userDataService, 'sqlFindUserInfoByIdentifier').mockImplementation() jest.spyOn(adminLogonEventDataService, 'sqlCreate').mockImplementation() const doneFunc = (err, res) => err || res await validateAndSave(req, 'teacher1', 'password', doneFunc) expect(adminLogonEventDataService.sqlCreate).toHaveBeenCalledWith( - { sessionId: 1, body: '{}', remoteIp: 'remoteAddress', userAgent: undefined, loginMethod: 'local', errorMsg: 'Invalid user', isAuthenticated: false } + { sessionId: 1, body: '{}', remoteIp: 'remoteAddress', userAgent: undefined, loginMethod: 'local', errorMsg: 'Invalid user', isAuthenticated: false, school_id: null } ) }) }) diff --git a/db/migrations/schema-objects/20221027152055.do.add-school-id-to-adminlogonevent.sql b/db/migrations/schema-objects/20221027152055.do.add-school-id-to-adminlogonevent.sql new file mode 100644 index 0000000000..da2235f592 --- /dev/null +++ b/db/migrations/schema-objects/20221027152055.do.add-school-id-to-adminlogonevent.sql @@ -0,0 +1,29 @@ +-- Add new school_id column +IF NOT EXISTS( + SELECT * + FROM sys.columns + WHERE object_ID = object_id('mtc_admin.adminLogonEvent') + AND col_name(object_ID, column_Id) = 'school_id') + BEGIN + ALTER TABLE [mtc_admin].[adminLogonEvent] + ADD [school_id] INT; + END + +go + +-- make it a foreign key +IF NOT EXISTS(SELECT * + FROM INFORMATION_SCHEMA.CONSTRAINT_COLUMN_USAGE + WHERE CONSTRAINT_COLUMN_USAGE.TABLE_SCHEMA = 'mtc_admin' + AND CONSTRAINT_COLUMN_USAGE.TABLE_NAME = 'adminLogonEvent' + AND CONSTRAINT_COLUMN_USAGE.COLUMN_NAME = 'school_id' + AND CONSTRAINT_NAME = 'FK_adminLogonEvent_school_id') + BEGIN + ALTER TABLE [mtc_admin].[adminLogonEvent] + ADD CONSTRAINT [FK_adminLogonEvent_school_id] + FOREIGN KEY (school_id) REFERENCES [mtc_admin].[school] (id); + END + +-- add an index +DROP INDEX IF EXISTS [mtc_admin].[adminLogonEvent].[IX_adminLogonEvent_school_id]; +CREATE INDEX IX_adminLogonEvent_school_id ON mtc_admin.adminLogonEvent([school_id]); diff --git a/db/migrations/schema-objects/20221027152055.undo.add-school-id-to-adminlogonevent.sql b/db/migrations/schema-objects/20221027152055.undo.add-school-id-to-adminlogonevent.sql new file mode 100644 index 0000000000..d14653f4f3 --- /dev/null +++ b/db/migrations/schema-objects/20221027152055.undo.add-school-id-to-adminlogonevent.sql @@ -0,0 +1,19 @@ +-- drop index +DROP INDEX IF EXISTS [mtc_admin].[adminLogonEvent].[IX_adminLogonEvent_school_id]; + +-- drop foreign key +IF EXISTS(SELECT * + FROM INFORMATION_SCHEMA.CONSTRAINT_COLUMN_USAGE + WHERE CONSTRAINT_COLUMN_USAGE.TABLE_SCHEMA = 'mtc_admin' + AND CONSTRAINT_COLUMN_USAGE.TABLE_NAME = 'adminLogonEvent' + AND CONSTRAINT_COLUMN_USAGE.COLUMN_NAME = 'school_id' + AND CONSTRAINT_NAME = 'FK_adminLogonEvent_school_id') + BEGIN + ALTER TABLE [mtc_admin].[adminLogonEvent] + DROP CONSTRAINT [FK_adminLogonEvent_school_id]; + END + + + +-- drop column +ALTER TABLE [mtc_admin].[adminLogonEvent] DROP COLUMN IF EXISTS [school_id]; diff --git a/test/admin-hpa/features/sign_in.feature b/test/admin-hpa/features/sign_in.feature index 6a20617745..9f44a16a81 100644 --- a/test/admin-hpa/features/sign_in.feature +++ b/test/admin-hpa/features/sign_in.feature @@ -21,4 +21,17 @@ Feature: Scenario: Contact page can be accessed via the footer Given I am on the sign in page When I decide to get in contact - Then I should be taken to the contact page for mtc \ No newline at end of file + Then I should be taken to the contact page for mtc + + Scenario Outline: School ID is recorded when a user logs in + Given I have logged in with + Then school ID or null should be recorded depnding if the user is linked to a school or not + + Examples: + | role | + | teacher1 | + | test-developer | + | service-manager | + | helpdesk | + | tech-support | + | sta-admin | diff --git a/test/admin-hpa/features/step_definitions/sign_in_steps.rb b/test/admin-hpa/features/step_definitions/sign_in_steps.rb index d3f5cbd608..33ad4df7cb 100644 --- a/test/admin-hpa/features/step_definitions/sign_in_steps.rb +++ b/test/admin-hpa/features/step_definitions/sign_in_steps.rb @@ -62,6 +62,16 @@ end Given(/^I have logged in with (.*)$/) do |teacher| + @teacher = teacher sign_in_page.load - sign_in_page.login(teacher, 'password') + sign_in_page.login(@teacher, 'password') +end + + +Then(/^school ID or null should be recorded depnding if the user is linked to a school or not$/) do + user = SqlDbHelper.find_teacher(@teacher) + logon_event_school_id = SqlDbHelper.login_event(user['id']).map {|a| a['school_id']}.uniq + expect(logon_event_school_id.size).to eql 1 + user_school_id = user['school_id'] + expect(user_school_id).to eql logon_event_school_id.first end diff --git a/test/admin-hpa/features/support/sql_db_helper.rb b/test/admin-hpa/features/support/sql_db_helper.rb index 815216bdc1..286a7e1820 100644 --- a/test/admin-hpa/features/support/sql_db_helper.rb +++ b/test/admin-hpa/features/support/sql_db_helper.rb @@ -704,4 +704,10 @@ def self.pupil_audit_record(pupil_id) result.each {|row| row.map} end + def self.login_event(user_id) + sql = "SELECT * FROM [mtc_admin].[adminLogonEvent] WHERE user_id = #{user_id}" + result = SQL_CLIENT.execute(sql) + result.each {|row| row.map} + end + end