-
Notifications
You must be signed in to change notification settings - Fork 259
Feature/triage malicious events #3204
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
Conversation
public String getStatus() { | ||
return status; | ||
} | ||
|
||
public void setStatus(String status) { | ||
this.status = status; | ||
} |
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.
just check once existing data should not fail...
When I added the new metadata field, there were issues in loading threat dashboard because old data did not have this field.
import com.akto.dto.type.URLMethods; | ||
import com.akto.proto.generated.threat_detection.service.dashboard_service.v1.FetchAlertFiltersResponse; | ||
import com.akto.proto.generated.threat_detection.service.dashboard_service.v1.ListMaliciousRequestsResponse; | ||
import com.akto.proto.generated.threat_detection.service.dashboard_service.v1.UpdateMaliciousEventStatusRequest; |
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.
Next time we can give this to intern. We need to clean up this mess...
- Use DAO, DTO like dashboard.
- See how can we utilise grpc more effectively
Map<String, Object> request = null; | ||
try { | ||
request = new com.fasterxml.jackson.databind.ObjectMapper().readValue( | ||
reqBody.asString(), Map.class); | ||
} catch (Exception e) { |
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.
Why the difference ? 2 apis are proto based these are json based ?
// Handle status filter | ||
if (filter.hasStatusFilter()) { | ||
String statusFilter = filter.getStatusFilter(); | ||
if ("UNDER_REVIEW".equals(statusFilter)) { |
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.
Use Constants instead of strings "UNDER_REVEIW"? Some where in libs maybe so it can be used everywhere ?
|
||
MaliciousEventModel.Status eventStatus = MaliciousEventModel.Status.valueOf(status.toUpperCase()); | ||
|
||
Bson filter = Filters.eq("_id", eventId); | ||
Bson update = Updates.set("status", eventStatus.toString()); | ||
|
||
boolean updateSuccess = coll.updateOne(filter, update).getModifiedCount() > 0; | ||
return updateSuccess; | ||
} catch (Exception e) { | ||
logger.error("Error updating malicious event status", e); | ||
return false; | ||
} | ||
} | ||
|
||
public int bulkUpdateMaliciousEventStatus(String accountId, List<String> eventIds, String status) { | ||
int updatedCount = 0; | ||
try { | ||
logger.info("Bulk updating events for account " + accountId + " with IDs: " + eventIds + " to status: " + status); | ||
|
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.
Use findAndModify instead to keep the query atomic.
Create a single API for these in case of single event Updation we cam simply send one eventId in the list
} | ||
|
||
// Handle status filter (for current tab) | ||
String statusFilter = (String) filter.get("statusFilter"); |
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.
Repeated code, create function out of this.
public int bulkDeleteMaliciousEvents(String accountId, List<String> eventIds) { | ||
try { | ||
MongoCollection<MaliciousEventModel> coll = | ||
this.mongoClient | ||
.getDatabase(accountId) | ||
.getCollection( | ||
MongoDBCollection.ThreatDetection.MALICIOUS_EVENTS, MaliciousEventModel.class); | ||
|
||
Document query = new Document("_id", new Document("$in", eventIds)); | ||
long deletedCount = coll.deleteMany(query).getDeletedCount(); | ||
|
||
logger.info("Deleted " + deletedCount + " malicious events"); | ||
return (int) deletedCount; | ||
} catch (Exception e) { | ||
logger.error("Error bulk deleting malicious events", e); | ||
return 0; | ||
} | ||
} | ||
|
||
public int bulkDeleteFilteredEvents(String accountId, Map<String, Object> filter) { | ||
try { | ||
MongoCollection<MaliciousEventModel> coll = | ||
this.mongoClient | ||
.getDatabase(accountId) |
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.
These two APIs bulkDeleteMaliciousEvents, bulkDeleteFilteredEvents are also unnecessary, since most of the code is repeated.
Just send an key action: UPDATE/DELETE from the FE and use that.
More APIs, more code maintainence, more bugs. 😛
try { | ||
const response = await threatDetectionApi.updateMaliciousEventStatus(eventId, newStatus); | ||
if (response?.updateSuccess) { | ||
console.log(`Event successfully marked as ${newStatus.toLowerCase()}`); |
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.
console log remove
const statusText = newStatus === 'UNDER_REVIEW' ? 'marked for review' : | ||
newStatus === 'IGNORED' ? 'ignored' : 'reactivated'; | ||
func.setToast(true, false, `Event ${statusText} successfully`); |
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.
constants for strings
console.log(`Selected IDs for ${label.ing}:`, selectedIds); | ||
|
||
if (!selectedIds || selectedIds.length === 0) { | ||
console.error(`No IDs selected for ${label.ing}`); |
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.
remove consoles from everywhere
return url + ":" + filterId; | ||
} | ||
|
||
public static boolean isIgnoredInCache(String accountId, String url, String filterId) { |
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.
Edge case: can handle maybe ? On deletion or status change from ignored to active remove from cache as well ?
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.
cache will reload in 5 mins anyways, so decided to not put any additional logic since we could wait for that duration. Can add if needed
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.
Please check.
We should have at most 2 APIs.
bulkUpdateEvents -> eventIds !=null then do the eventIds based. Else do filterBased.
bulkDeleteEvents -> Same
Move the filterHandling common in update and delete to one function
Merge the bulkUpdateEvents and bulkUpdateFilteredEvents into one bulkUpdateEvents
bulkDeleteEvents.
// Get all matching events for cache management | ||
List<MaliciousEventModel> eventsToUpdate = new ArrayList<>(); | ||
try (MongoCursor<MaliciousEventModel> cursor = coll.find(query).cursor()) { |
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.
we not doing anything with eventsToUpdate
then why fetch them ??
if (this.eventId != null && !this.eventId.isEmpty()) { | ||
// Single event update | ||
requestBuilder.setEventId(this.eventId); | ||
} else if (this.eventIds != null && !this.eventIds.isEmpty()) { | ||
// Bulk update by IDs | ||
requestBuilder.addAllEventIds(this.eventIds); |
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.
Nitpick: can ignore.
Why not just use the eventIds ? In case of one id the list size will be 1
MongoCollection<MaliciousEventModel> coll = | ||
this.mongoClient | ||
.getDatabase(accountId) | ||
.getCollection( |
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.
re-use in functions. most code is same.
|
||
if (this.eventId != null && !this.eventId.isEmpty()) { | ||
// Single event delete | ||
requestBuilder.addEventIds(this.eventId); | ||
} else if (this.eventIds != null && !this.eventIds.isEmpty()) { | ||
// Bulk delete by IDs | ||
requestBuilder.addAllEventIds(this.eventIds); | ||
} else { | ||
// Delete by filter | ||
Filter.Builder filterBuilder = buildFilterFromParams(); | ||
requestBuilder.setFilter(filterBuilder); |
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.
move common parts to function ?
} | ||
|
||
public String getStatus() { | ||
return status; | ||
} | ||
|
||
public void setStatus(String status) { | ||
this.status = status; | ||
} | ||
|
||
public boolean isUpdateSuccess() { |
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.
lombok getter seter.
No description provided.