diff --git a/services/web/app/src/Features/Institutions/InstitutionsFeatures.js b/services/web/app/src/Features/Institutions/InstitutionsFeatures.js index 1196b2d550..ad2b52acab 100644 --- a/services/web/app/src/Features/Institutions/InstitutionsFeatures.js +++ b/services/web/app/src/Features/Institutions/InstitutionsFeatures.js @@ -1,71 +1,43 @@ -/* eslint-disable - handle-callback-err, - max-len, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS207: Consider shorter variations of null checks - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ let InstitutionsFeatures -const InstitutionsGetter = require('./InstitutionsGetter') +const UserGetter = require('../User/UserGetter') const PlansLocator = require('../Subscription/PlansLocator') const Settings = require('settings-sharelatex') -const logger = require('logger-sharelatex') module.exports = InstitutionsFeatures = { getInstitutionsFeatures(userId, callback) { - if (callback == null) { - callback = function(error, features) {} - } - return InstitutionsFeatures.getInstitutionsPlan(userId, function( - error, - plan - ) { - if (error != null) { + InstitutionsFeatures.getInstitutionsPlan(userId, function(error, planCode) { + if (error) { return callback(error) } - plan = PlansLocator.findLocalPlanInSettings(plan) - return callback(null, (plan != null ? plan.features : undefined) || {}) + const plan = planCode && PlansLocator.findLocalPlanInSettings(planCode) + const features = plan && plan.features + callback(null, features || {}) }) }, getInstitutionsPlan(userId, callback) { - if (callback == null) { - callback = function(error, plan) {} - } - return InstitutionsFeatures.hasLicence(userId, function(error, hasLicence) { - if (error != null) { + InstitutionsFeatures.hasLicence(userId, function(error, hasLicence) { + if (error) { return callback(error) } if (!hasLicence) { return callback(null, null) } - return callback(null, Settings.institutionPlanCode) + callback(null, Settings.institutionPlanCode) }) }, hasLicence(userId, callback) { - if (callback == null) { - callback = function(error, hasLicence) {} - } - return InstitutionsGetter.getConfirmedAffiliations(userId, function( - error, - affiliations - ) { - if (error != null) { + UserGetter.getUserFullEmails(userId, function(error, emailsData) { + if (error) { return callback(error) } - const hasLicence = affiliations.some( - affiliation => affiliation.licence && affiliation.licence !== 'free' + const hasLicence = emailsData.some( + emailData => emailData.emailHasInstitutionLicence ) - return callback(null, hasLicence) + callback(null, hasLicence) }) } } diff --git a/services/web/app/src/Features/Institutions/InstitutionsHelper.js b/services/web/app/src/Features/Institutions/InstitutionsHelper.js new file mode 100644 index 0000000000..9b4a72028b --- /dev/null +++ b/services/web/app/src/Features/Institutions/InstitutionsHelper.js @@ -0,0 +1,24 @@ +function emailHasLicence(emailData) { + if (!emailData.confirmedAt) { + return false + } + if (!emailData.affiliation) { + return false + } + const affiliation = emailData.affiliation + const institution = affiliation.institution + if (!institution) { + return false + } + if (!institution.confirmed) { + return false + } + if (!affiliation.licence) { + return false + } + return affiliation.licence !== 'free' +} + +module.exports = { + emailHasLicence +} diff --git a/services/web/app/src/Features/User/UserGetter.js b/services/web/app/src/Features/User/UserGetter.js index 6fba005277..afed144c53 100644 --- a/services/web/app/src/Features/User/UserGetter.js +++ b/services/web/app/src/Features/User/UserGetter.js @@ -5,6 +5,7 @@ const { db } = mongojs const { ObjectId } = mongojs const { promisifyAll } = require('../../util/promises') const { getUserAffiliations } = require('../Institutions/InstitutionsAPI') +const InstitutionsHelper = require('../Institutions/InstitutionsHelper') const Errors = require('../Errors/Errors') const Features = require('../../infrastructure/Features') @@ -37,7 +38,10 @@ const UserGetter = { }, getUserFullEmails(userId, callback) { - this.getUser(userId, { email: 1, emails: 1 }, function(error, user) { + this.getUser(userId, { email: 1, emails: 1, samlIdentifiers: 1 }, function( + error, + user + ) { if (error) { return callback(error) } @@ -46,7 +50,10 @@ const UserGetter = { } if (!Features.hasFeature('affiliations')) { - return callback(null, decorateFullEmails(user.email, user.emails, [])) + return callback( + null, + decorateFullEmails(user.email, user.emails, [], []) + ) } getUserAffiliations(userId, function(error, affiliationsData) { @@ -55,7 +62,12 @@ const UserGetter = { } callback( null, - decorateFullEmails(user.email, user.emails || [], affiliationsData) + decorateFullEmails( + user.email, + user.emails || [], + affiliationsData, + user.samlIdentifiers || [] + ) ) }) }) @@ -147,7 +159,12 @@ const UserGetter = { } } -var decorateFullEmails = (defaultEmail, emailsData, affiliationsData) => +var decorateFullEmails = ( + defaultEmail, + emailsData, + affiliationsData, + samlIdentifiers +) => emailsData.map(function(emailData) { emailData.default = emailData.email === defaultEmail @@ -167,6 +184,18 @@ var decorateFullEmails = (defaultEmail, emailsData, affiliationsData) => emailsData.affiliation = null } + if (emailData.samlProviderId) { + emailData.samlIdentifier = samlIdentifiers.find( + samlIdentifier => samlIdentifier.providerId === emailData.samlProviderId + ) + } else { + emailsData.samlIdentifier = null + } + + emailData.emailHasInstitutionLicence = InstitutionsHelper.emailHasLicence( + emailData + ) + return emailData }) ;[ diff --git a/services/web/test/unit/src/Institutions/InstitutionHelperTests.js b/services/web/test/unit/src/Institutions/InstitutionHelperTests.js new file mode 100644 index 0000000000..dacec20d84 --- /dev/null +++ b/services/web/test/unit/src/Institutions/InstitutionHelperTests.js @@ -0,0 +1,65 @@ +const { expect } = require('chai') +const path = require('path') +const InstitutionsHelper = require(path.join( + __dirname, + '/../../../../app/src/Features/Institutions/InstitutionsHelper' +)) + +describe('InstitutionsHelper', function() { + describe('emailHasLicence', function() { + it('returns licence', function() { + const emailHasLicence = InstitutionsHelper.emailHasLicence({ + confirmedAt: new Date(), + affiliation: { + institution: { confirmed: true }, + licence: 'pro_plus' + } + }) + expect(emailHasLicence).to.be.true + }) + + it('returns false if licence is free', function() { + const emailHasLicence = InstitutionsHelper.emailHasLicence({ + confirmedAt: new Date(), + affiliation: { + institution: { confirmed: true }, + licence: 'free' + } + }) + expect(emailHasLicence).to.be.false + }) + + it('returns false if licence is null', function() { + const emailHasLicence = InstitutionsHelper.emailHasLicence({ + confirmedAt: new Date(), + affiliation: { + institution: { confirmed: true }, + licence: null + } + }) + expect(emailHasLicence).to.be.false + }) + + it('returns false if institution is not confirmed', function() { + const emailHasLicence = InstitutionsHelper.emailHasLicence({ + confirmedAt: new Date(), + affiliation: { + institution: { confirmed: false }, + licence: 'pro_plus' + } + }) + expect(emailHasLicence).to.be.false + }) + + it('returns false if email is not confirmed', function() { + const emailHasLicence = InstitutionsHelper.emailHasLicence({ + confirmedAt: null, + affiliation: { + institution: { confirmed: true }, + licence: 'pro_plus' + } + }) + expect(emailHasLicence).to.be.false + }) + }) +}) diff --git a/services/web/test/unit/src/Institutions/InstitutionsFeaturesTests.js b/services/web/test/unit/src/Institutions/InstitutionsFeaturesTests.js index 7641c25df2..6b9429e76e 100644 --- a/services/web/test/unit/src/Institutions/InstitutionsFeaturesTests.js +++ b/services/web/test/unit/src/Institutions/InstitutionsFeaturesTests.js @@ -22,7 +22,7 @@ const modulePath = require('path').join( describe('InstitutionsFeatures', function() { beforeEach(function() { - this.InstitutionsGetter = { getConfirmedAffiliations: sinon.stub() } + this.UserGetter = { getUserFullEmails: sinon.stub() } this.PlansLocator = { findLocalPlanInSettings: sinon.stub() } this.institutionPlanCode = 'institution_plan_code' this.InstitutionsFeatures = SandboxedModule.require(modulePath, { @@ -30,7 +30,7 @@ describe('InstitutionsFeatures', function() { console: console }, requires: { - './InstitutionsGetter': this.InstitutionsGetter, + '../User/UserGetter': this.UserGetter, '../Subscription/PlansLocator': this.PlansLocator, 'settings-sharelatex': { institutionPlanCode: this.institutionPlanCode @@ -47,7 +47,7 @@ describe('InstitutionsFeatures', function() { describe('hasLicence', function() { it('should handle error', function(done) { - this.InstitutionsGetter.getConfirmedAffiliations.yields(new Error('Nope')) + this.UserGetter.getUserFullEmails.yields(new Error('Nope')) return this.InstitutionsFeatures.hasLicence( this.userId, (error, hasLicence) => { @@ -57,28 +57,9 @@ describe('InstitutionsFeatures', function() { ) }) - it('should return false if user has no confirmed affiliations', function(done) { - const affiliations = [] - this.InstitutionsGetter.getConfirmedAffiliations.yields( - null, - affiliations - ) - return this.InstitutionsFeatures.hasLicence( - this.userId, - (error, hasLicence) => { - expect(error).to.not.exist - expect(hasLicence).to.be.false - return done() - } - ) - }) - it('should return false if user has no paid affiliations', function(done) { - const affiliations = [{ licence: 'free' }] - this.InstitutionsGetter.getConfirmedAffiliations.yields( - null, - affiliations - ) + const emailData = [{ emailHasInstitutionLicence: false }] + this.UserGetter.getUserFullEmails.yields(null, emailData) return this.InstitutionsFeatures.hasLicence( this.userId, (error, hasLicence) => { @@ -90,16 +71,11 @@ describe('InstitutionsFeatures', function() { }) it('should return true if user has confirmed paid affiliation', function(done) { - const affiliations = [ - { licence: 'pro_plus' }, - { licence: 'free' }, - { licence: 'pro' }, - { licence: null } + const emailData = [ + { emailHasInstitutionLicence: true }, + { emailHasInstitutionLicence: false } ] - this.InstitutionsGetter.getConfirmedAffiliations.yields( - null, - affiliations - ) + this.UserGetter.getUserFullEmails.yields(null, emailData) return this.InstitutionsFeatures.hasLicence( this.userId, (error, hasLicence) => { diff --git a/services/web/test/unit/src/User/UserGetterTests.js b/services/web/test/unit/src/User/UserGetterTests.js index 5bca1c058e..37b114fc6b 100644 --- a/services/web/test/unit/src/User/UserGetterTests.js +++ b/services/web/test/unit/src/User/UserGetterTests.js @@ -17,7 +17,11 @@ describe('UserGetter', function() { _id: '12390i', email: 'email2@foo.bar', emails: [ - { email: 'email1@foo.bar', reversedHostname: 'rab.oof' }, + { + email: 'email1@foo.bar', + reversedHostname: 'rab.oof', + confirmedAt: new Date() + }, { email: 'email2@foo.bar', reversedHostname: 'rab.oof' } ] } @@ -85,7 +89,7 @@ describe('UserGetter', function() { this.UserGetter.getUser = sinon .stub() .callsArgWith(2, null, this.fakeUser) - const projection = { email: 1, emails: 1 } + const projection = { email: 1, emails: 1, samlIdentifiers: 1 } this.UserGetter.getUserFullEmails( this.fakeUser._id, (error, fullEmails) => { @@ -111,11 +115,14 @@ describe('UserGetter', function() { { email: 'email1@foo.bar', reversedHostname: 'rab.oof', + confirmedAt: this.fakeUser.emails[0].confirmedAt, + emailHasInstitutionLicence: false, default: false }, { email: 'email2@foo.bar', reversedHostname: 'rab.oof', + emailHasInstitutionLicence: false, default: true } ]) @@ -135,7 +142,11 @@ describe('UserGetter', function() { department: 'Maths', inferred: false, licence: 'pro_plus', - institution: { name: 'University Name', isUniversity: true } + institution: { + name: 'University Name', + isUniversity: true, + confirmed: true + } } ] this.getUserAffiliations.callsArgWith(1, null, affiliationsData) @@ -147,7 +158,9 @@ describe('UserGetter', function() { { email: 'email1@foo.bar', reversedHostname: 'rab.oof', + confirmedAt: this.fakeUser.emails[0].confirmedAt, default: false, + emailHasInstitutionLicence: true, affiliation: { institution: affiliationsData[0].institution, inferred: affiliationsData[0].inferred, @@ -159,6 +172,42 @@ describe('UserGetter', function() { { email: 'email2@foo.bar', reversedHostname: 'rab.oof', + emailHasInstitutionLicence: false, + default: true + } + ]) + done() + } + ) + }) + + it('should merge SAML identifier', function(done) { + const fakeSamlIdentifiers = [ + { providerId: 'saml_id', exteranlUserId: 'whatever' } + ] + const fakeUserWithSaml = this.fakeUser + fakeUserWithSaml.emails[0].samlProviderId = 'saml_id' + fakeUserWithSaml.samlIdentifiers = fakeSamlIdentifiers + this.UserGetter.getUser = sinon.stub().yields(null, this.fakeUser) + this.getUserAffiliations.callsArgWith(1, null, []) + this.UserGetter.getUserFullEmails( + this.fakeUser._id, + (error, fullEmails) => { + expect(error).to.not.exist + assert.deepEqual(fullEmails, [ + { + email: 'email1@foo.bar', + reversedHostname: 'rab.oof', + confirmedAt: this.fakeUser.emails[0].confirmedAt, + default: false, + emailHasInstitutionLicence: false, + samlProviderId: 'saml_id', + samlIdentifier: fakeSamlIdentifiers[0] + }, + { + email: 'email2@foo.bar', + reversedHostname: 'rab.oof', + emailHasInstitutionLicence: false, default: true } ]) @@ -175,7 +224,7 @@ describe('UserGetter', function() { this.UserGetter.getUser = sinon .stub() .callsArgWith(2, null, this.fakeUser) - const projection = { email: 1, emails: 1 } + const projection = { email: 1, emails: 1, samlIdentifiers: 1 } this.UserGetter.getUserFullEmails( this.fakeUser._id, (error, fullEmails) => {