require event_id parameter on personnel API calls

[srabraham]

The motivation is really just to lock down this endpoint. The only Rangers who need it are those with readIncident or writeIncident permissions. That's already a small subset of Rangers, even during event week. For the rest of the year, when almost no one has access to read or write incidents, the privacy benefit here becomes better still.

[github-actions]

⚠️ Optional matrix job Py:3.14.0-alpha.1 - ubuntu-latest failed ⚠️

• tox prefix: test
• exit status: 1

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 69.18%. Comparing base (a76c0b4) to head (8eb1e80).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/ims/auth/_provider.py	0.00%	2 Missing ⚠️
src/ims/application/_api.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
- Coverage   69.21%   69.18%   -0.04%     
==========================================
  Files         181      181              
  Lines        8892     8894       +2     
  Branches     1488     1488              
==========================================
- Hits         6155     6153       -2     
- Misses       2638     2641       +3     
- Partials       99      100       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wsanchez]

This puts the same endpoint in multiple places…
I wonder if it's cleaner to compute readPersonnel permission to include anyone with event access instead…

[wsanchez]

I wonder if it's cleaner to compute readPersonnel permission to include anyone with event access instead…

eg. in the two cases where we do authorizations |= Authorization.readIncidents , also do authorizations |= Authorization.readPersonnel

[srabraham]

I wonder if it's cleaner to compute readPersonnel permission to include anyone with event access instead…

eg. in the two cases where we do authorizations |= Authorization.readIncidents , also do authorizations |= Authorization.readPersonnel

That doesn't work on its own (that's the first thing I tried). A request for /ims/api/personnel won't include an event_id, so this whole block of code won't trigger when the auth check occurs

ranger-ims-server/src/ims/auth/_provider.py

Lines 406 to 424 in d99c05f

	if eventID is not None:
	if self._matchACL(
	user, frozenset(await self.store.writers(eventID))
	):
	authorizations |= Authorization.writeIncidents
	authorizations |= Authorization.readIncidents
	authorizations |= Authorization.writeIncidentReports
	else:
	if self._matchACL(
	user, frozenset(await self.store.readers(eventID))
	):
	authorizations |= Authorization.readIncidents
	if self._matchACL(
	user,
	frozenset(await self.store.reporters(eventID)),
	):
	authorizations |= Authorization.writeIncidentReports

This puts the same endpoint in multiple places…

That's true, but only because we're always just serving the current roster of Rangers. In an ideal system, a user would be able to go back to the 2022 event and add/remove Rangers from incidents using the list of personnel that was current in 2022. It seems that the personnel resource should be event-specific, even if we can't be bothered to make such a complex change.

[wsanchez]

That doesn't work on its own (that's the first thing I tried). A request for /ims/api/personnel won't include an event_id, so this whole block of code won't trigger when the auth check occurs

Right, yes.

This puts the same endpoint in multiple places…

That's true, but only because we're always just serving the current roster of Rangers. In an ideal system, a user would be able to go back to the 2022 event and add/remove Rangers from incidents using the list of personnel that was current in 2022. It seems that the personnel resource should be event-specific, even if we can't be bothered to make such a complex change.

I don't think of personnel as part of an event, but I see your point, though perhaps we can get that from the same resource with a query parameter, which would be one of several ways to filter. I could see making that parameter required and then limiting access as above, even if we return the same data for all events for the time being.

The more useful feature, I think, we could add is to ask the server to identify which personnel are currently on duty, so that we might put that at the top of the pop-up, followed by on-site staff, followed by others (or not? we'd have to trust that on-site is always correct).

I could be convinced otherwise, but grouping personnel under event suggests there's never a use case outside of the event context. That may be true… have to mull that over.

[srabraham]

That doesn't work on its own (that's the first thing I tried). A request for /ims/api/personnel won't include an event_id, so this whole block of code won't trigger when the auth check occurs

Right, yes.

This puts the same endpoint in multiple places…

That's true, but only because we're always just serving the current roster of Rangers. In an ideal system, a user would be able to go back to the 2022 event and add/remove Rangers from incidents using the list of personnel that was current in 2022. It seems that the personnel resource should be event-specific, even if we can't be bothered to make such a complex change.

I don't think of personnel as part of an event, but I see your point, though perhaps we can get that from the same resource with a query parameter, which would be one of several ways to filter. I could see making that parameter required and then limiting access as above, even if we return the same data for all events for the time being.

The more useful feature, I think, we could add is to ask the server to identify which personnel are currently on duty, so that we might put that at the top of the pop-up, followed by on-site staff, followed by others (or not? we'd have to trust that on-site is always correct).

I could be convinced otherwise, but grouping personnel under event suggests there's never a use case outside of the event context. That may be true… have to mull that over.

Going with a query parameter sounds good to me. I'll have a look at the on-duty idea too.

I might see about adding a filtering query parameter to the incidents endpoint too, now that you mention it, which would allow excluding all the generated incident entries (which are really noisy and pure overhead for serving the incidents table)

[srabraham]

That doesn't work on its own (that's the first thing I tried). A request for /ims/api/personnel won't include an event_id, so this whole block of code won't trigger when the auth check occurs

Right, yes.

This puts the same endpoint in multiple places…

That's true, but only because we're always just serving the current roster of Rangers. In an ideal system, a user would be able to go back to the 2022 event and add/remove Rangers from incidents using the list of personnel that was current in 2022. It seems that the personnel resource should be event-specific, even if we can't be bothered to make such a complex change.

I don't think of personnel as part of an event, but I see your point, though perhaps we can get that from the same resource with a query parameter, which would be one of several ways to filter. I could see making that parameter required and then limiting access as above, even if we return the same data for all events for the time being.
The more useful feature, I think, we could add is to ask the server to identify which personnel are currently on duty, so that we might put that at the top of the pop-up, followed by on-site staff, followed by others (or not? we'd have to trust that on-site is always correct).
I could be convinced otherwise, but grouping personnel under event suggests there's never a use case outside of the event context. That may be true… have to mull that over.

Going with a query parameter sounds good to me. I'll have a look at the on-duty idea too.

I might see about adding a filtering query parameter to the incidents endpoint too, now that you mention it, which would allow excluding all the generated incident entries (which are really noise and pure overhead for serving the incidents table)

switched to query param