Allow admins to fetch misuse stats summaries for a given user #506

chrisjpurdy · 2022-12-16T18:43:26Z

And allow admins to reset misuse event counters even if the event isn't yet "misused"

Pull Request Check List

Security - Access Control - Check authorisation on every new endpoint
Peer-Reviewed

And allow admins to reset misuse event counters even if the event isn't yet "misused"

codecov · 2022-12-16T18:49:18Z

Codecov Report

Base: 25.75% // Head: 26.80% // Increases project coverage by +1.04% 🎉

Coverage data is based on head (f2a3c2e) compared to base (2960f6f).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head f2a3c2e differs from pull request most recent head c5ebdc1. Consider uploading reports for the commit c5ebdc1 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #506      +/-   ##
==========================================
+ Coverage   25.75%   26.80%   +1.04%     
==========================================
  Files         481      482       +1     
  Lines       21932    21973      +41     
  Branches     2723     2722       -1     
==========================================
+ Hits         5649     5889     +240     
+ Misses      15791    15564     -227     
- Partials      492      520      +28

Impacted Files	Coverage Δ
...uk/ac/cam/cl/dtg/isaac/dto/MisuseStatisticDTO.java	`0.00% <0.00%> (ø)`
...n/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java	`0.00% <0.00%> (ø)`
.../dtg/segue/api/monitors/InMemoryMisuseMonitor.java	`54.76% <0.00%> (-16.01%)`	⬇️
...dtg/segue/api/managers/UserAssociationManager.java	`50.00% <0.00%> (-4.55%)`	⬇️
.../java/uk/ac/cam/cl/dtg/isaac/api/EventsFacade.java	`12.32% <0.00%> (-0.02%)`	⬇️
...n/java/uk/ac/cam/cl/dtg/segue/api/UsersFacade.java	`0.00% <0.00%> (ø)`
...ava/uk/ac/cam/cl/dtg/segue/api/QuestionFacade.java	`13.13% <0.00%> (ø)`
...va/uk/ac/cam/cl/dtg/isaac/api/IsaacController.java	`0.00% <0.00%> (ø)`
...cl/dtg/isaac/api/managers/EventBookingManager.java	`46.38% <0.00%> (ø)`
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java

