chore: refactor parameter parsing in ListIdentities and disallow combining filters #4244
+122
−82
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,10 +119,7 @@ | |
| // Paginated Identity List Response | ||
| // | ||
| // swagger:response listIdentities | ||
| type _ struct { | ||
| migrationpagination.ResponseHeaderAnnotation | ||
|
|
||
| // List of identities | ||
|
|
@@ -133,11 +130,10 @@ | |
|
|
||
| // Paginated List Identity Parameters | ||
| // | ||
| // Note: Filters cannot be combined. | ||
| // | ||
| // swagger:parameters listIdentities | ||
| type _ struct { | ||
| migrationpagination.RequestParameters | ||
|
|
||
| // List of ids used to filter identities. | ||
|
|
@@ -183,11 +179,73 @@ | |
| crdbx.ConsistencyRequestParameters | ||
| } | ||
|
|
||
| func parseListIdentitiesParameters(r *http.Request) (params ListIdentityParameters, err error) { | ||
| query := r.URL.Query() | ||
| var requestedFilters int | ||
|
|
||
| params.Expand = ExpandDefault | ||
|
|
||
| if ids := query["ids"]; len(ids) > 0 { | ||
| requestedFilters++ | ||
| for _, v := range ids { | ||
| id, err := uuid.FromString(v) | ||
| if err != nil { | ||
| return params, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Invalid UUID value `%s` for parameter `ids`.", v)) | ||
| } | ||
| params.IdsFilter = append(params.IdsFilter, id) | ||
| } | ||
| } | ||
| if len(params.IdsFilter) > 500 { | ||
| return params, errors.WithStack(herodot.ErrBadRequest.WithReason("The number of ids to filter must not exceed 500.")) | ||
| } | ||
|
|
||
| if orgID := query.Get("organization_id"); orgID != "" { | ||
| requestedFilters++ | ||
| params.OrganizationID, err = uuid.FromString(orgID) | ||
| if err != nil { | ||
| return params, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Invalid UUID value `%s` for parameter `organization_id`.", orgID)) | ||
| } | ||
| } | ||
|
|
||
| if identifier := query.Get("credentials_identifier"); identifier != "" { | ||
| requestedFilters++ | ||
| params.Expand = ExpandEverything | ||
| params.CredentialsIdentifier = identifier | ||
| } | ||
|
|
||
| if identifier := query.Get("credentials_identifier_similar"); identifier != "" { | ||
| requestedFilters++ | ||
| params.Expand = ExpandEverything | ||
| params.CredentialsIdentifierSimilar = identifier | ||
| } | ||
|
|
||
| for _, v := range query["include_credential"] { | ||
| params.Expand = ExpandEverything | ||
| tc, ok := ParseCredentialsType(v) | ||
| if !ok { | ||
| return params, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Invalid value `%s` for parameter `include_credential`.", v)) | ||
| } | ||
| params.DeclassifyCredentials = append(params.DeclassifyCredentials, tc) | ||
| } | ||
|
|
||
| if requestedFilters > 1 { | ||
| return params, errors.WithStack(herodot.ErrBadRequest.WithReason("You cannot combine multiple filters in this API")) | ||
| } | ||
|
|
||
| params.KeySetPagination, params.PagePagination, err = x.ParseKeysetOrPagePagination(r) | ||
| if err != nil { | ||
| return params, err | ||
| } | ||
| params.ConsistencyLevel = crdbx.ConsistencyLevelFromRequest(r) | ||
|
|
||
| return params, nil | ||
| } | ||
|
|
||
| // swagger:route GET /admin/identities identity listIdentities | ||
| // | ||
| // # List Identities | ||
| // | ||
| // Lists all [identities](https://www.ory.sh/docs/kratos/concepts/identity-user-model) in the system. Note: filters cannot be combined. | ||
| // | ||
| // Produces: | ||
| // - application/json | ||
|
|
@@ -201,54 +259,7 @@ | |
| // 200: listIdentities | ||
| // default: errorGeneric | ||
| func (h *Handler) list(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { | ||
| params, err := parseListIdentitiesParameters(r) | ||
| if err != nil { | ||
| h.r.Writer().WriteError(w, r, err) | ||
| return | ||
|
|
@@ -271,7 +282,7 @@ | |
| } | ||
| u := *r.URL | ||
| pagepagination.PaginationHeader(w, &u, total, params.PagePagination.Page, params.PagePagination.ItemsPerPage) | ||
| } else if nextPage != nil { | ||
|
There was a problem hiding this comment.
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. |
||
| u := *r.URL | ||
| keysetpagination.Header(w, &u, nextPage) | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -369,21 +369,50 @@ func TestHandler(t *testing.T) { | |
| id := x.ParseUUID(res.Get("id").String()) | ||
| ids = append(ids, id) | ||
| } | ||
| require.Len(t, ids, identitiesAmount) | ||
| }) | ||
|
|
||
| t.Run("case=list few identities", func(t *testing.T) { | ||
| url := "/identities?ids=" + ids[0].String() + "&ids=" + ids[0].String() // duplicate ID is deduplicated in result | ||
| for i := 1; i < listAmount; i++ { | ||
| url += "&ids=" + ids[i].String() | ||
| } | ||
| res := get(t, adminTS, url, http.StatusOK) | ||
|
|
||
| identities := res.Array() | ||
| require.Len(t, identities, listAmount) | ||
| }) | ||
| }) | ||
|
|
||
| t.Run("case=list identities by ID is capped at 500", func(t *testing.T) { | ||
| url := "/identities?ids=" + x.NewUUID().String() | ||
| for i := 0; i < 501; i++ { | ||
| url += "&ids=" + x.NewUUID().String() | ||
| } | ||
| res := get(t, adminTS, url, http.StatusBadRequest) | ||
| assert.Contains(t, res.Get("error.reason").String(), "must not exceed 500") | ||
| }) | ||
|
|
||
| t.Run("case=list identities cannot combine filters", func(t *testing.T) { | ||
| filters := []string{ | ||
| "ids=" + x.NewUUID().String(), | ||
| "[email protected]", | ||
| "credentials_identifier_similar=bar.com", | ||
| "organization_id=" + x.NewUUID().String(), | ||
| } | ||
| for i := range filters { | ||
| for j := range filters { | ||
| if i == j { | ||
| continue // OK to use the same filter multiple times. Behavior varies by filter, though. | ||
| } | ||
|
|
||
| url := "/identities?" + filters[i] + "&" + filters[j] | ||
| res := get(t, adminTS, url, http.StatusBadRequest) | ||
| assert.Contains(t, res.Get("error.reason").String(), "cannot combine multiple filters") | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| t.Run("case=malformed ids should return an error", func(t *testing.T) { | ||
| res := get(t, adminTS, "/identities?ids=not-a-uuid", http.StatusBadRequest) | ||
| assert.Contains(t, res.Get("error.reason").String(), "Invalid UUID value `not-a-uuid` for parameter `ids`.", "%s", res.Raw) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 could consider to reduce the impact of this breaking change by just ignoring additional filters. If
ids=a&ids=b&organization_id=xpreviously returned onlya, it could now returna,bignoring 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.