From dcf1b9bf0d551c0de0c313ba22e999f3cc202a6e Mon Sep 17 00:00:00 2001 From: Drew Bollinger Date: Wed, 29 Jan 2025 12:19:08 -0500 Subject: [PATCH 1/2] chore: remove errant login logging --- api/admin/passport.js | 20 +++-------- api/services/passport.js | 6 +--- api/services/uaaStrategy.js | 24 +++++++++---- test/api/unit/services/uaaStrategy.test.js | 40 ++++++++++++++-------- 4 files changed, 48 insertions(+), 42 deletions(-) diff --git a/api/admin/passport.js b/api/admin/passport.js index aeb04feb5..6fda9b631 100644 --- a/api/admin/passport.js +++ b/api/admin/passport.js @@ -31,24 +31,12 @@ const uaaOptions = { const verify = async (req, accessToken, refreshToken, profile, callback) => { try { - const supportUser = await verifyUAAUser(accessToken, refreshToken, profile, [ - 'pages.support', - ]); + const { user, role } = await verifyUAAUser(accessToken, refreshToken, profile); - if (supportUser) { + if (user && role) { return callback(null, { - ...supportUser.dataValues, - role: 'pages.support', - }); - } - - const adminUser = await verifyUAAUser(accessToken, refreshToken, profile, [ - 'pages.admin', - ]); - if (adminUser) { - return callback(null, { - ...adminUser.dataValues, - role: 'pages.admin', + ...user.dataValues, + role, }); } diff --git a/api/services/passport.js b/api/services/passport.js index 9cf1b045f..33578e57a 100644 --- a/api/services/passport.js +++ b/api/services/passport.js @@ -76,11 +76,7 @@ const uaaOptions = { const verifyUAA = async (accessToken, refreshToken, profile, callback) => { try { - const user = await verifyUAAUser(accessToken, refreshToken, profile, [ - 'pages.user', - 'pages.support', - 'pages.admin', - ]); + const { user } = await verifyUAAUser(accessToken, refreshToken, profile); if (!user) return callback(null, false, flashMessage); diff --git a/api/services/uaaStrategy.js b/api/services/uaaStrategy.js index e8d741077..7cb63aef5 100644 --- a/api/services/uaaStrategy.js +++ b/api/services/uaaStrategy.js @@ -34,12 +34,18 @@ function createUAAStrategy(options, verify) { return strategy; } -async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) { +async function verifyUAAUser(accessToken, refreshToken, profile) { const { user_id: uaaId, email } = profile; const client = new UAAClient(); - const isVerified = await client.verifyUserGroup(uaaId, uaaGroups); - if (!isVerified) { + const clientToken = await client.fetchClientToken(); + const { groups, origin, verified } = await client.fetchUser(uaaId, clientToken); + const userGroups = groups.map((g) => g.display).filter((g) => g.startsWith('pages')); + + // the profile isn't verified if: + // unverified and cloud.gov origin OR + // no pages group membership + if ((origin === 'cloud.gov' && !verified) || !userGroups.length) { EventCreator.audit( Event.labels.AUTHENTICATION, null, @@ -49,7 +55,7 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) { }, ); - return null; + return { user: null, role: null }; } const identity = await UAAIdentity.findOne({ @@ -70,7 +76,7 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) { profile, }, ); - return null; + return { user: null, role: null }; } if (!identity.User) { @@ -83,7 +89,7 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) { identity, }, ); - return null; + return { user: null, role: null }; } await identity.update({ @@ -92,7 +98,11 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) { refreshToken, }); - return identity.User; + // add role based on highest permissioned user group + const ordereredRoles = ['pages.admin', 'pages.support', 'pages.user']; + const role = ordereredRoles.find((or) => userGroups.includes(or)); + + return { user: identity.User, role }; } module.exports = { diff --git a/test/api/unit/services/uaaStrategy.test.js b/test/api/unit/services/uaaStrategy.test.js index de426cd9b..940d285e8 100644 --- a/test/api/unit/services/uaaStrategy.test.js +++ b/test/api/unit/services/uaaStrategy.test.js @@ -45,7 +45,7 @@ describe('verifyUAAUser', () => { uaaId, groups: [ { - display: 'group.one', + display: 'pages.user', }, { display: 'group.two', @@ -63,12 +63,15 @@ describe('verifyUAAUser', () => { expect(identity.accessToken).to.be.null; expect(identity.refreshToken).to.be.null; - const verifiedUser = await verifyUAAUser(accessToken, refreshToken, uaaUserProfile, [ - 'group.one', - ]); + const { user: verifiedUser, role } = await verifyUAAUser( + accessToken, + refreshToken, + uaaUserProfile, + ); await identity.reload(); + expect(role).to.be.equal('pages.user'); expect(verifiedUser.dataValues).to.deep.equal(user.dataValues); expect(identity.accessToken).to.equal(accessToken); expect(identity.refreshToken).to.equal(refreshToken); @@ -99,11 +102,14 @@ describe('verifyUAAUser', () => { cfUAANock.mockVerifyUserGroup(uaaId, uaaUserResponse); - const result = await verifyUAAUser(accessToken, refreshToken, uaaUserProfile, [ - 'group.three', - ]); + const { user: result, role } = await verifyUAAUser( + accessToken, + refreshToken, + uaaUserProfile, + ); expect(eventAuditStub.called).to.equal(true); + expect(role).to.be.null; return expect(result).to.be.null; }); @@ -131,11 +137,14 @@ describe('verifyUAAUser', () => { cfUAANock.mockVerifyUserGroup(uaaId, uaaUserResponse); - const result = await verifyUAAUser(accessToken, refreshToken, uaaUserProfile, [ - 'group.three', - ]); + const { user: result, role } = await verifyUAAUser( + accessToken, + refreshToken, + uaaUserProfile, + ); expect(eventAuditStub.called).to.equal(true); + expect(role).to.be.null; return expect(result).to.be.null; }); @@ -149,7 +158,7 @@ describe('verifyUAAUser', () => { uaaId, groups: [ { - display: 'group.one', + display: 'pages.user', }, { display: 'group.two', @@ -166,11 +175,14 @@ describe('verifyUAAUser', () => { await user.destroy(); - const result = await verifyUAAUser(accessToken, refreshToken, uaaUserProfile, [ - 'group.one', - ]); + const { user: result, role } = await verifyUAAUser( + accessToken, + refreshToken, + uaaUserProfile, + ); expect(eventAuditStub.called).to.equal(true); + expect(role).to.be.null; return expect(result).to.be.null; }); }); From b5ce70d1cce88a83e3b119842cdb352fc4c4eab1 Mon Sep 17 00:00:00 2001 From: Drew Bollinger Date: Thu, 6 Feb 2025 09:58:41 -0500 Subject: [PATCH 2/2] chore: move initial uaa group checking to login/verify only --- api/admin/controllers/user.js | 1 + api/controllers/organization.js | 1 + api/services/organization/Organization.js | 64 +++------- api/utils/uaaClient.js | 17 --- test/api/admin/requests/admin-auth.test.js | 8 +- test/api/requests/auth.test.js | 4 +- test/api/support/cfUAANock.js | 4 +- test/api/unit/services/organization.test.js | 36 +----- test/api/unit/services/uaaStrategy.test.js | 87 ++++++++++++- test/api/unit/utils/uaaClient.test.js | 133 -------------------- 10 files changed, 113 insertions(+), 242 deletions(-) diff --git a/api/admin/controllers/user.js b/api/admin/controllers/user.js index 5ddcd5c91..b9279dc1c 100644 --- a/api/admin/controllers/user.js +++ b/api/admin/controllers/user.js @@ -169,6 +169,7 @@ module.exports = wrapHandlers({ toInt(organizationId), toInt(roleId), uaaEmail, + true, ); EventCreator.audit(Event.labels.ADMIN_ACTION, req.user, 'User Invited', { organizationId, diff --git a/api/controllers/organization.js b/api/controllers/organization.js index 02b0b3d01..8a5982115 100644 --- a/api/controllers/organization.js +++ b/api/controllers/organization.js @@ -61,6 +61,7 @@ module.exports = wrapHandlers({ org.id, toInt(roleId), uaaEmail, + false, ); // TODO - refactor above method to return user so this extra query is not necessary diff --git a/api/services/organization/Organization.js b/api/services/organization/Organization.js index bdb29ddca..a2665a7d9 100644 --- a/api/services/organization/Organization.js +++ b/api/services/organization/Organization.js @@ -59,17 +59,6 @@ module.exports = { return uaaClient.inviteUserToUserGroup(targetUserEmail, accessToken); }, - /** - * @param {UAAIdentityType} currentUserUAAIdentity - */ - async isUAAAdmin(currentUserUAAIdentity) { - const uaaClient = new UAAClient(); - return uaaClient.verifyUserGroup(currentUserUAAIdentity.uaaId, [ - 'pages.admin', - 'pages.support', - ]); - }, - /** * @param {string} uaaEmail * @returns {Promise} @@ -143,9 +132,16 @@ module.exports = { * @param {number} organizationId * @param {number} roleId * @param {string} targetUserEmail - the email address of the user to invite + * @param {boolean} isAdmin - called from the admin API * @returns {Promise} */ - async inviteUserToOrganization(currentUser, organizationId, roleId, targetUserEmail) { + async inviteUserToOrganization( + currentUser, + organizationId, + roleId, + targetUserEmail, + isAdmin = false, + ) { const currentUserUAAIdentity = await currentUser.getUAAIdentity(); if (!currentUserUAAIdentity) { @@ -155,20 +151,17 @@ module.exports = { ); } - const [isAdmin, org] = await Promise.all([ - this.isUAAAdmin(currentUserUAAIdentity), - Organization.findOne({ - where: { - id: organizationId, + const org = await Organization.findOne({ + where: { + id: organizationId, + }, + include: [ + { + model: OrganizationRole, + include: [Role, User], }, - include: [ - { - model: OrganizationRole, - include: [Role, User], - }, - ], - }), - ]); + ], + }); if (!isAdmin && !hasManager(org, currentUser)) { throwError( @@ -213,32 +206,13 @@ module.exports = { ); } - const isAdmin = await this.isUAAAdmin(currentUserUAAIdentity); - - if (!isAdmin) { - throwError( - // eslint-disable-next-line max-len - `Current user ${currentUser.username} must be a Pages admin in UAA to create an organization.`, - ); - } - const [user, uaaUserAttributes] = await this.findOrCreateUAAUser( currentUserUAAIdentity, targetUserEmail, ); - const managerRole = await Role.findOne({ - where: { - name: 'manager', - }, - }); - const org = await Organization.create(organizationParams); - await org.addUser(user, { - through: { - roleId: managerRole.id, - }, - }); + await org.addRoleUser(user, 'manager'); return [org, uaaUserAttributes]; }, diff --git a/api/utils/uaaClient.js b/api/utils/uaaClient.js index a408d0f5c..33a715373 100644 --- a/api/utils/uaaClient.js +++ b/api/utils/uaaClient.js @@ -88,23 +88,6 @@ class UAAClient { return userInviteAttributes; } - /** - * @param {string} userId - a UAA user id - * @param {[string]} groupNames - allowed UAA group names, ex: ['pages.user'] - * - * Verifies that the UAA user is in the specified UAA group - */ - async verifyUserGroup(userId, groupNames) { - const clientToken = await this.fetchClientToken(); - const { groups, origin, verified } = await this.fetchUser(userId, clientToken); - - if (origin === 'cloud.gov' && !verified) { - return false; - } - - return this.userInGroup(groups, groupNames); - } - /** * * Utility diff --git a/test/api/admin/requests/admin-auth.test.js b/test/api/admin/requests/admin-auth.test.js index 7d53b8e0f..bef0d4279 100644 --- a/test/api/admin/requests/admin-auth.test.js +++ b/test/api/admin/requests/admin-auth.test.js @@ -54,9 +54,7 @@ describe('Admin authentication request', () => { }); cfUAANock.mockUAAAuth(profile, code); - cfUAANock.mockVerifyUserGroup(uaaId, userProfile); - // mocked twice for subsequent calls - cfUAANock.mockVerifyUserGroup(uaaId, userProfile); + cfUAANock.mockVerifyUser(uaaId, userProfile); return request(app) .get(`/admin/auth/uaa/callback?code=${code}&state=abc123`) @@ -92,9 +90,7 @@ describe('Admin authentication request', () => { beforeEach(() => { cfUAANock.mockUAAAuth(uaaUserProfile, code); - cfUAANock.mockVerifyUserGroup(uaaId, uaaUserInfo); - // mocked twice for subsequent calls - cfUAANock.mockVerifyUserGroup(uaaId, uaaUserInfo); + cfUAANock.mockVerifyUser(uaaId, uaaUserInfo); }); it('returns a script tag', (done) => { diff --git a/test/api/requests/auth.test.js b/test/api/requests/auth.test.js index 985e7cae1..c510e6174 100644 --- a/test/api/requests/auth.test.js +++ b/test/api/requests/auth.test.js @@ -113,7 +113,7 @@ describe('Authentication requests', () => { const code = 'code'; cfUAANocks.mockUAAAuth(uaaUserProfile, code); - cfUAANocks.mockVerifyUserGroup(uaaId, uaaUserInfo); + cfUAANocks.mockVerifyUser(uaaId, uaaUserInfo); const cookie = await unauthenticatedSession({ oauthState, @@ -153,7 +153,7 @@ describe('Authentication requests', () => { const code = 'code'; cfUAANocks.mockUAAAuth(profile, code); - cfUAANocks.mockVerifyUserGroup(uaaId, userProfile); + cfUAANocks.mockVerifyUser(uaaId, userProfile); const cookie = await unauthenticatedSession({ oauthState, diff --git a/test/api/support/cfUAANock.js b/test/api/support/cfUAANock.js index 1658cc2aa..44f506c58 100644 --- a/test/api/support/cfUAANock.js +++ b/test/api/support/cfUAANock.js @@ -238,7 +238,7 @@ function mockUAAAuth(profile, code) { * @param {string} userId * @param {object} profile */ -function mockVerifyUserGroup(userId, profile) { +function mockVerifyUser(userId, profile) { const token = 'token'; mockFetchClientToken(token); mockFetchUser(userId, profile, token); @@ -291,6 +291,6 @@ module.exports = { mockInviteUser, mockInviteUserToUserGroup, mockRefreshToken, - mockVerifyUserGroup, + mockVerifyUser, mockServerErrorStatus, }; diff --git a/test/api/unit/services/organization.test.js b/test/api/unit/services/organization.test.js index d556bd217..ee03ceea5 100644 --- a/test/api/unit/services/organization.test.js +++ b/test/api/unit/services/organization.test.js @@ -252,9 +252,6 @@ describe('OrganizationService', () => { }), ]); - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(false); - const error = await OrganizationService.inviteUserToOrganization( currentUser, orgId, @@ -289,9 +286,6 @@ describe('OrganizationService', () => { }, }); - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(false); - const error = await OrganizationService.inviteUserToOrganization( currentUser, org.id, @@ -325,9 +319,6 @@ describe('OrganizationService', () => { }, }); - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(true); - const error = await OrganizationService.inviteUserToOrganization( currentUser, org.id, @@ -347,9 +338,6 @@ describe('OrganizationService', () => { it('adds them to the org with the provided role', async () => { const uaaEmail = 'foo@bar.com'; - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(false); - const targetUser = await createUserWithUAAIdentity(); const findOrCreateUAAUserStub = sinon.stub( OrganizationService, @@ -440,9 +428,6 @@ describe('OrganizationService', () => { it('adds them to the org with the provided role', async () => { const uaaEmail = 'foo@bar.com'; - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(true); - const targetUser = await createUserWithUAAIdentity(); const findOrCreateUAAUserStub = sinon.stub( OrganizationService, @@ -475,6 +460,7 @@ describe('OrganizationService', () => { org.id, userRole.id, uaaEmail, + true, ); sinon.assert.calledOnceWithMatch( @@ -536,22 +522,8 @@ describe('OrganizationService', () => { }); context('when the current user is not a Pages admin in UAA', () => { - it('throws an error', async () => { - const currentUser = await createUserWithUAAIdentity(); - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(false); - - const error = await OrganizationService.createOrganization( - {}, - currentUser, - '', - false, - '', - ).catch((e) => e); - - expect(error).to.be.an('Error'); - expect(error.message).to.contain('must be a Pages admin in UAA'); - }); + // this case can't occur the only controller which calls this method + // is in the admin API }); context('when the target user exists in Pages and UAA', () => { @@ -561,8 +533,6 @@ describe('OrganizationService', () => { const uaaEmail = 'foo@bar.com'; const targetUser = await createUserWithUAAIdentity(); - const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin'); - isUAAAdminStub.resolves(true); const findOrCreateUAAUserStub = sinon.stub( OrganizationService, 'findOrCreateUAAUser', diff --git a/test/api/unit/services/uaaStrategy.test.js b/test/api/unit/services/uaaStrategy.test.js index 940d285e8..afa62652b 100644 --- a/test/api/unit/services/uaaStrategy.test.js +++ b/test/api/unit/services/uaaStrategy.test.js @@ -58,7 +58,7 @@ describe('verifyUAAUser', () => { email: identity.email, }); - cfUAANock.mockVerifyUserGroup(uaaId, uaaUserResponse); + cfUAANock.mockVerifyUser(uaaId, uaaUserResponse); expect(identity.accessToken).to.be.null; expect(identity.refreshToken).to.be.null; @@ -100,7 +100,7 @@ describe('verifyUAAUser', () => { email: identity.email, }); - cfUAANock.mockVerifyUserGroup(uaaId, uaaUserResponse); + cfUAANock.mockVerifyUser(uaaId, uaaUserResponse); const { user: result, role } = await verifyUAAUser( accessToken, @@ -135,7 +135,7 @@ describe('verifyUAAUser', () => { email, }); - cfUAANock.mockVerifyUserGroup(uaaId, uaaUserResponse); + cfUAANock.mockVerifyUser(uaaId, uaaUserResponse); const { user: result, role } = await verifyUAAUser( accessToken, @@ -171,7 +171,7 @@ describe('verifyUAAUser', () => { email: identity.email, }); - cfUAANock.mockVerifyUserGroup(uaaId, uaaUserResponse); + cfUAANock.mockVerifyUser(uaaId, uaaUserResponse); await user.destroy(); @@ -185,4 +185,83 @@ describe('verifyUAAUser', () => { expect(role).to.be.null; return expect(result).to.be.null; }); + + it('should verify a user with cloud.gov origin and verified email', async () => { + const accessToken = 'a-token'; + const refreshToken = 'refresh-token'; + const user = await createUser(); + const identity = await createUAAIdentity({ userId: user.id }); + const { uaaId } = identity; + const userGroups = [ + { + display: 'pages.user', + }, + { + display: 'group.two', + }, + ]; + + const uaaUserResponse = uaaUser({ + uaaId, + groups: userGroups, + origin: 'cloud.gov', + verified: true, + }); + + const uaaUserProfile = uaaProfile({ + userId: uaaId, + email: identity.email, + }); + + cfUAANock.mockVerifyUser(uaaId, uaaUserResponse); + + const { user: result, role } = await verifyUAAUser( + accessToken, + refreshToken, + uaaUserProfile, + ); + + expect(role).to.equal('pages.user'); + return expect(result.dataValues).to.deep.equal(user.dataValues); + }); + + it(`should not verify a user with cloud.gov + origin and unverified verified email`, async () => { + const accessToken = 'a-token'; + const refreshToken = 'refresh-token'; + const user = await createUser(); + const identity = await createUAAIdentity({ userId: user.id }); + const { uaaId } = identity; + const userGroups = [ + { + display: 'pages.user', + }, + { + display: 'group.two', + }, + ]; + + const uaaUserResponse = uaaUser({ + uaaId, + groups: userGroups, + origin: 'cloud.gov', + verified: false, + }); + + const uaaUserProfile = uaaProfile({ + userId: uaaId, + email: identity.email, + }); + + cfUAANock.mockVerifyUser(uaaId, uaaUserResponse); + + const { user: result, role } = await verifyUAAUser( + accessToken, + refreshToken, + uaaUserProfile, + ); + + expect(role).to.equal(null); + return expect(result).to.equal(null); + }); }); diff --git a/test/api/unit/utils/uaaClient.test.js b/test/api/unit/utils/uaaClient.test.js index 3c9914c5c..580d9e6e6 100644 --- a/test/api/unit/utils/uaaClient.test.js +++ b/test/api/unit/utils/uaaClient.test.js @@ -4,7 +4,6 @@ const sinon = require('sinon'); const UAAClient = require('../../../../api/utils/uaaClient'); const cfUAANock = require('../../support/cfUAANock'); -const { uaaUser } = require('../../support/factory/uaa-identity'); describe('UAAClient', () => { let uaaClient; @@ -183,138 +182,6 @@ describe('UAAClient', () => { }); }); - describe('.verifyUserGroup()', () => { - it('should verify an active user', async () => { - const groupName = 'users-group'; - const allowedGroupNames = [groupName]; - const userGroups = [ - { - display: 'group-1', - }, - { - display: groupName, - }, - ]; - const userIdentity = uaaUser({ - groups: userGroups, - }); - - cfUAANock.mockVerifyUserGroup(userIdentity.id, userIdentity); - - const verified = await uaaClient.verifyUserGroup( - userIdentity.id, - allowedGroupNames, - ); - - return expect(verified).to.be.true; - }); - - it('should verify an active user when one of the group names matches', async () => { - const groupName = 'users-group'; - const allowedGroupNames = [groupName, 'another-group']; - const userGroups = [ - { - display: 'group-1', - }, - { - display: groupName, - }, - ]; - const userIdentity = uaaUser({ - groups: userGroups, - }); - - cfUAANock.mockVerifyUserGroup(userIdentity.id, userIdentity); - - const verified = await uaaClient.verifyUserGroup( - userIdentity.id, - allowedGroupNames, - ); - - return expect(verified).to.be.true; - }); - - it('should verify a user with cloud.gov origin and verified email', async () => { - const groupName = 'users-group'; - const allowedGroupNames = [groupName]; - const userGroups = [ - { - display: 'group-1', - }, - { - display: groupName, - }, - ]; - const userIdentity = uaaUser({ - groups: userGroups, - origin: 'cloud.gov', - verified: true, - }); - - cfUAANock.mockVerifyUserGroup(userIdentity.id, userIdentity); - - const userVerified = await uaaClient.verifyUserGroup( - userIdentity.id, - allowedGroupNames, - ); - - return expect(userVerified).to.be.true; - }); - - it(`should not verify a user with cloud.gov - origin and unverified verified email`, async () => { - const groupName = 'users-group'; - const allowedGroupNames = [groupName]; - const userGroups = [ - { - display: 'group-1', - }, - { - display: groupName, - }, - ]; - const userIdentity = uaaUser({ - groups: userGroups, - origin: 'cloud.gov', - verified: false, - }); - - cfUAANock.mockVerifyUserGroup(userIdentity.id, userIdentity); - - const userVerified = await uaaClient.verifyUserGroup( - userIdentity.id, - allowedGroupNames, - ); - - return expect(userVerified).to.be.false; - }); - - it('should not verify a user not in the specified group', async () => { - const groupName = 'user-not-a-member-group'; - const allowedGroupNames = [groupName]; - const userGroups = [ - { - display: 'group-1', - }, - { - display: 'group-2', - }, - ]; - const userIdentity = uaaUser({ - groups: userGroups, - }); - - cfUAANock.mockVerifyUserGroup(userIdentity.id, userIdentity); - - const userVerified = await uaaClient.verifyUserGroup( - userIdentity.id, - allowedGroupNames, - ); - - return expect(userVerified).to.be.false; - }); - }); - describe('.inviteUserToUserGroup()', () => { const clientToken = 'client-token';