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

[PM-12777] Fixed Issue #4034, API endpoint now handles optional parameters #4812

Merged
merged 11 commits into from
Oct 17, 2024

Conversation

BensonB12
Copy link
Contributor

🎟️ Tracking

#4034

📔 Objective

The API endpoint: 'https://scim.bitwarden.com/v2/{Tenant-ID}/Users' now accepts null values for 'count' and 'startingIndex'. It now has a default value of count 50 and startingIndex 1. We made an integration test (based on a previously made one) that now tests that function.

User: https:/dougedey-sf said that this new function is the wanted behavior. We did not double check that, but we did notice that https:/cbbit commented without contradicting the issue.

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@BensonB12 BensonB12 requested a review from a team as a code owner September 27, 2024 20:36
@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-12777

@bitwarden-bot bitwarden-bot changed the title Fixed Issue #4034, API endpoint now handles optional parameters [PM-12777] Fixed Issue #4034, API endpoint now handles optional parameters Sep 27, 2024
@jrmccannon jrmccannon self-assigned this Oct 3, 2024
Copy link
Contributor

@jrmccannon jrmccannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thank you for contributing to Bitwarden. This is a good initial pass and thank you for including a test. To improve on this, you could maybe add a new request object that uses some data annotations that limits the values that could be passed in to that endpoint. You can then provide that object as a parameter in UserController#Get with the [FromQuery] attribute.

That class could look something like

public class GetUsersQueryParam
{
        public string Filter { get; init; } = string.Empty;

        [Range(1, int.MaxValue)]
        public int Count { get; init; } = 50;

        [Range(1, int.MaxValue)]
        public int StartIndex { get; init; } = 1;
}

This would provide callers with helpful error messages in the event they accidentally provide invalid values. Then you could refactor the method signature and syntax of GetUsersListQuery#GetUserListAsync() to takeints rather than int?s.

Thanks again!

bitwarden_license/src/Scim/Users/GetUsersListQuery.cs Outdated Show resolved Hide resolved
BensonB12 and others added 3 commits October 7, 2024 14:29
Co-authored-by: Ahmad Mustafa Jebran <[email protected]>
Co-authored-by: Luris Solis <[email protected]>
Co-authored-by: Ahmad Mustafa Jebran <[email protected]>
Co-authored-by: Luris Solis <[email protected]>
Copy link
Contributor

github-actions bot commented Oct 9, 2024

Logo
Checkmarx One – Scan Summary & Details94a429b9-ace1-4c1a-9736-7fe1eb1cfbe0

No New Or Fixed Issues Found

@jrmccannon jrmccannon merged commit da04218 into bitwarden:main Oct 17, 2024
85 of 104 checks passed
@jrmccannon
Copy link
Contributor

Sorry for the delay. This has passed through QA and will be included in the 2024.10.1 release. Thank you again for your contribution.

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

Successfully merging this pull request may close these issues.

SCIM /Users listing does not return any users unless the startIndex and count is specified
3 participants