jsharkey13 · 2023-01-06T11:07:17Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java

    @Produces(MediaType.APPLICATION_JSON)
    @Operation(summary = "Reset a misuse monitor counter to zero.")
    public Response resetMisuseMonitor(@Context final HttpServletRequest request,
                                       @PathParam("event_label") final String eventLabel,
-                                       final String agentIdentifier) {
+                                       @PathParam("agent_identifier") final String agentIdentifier) {


Some of these agent identifiers are not URL-safe values and probably shouldn't be path parameters, since they could reasonably contain slashes. I suppose they could be query parameters, and URL-encoded.

This feels no less hacky than the previous plain-text-body-as-identifier. Really a Map<String, String> detailsObject as the body is the right way to do it, and then detailsObject.get("agentIdentifier"). Then it can @Consumes(MediaType.APPLICATION_JSON) like it ought to!

I wasn't sure what the agent identifier was, so assumed user id - I've taken a look at everywhere that uses notifyEvent and can see that there is a fair spread of things now so yeah I'll fix that.

jsharkey13 · 2023-01-06T11:07:51Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java

-                        eventLabel, agentIdentifier));
-                return Response.ok(ImmutableMap.of("status", "Reset successfully!")).build();
-            } else {
-                return Response.ok(ImmutableMap.of("status", "Nothing to reset.")).build();


Knowing whether it reset something is really very useful. Was this removed for any reason?

This was removed because I removed the misuseMonitor.hasMisused(...) condition, since it makes sense for admins to be able to reset the misuse count at any point, not just after is becomes misused. Because of this, the only response would be "Reset successfully!" which IMO is just as good as a 200.

But I need to know I have actually reset the thing I was intending to reset? If I can't find it in the new UI, I need to know that I have got the right (encoding of the) agent identifier and without this there's no way an admin can tell any longer!

I see that it's nice to be able to reset it before it crosses the threshold, though. Maybe it's fine like this. I certainly don't want any more time spent on this.

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java

jsharkey13 · 2023-01-06T13:57:17Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/monitors/InMemoryMisuseMonitor.java

-
+
+    @Override
+    public List<MisuseStatisticDTO> getMisuseStatistics(final String agentIdentifier) {


See my comment above, I am unsure this is that useful given how varied the agent identifiers are.

… type

By returning a sensible placeholder. This previously crashed the client-side admin tools page

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java

+            return SegueErrorResponse.getNotLoggedInResponse();
+        }


jsharkey13 · 2023-01-11T09:59:30Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/InfoFacade.java

@@ -90,7 +91,7 @@ public InfoFacade(final PropertiesLoader properties, final GitContentManager con
    @Operation(summary = "Get the currently running API build version.")
    public final Response getSegueAppVersion() {
        ImmutableMap<String, String> result = new ImmutableMap.Builder<String, String>().put("segueVersion",
-                SegueGuiceConfigurationModule.getSegueVersion()).build();
+                Objects.requireNonNullElse(SegueGuiceConfigurationModule.getSegueVersion(), "unknown")).build();


What is this doing here? It's not at all related to the PR?

On adding the misuse UI to admin tools, it kept crashing because this endpoint would always return 500. I decided to add this as it seems sensible and allowed me to actually use admin tools on a local dev build. I understand that it is a bit out of place, but I don't think it should be removed?

jsharkey13 · 2023-01-11T10:01:27Z

src/main/java/uk/ac/cam/cl/dtg/segue/api/monitors/InMemoryMisuseMonitor.java

-
+
+    @Override
+    public Map<String, List<MisuseStatisticDTO>> getMisuseStatistics(final long n) {


I think this should really take an @Nullable final Long n and here is the place for the default value to live. But it's not worth changing now.

Allow admins to fetch misuse stats summaries for a given user

c7f3f71

And allow admins to reset misuse event counters even if the event isn't yet "misused"

github-advanced-security bot found potential problems Dec 16, 2022

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java Fixed Show resolved Hide resolved

jsharkey13 reviewed Jan 6, 2023

View reviewed changes

chrisjpurdy added 2 commits January 9, 2023 15:54

Rewrite so misuse stats summary includes top N misuses for each event…

f2a3c2e

… type

Prevent 500 errors if segue version isn't defined

0751958

By returning a sensible placeholder. This previously crashed the client-side admin tools page

chrisjpurdy requested a review from jsharkey13 January 9, 2023 15:58

Refactor magic constant number

c5ebdc1

github-advanced-security bot found potential problems Jan 9, 2023

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/segue/api/AdminFacade.java

Comment on lines +1185 to +1186

return SegueErrorResponse.getNotLoggedInResponse();

}

Check failure

Code scanning / CodeQL

Log Injection

This log entry depends on a [user-provided value](1).

jsharkey13 reviewed Jan 11, 2023

View reviewed changes

chrisjpurdy merged commit 233edc0 into master Jan 11, 2023

chrisjpurdy deleted the feature/user-misuse-statistics branch January 11, 2023 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow admins to fetch misuse stats summaries for a given user #506

Allow admins to fetch misuse stats summaries for a given user #506

Uh oh!

chrisjpurdy commented Dec 16, 2022

Uh oh!

codecov bot commented Dec 16, 2022 •

edited

Loading

Uh oh!

Uh oh!

jsharkey13 Jan 6, 2023

Uh oh!

chrisjpurdy Jan 9, 2023

Uh oh!

jsharkey13 Jan 6, 2023

Uh oh!

chrisjpurdy Jan 9, 2023

Uh oh!

jsharkey13 Jan 11, 2023

Uh oh!

jsharkey13 Jan 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsharkey13 Jan 6, 2023

Uh oh!

Check failure

jsharkey13 Jan 11, 2023

Uh oh!

chrisjpurdy Jan 11, 2023

Uh oh!

jsharkey13 Jan 11, 2023

Uh oh!

Uh oh!



		@Override
		public List<MisuseStatisticDTO> getMisuseStatistics(final String agentIdentifier) {



		@Override
		public Map<String, List<MisuseStatisticDTO>> getMisuseStatistics(final long n) {

Allow admins to fetch misuse stats summaries for a given user #506

Allow admins to fetch misuse stats summaries for a given user #506

Uh oh!

Conversation

chrisjpurdy commented Dec 16, 2022

Uh oh!

codecov bot commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check failure

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Dec 16, 2022 •

edited

Loading