Skip to content

Commit

Permalink
Merge pull request #4711 from cloud-gov/fix-admin-login-4656
Browse files Browse the repository at this point in the history 
update role authorization checks, auth logging, and code updates
  • Loading branch information
@drewbo
drewbo authored Feb 6, 2025
2 parents 94e4dec + b5ce70d commit 5b71243
Show file tree
Hide file tree
Showing 13 changed files with 161 additions and 284 deletions.
1 change: 1 addition & 0 deletions api/admin/controllers/user.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ module.exports = wrapHandlers({
toInt(organizationId),
toInt(roleId),
uaaEmail,
true,
);
EventCreator.audit(Event.labels.ADMIN_ACTION, req.user, 'User Invited', {
organizationId,
Expand Down
20 changes: 4 additions & 16 deletions api/admin/passport.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
}

Expand Down
1 change: 1 addition & 0 deletions api/controllers/organization.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 19 additions & 45 deletions api/services/organization/Organization.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<UserType | null>}
Expand Down Expand Up @@ -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<UAAClient.UAAUserAttributes>}
*/
async inviteUserToOrganization(currentUser, organizationId, roleId, targetUserEmail) {
async inviteUserToOrganization(
currentUser,
organizationId,
roleId,
targetUserEmail,
isAdmin = false,
) {
const currentUserUAAIdentity = await currentUser.getUAAIdentity();

if (!currentUserUAAIdentity) {
Expand All @@ -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(
Expand Down Expand Up @@ -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];
},
Expand Down
6 changes: 1 addition & 5 deletions api/services/passport.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
24 changes: 17 additions & 7 deletions api/services/uaaStrategy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -49,7 +55,7 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) {
},
);

return null;
return { user: null, role: null };
}

const identity = await UAAIdentity.findOne({
Expand All @@ -70,7 +76,7 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) {
profile,
},
);
return null;
return { user: null, role: null };
}

if (!identity.User) {
Expand All @@ -83,7 +89,7 @@ async function verifyUAAUser(accessToken, refreshToken, profile, uaaGroups) {
identity,
},
);
return null;
return { user: null, role: null };
}

await identity.update({
Expand All @@ -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 = {
Expand Down
17 changes: 0 additions & 17 deletions api/utils/uaaClient.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 2 additions & 6 deletions test/api/admin/requests/admin-auth.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand Down Expand Up @@ -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) => {
Expand Down
4 changes: 2 additions & 2 deletions test/api/requests/auth.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions test/api/support/cfUAANock.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -291,6 +291,6 @@ module.exports = {
mockInviteUser,
mockInviteUserToUserGroup,
mockRefreshToken,
mockVerifyUserGroup,
mockVerifyUser,
mockServerErrorStatus,
};
36 changes: 3 additions & 33 deletions test/api/unit/services/organization.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,6 @@ describe('OrganizationService', () => {
}),
]);

const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin');
isUAAAdminStub.resolves(false);

const error = await OrganizationService.inviteUserToOrganization(
currentUser,
orgId,
Expand Down Expand Up @@ -289,9 +286,6 @@ describe('OrganizationService', () => {
},
});

const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin');
isUAAAdminStub.resolves(false);

const error = await OrganizationService.inviteUserToOrganization(
currentUser,
org.id,
Expand Down Expand Up @@ -325,9 +319,6 @@ describe('OrganizationService', () => {
},
});

const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin');
isUAAAdminStub.resolves(true);

const error = await OrganizationService.inviteUserToOrganization(
currentUser,
org.id,
Expand All @@ -347,9 +338,6 @@ describe('OrganizationService', () => {
it('adds them to the org with the provided role', async () => {
const uaaEmail = '[email protected]';

const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin');
isUAAAdminStub.resolves(false);

const targetUser = await createUserWithUAAIdentity();
const findOrCreateUAAUserStub = sinon.stub(
OrganizationService,
Expand Down Expand Up @@ -440,9 +428,6 @@ describe('OrganizationService', () => {
it('adds them to the org with the provided role', async () => {
const uaaEmail = '[email protected]';

const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin');
isUAAAdminStub.resolves(true);

const targetUser = await createUserWithUAAIdentity();
const findOrCreateUAAUserStub = sinon.stub(
OrganizationService,
Expand Down Expand Up @@ -475,6 +460,7 @@ describe('OrganizationService', () => {
org.id,
userRole.id,
uaaEmail,
true,
);

sinon.assert.calledOnceWithMatch(
Expand Down Expand Up @@ -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', () => {
Expand All @@ -561,8 +533,6 @@ describe('OrganizationService', () => {
const uaaEmail = '[email protected]';

const targetUser = await createUserWithUAAIdentity();
const isUAAAdminStub = sinon.stub(OrganizationService, 'isUAAAdmin');
isUAAAdminStub.resolves(true);
const findOrCreateUAAUserStub = sinon.stub(
OrganizationService,
'findOrCreateUAAUser',
Expand Down
Loading

0 comments on commit 5b71243

Please sign in to comment.