Skip to content
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

Expose limit for admin/users/list API #4861

Closed
wants to merge 1 commit into from

Conversation

JaredReisinger
Copy link
Contributor

Expose querystring access to the "limit" parameter so that we're not restricted to only the first 100 users. (See also the discussion at https://meta.discourse.org/t/using-the-api-for-user-management-the-100-user-limit/37562.)

Added tests to ensure that the parameter can't be used in a SQL injection attack, and that if anything is amiss it falls back to the previously-existing behavior.

Do note that this PR is offered in advance of full agreement from folks that this is even a reasonable thing to do... but it gives a starting point for the discussion.

Expose querystring access to the "limit" parameter so that we're not
restricted to only the first 100 users.  (See also the discussion at
https://meta.discourse.org/t/using-the-api-for-user-management-the-100-user-limit/37562.)

Added tests to ensure that the parameter can't be used in a SQL
injection attack, and that if anything is amiss it falls back to the
previously-existing behavior.
@discoursebot
Copy link

You've signed the CLA, JaredReisinger. Thank you! This pull request is ready for review.

@JaredReisinger
Copy link
Contributor Author

Closing this as per @oblakeerickson's comments about the API design. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants