chore: refactor parameter parsing in ListIdentities and disallow combining filters #4244
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4244 +/- ##
==========================================
- Coverage 79.10% 78.58% -0.53%
==========================================
Files 381 380 -1
Lines 28012 27275 -737
==========================================
- Hits 22159 21434 -725
- Misses 4209 4221 +12
+ Partials 1644 1620 -24 ☔ View full report in Codecov by Sentry. |
| if requestedFilters > 1 { | ||
| return params, errors.WithStack(herodot.ErrBadRequest.WithReason("You cannot combine multiple filters in this API")) | ||
| } |
There was a problem hiding this comment.
We could consider to reduce the impact of this breaking change by just ignoring additional filters. If ids=a&ids=b&organization_id=x previously returned only a, it could now return a,b ignoring the second filter. The result should always be a super-set of the previous version, therefore significantly limiting the impact.
On the other side, someone might rely on the behavior and this could in the worst case lead to security issues if the assumption is that all returned identities are part of the given organization.
WDYT?
There was a problem hiding this comment.
I've checked our logs and could not find anyone combining filters, so I think we're good.
Changing semantics silently is a recipe for disaster.
| @@ -271,7 +282,7 @@ func (h *Handler) list(w http.ResponseWriter, r *http.Request, _ httprouter.Para | |||
| } | |||
| u := *r.URL | |||
| pagepagination.PaginationHeader(w, &u, total, params.PagePagination.Page, params.PagePagination.ItemsPerPage) | |||
| } else { | |||
| } else if nextPage != nil { | |||
There was a problem hiding this comment.
nextPage should never be nil, and the header is also relevant in case of the last page.
There was a problem hiding this comment.
When using the ID filter, pagination doesn't make sense. In https://github.com/ory-corp/cloud/pull/7471, I'm returning a nil paginator. Mostly because I don't know how to construct a "no-next-page" paginator and didn't want to change ory/x.
The code for ListIdentities has gotten out of hand. The string-based query building is not sustainable. @aeneasr and I feel we have no choice for now but to disable combining filters (like
GET /admin/identities?organization_id=abc&credential_identifier=xyz).This will allow us to build a simple switch into the ListIdentities handler to go to optimized implementations for each filter in the future and in cloud.