Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pentest #1566

Merged
merged 89 commits into from
Dec 13, 2024
Merged

Pentest #1566

merged 89 commits into from
Dec 13, 2024

Conversation

anyoussefinia
Copy link
Collaborator

@anyoussefinia anyoussefinia commented Dec 13, 2024

Story: https://jiraent.cms.gov/browse/OY2-31468
https://jiraent.cms.gov/browse/OY2-31474
Endpoint: https://d108315ije1eu7.cloudfront.net/

Details

added a batch function that modifies the idToken JWT with a "custom:user_roles" attribute with the roles retrieved for each user from dynamo.

Added route protection for the /usermanagement route which checks the jwt for proper role access

Changes

-also added the trigger function for potential use if the intermittent functionality problem is fixed.

Test Plan

Just a smoke test to make sure nothing is broken, The pen testers will need to verify that the usermanagent screen cannot be accessed by modifying the response to UserDataApi.userProfile by setting the admin role status from "revoked" to "active"

Copy link

Endpoint URL - https://d108315ije1eu7.cloudfront.net

return false;
}

const allowedRoles = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for turning this into a whitelist of allowed roles!

@@ -106,6 +106,7 @@ const UserManagement = () => {
console.log("Error while fetching user list.", error);
setAlertCode(RESPONSE_CODE[error.message]);
});
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please discard this change as no functional changes were made to this file

@@ -161,7 +161,7 @@ const UserPage = () => {
const { userProfile, setUserInfo, updatePhoneNumber, userRole, userStatus } =
useAppContext();
const location = useLocation();
const { userId } = useParams() ?? {};
let { userId } = useParams() ?? {};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why this change was made........might even be flagged by lint if setting with let but is never overridden.....but also probably userId is something we dont want to let get overwritten?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right I did this when I was considering how to encrypt the email/userId before Andy fixed it

@anyoussefinia anyoussefinia merged commit 6696a62 into develop Dec 13, 2024
17 of 37 checks passed
@anyoussefinia anyoussefinia deleted the pentest branch December 13, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants