-
Notifications
You must be signed in to change notification settings - Fork 10
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
[RW-13914][RW-13916][risk=no] Handle vwb egress alert (1st part) #8990
Conversation
api/db/changelog/db.changelog-246-add-vwb-egress-event-columns.xml
Outdated
Show resolved
Hide resolved
"An egress event happened for workspace: %s, but we're unable to find a user %s.", | ||
egressEvent.getVwbWorkspaceId(), egressEvent.getUserEmail())); | ||
} | ||
this.egressEventAuditor.fireVwbEgressEvent(egressEvent, egressUser.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 111 implies that egressUser can be empty, line 117 makes a call to egressUser.get which might be empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this whole part was copied from https://github.com/all-of-us/workbench/blob/fcac632a7d5287b30a159ee71a810c46f2974d79/api/src/main/java/org/pmiops/workbench/exfiltration/EgressEventServiceImpl.java#L84L100, somehow I grouped both into 1 because user should not be empty?
@@ -280,6 +290,18 @@ protected void disableUser(DbUser user) { | |||
stopUserRuntimesAndApps(user.getUsername()); | |||
} | |||
|
|||
protected void disableUserVwbDataAccess(DbUser user) { | |||
userService.updateUserWithRetries( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just call disableUser here instead of duplicating the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will be adding logic in my next PR here. So I probably will leave it as it and come back to modify soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the static code analysis issue, everything else looks good to me. Thank you!
Thanks. That codify is an exising issues comes from parent class, don't know how to fix it without a large refactor |
Initial batch of egress processing. There would be few followup for email and jira.
This is incomplete work, but for now, everything is protected by feature flag
PR checklist
[risk=no|low|moderate|severe]
in the PR title as outlined in CONTRIBUTING.md.