-
Notifications
You must be signed in to change notification settings - Fork 611
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
feat: provision from SSO preferred_username #5347
Merged
rubenfiszel
merged 2 commits into
main
from
hugo/win-773-provision-username-from-ads-username-when-possible
Feb 21, 2025
Merged
feat: provision from SSO preferred_username #5347
rubenfiszel
merged 2 commits into
main
from
hugo/win-773-provision-username-from-ads-username-when-possible
Feb 21, 2025
+2
−2
Conversation
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
Deploying windmill with
|
Latest commit: |
c888e66
|
Status: | ✅ Deploy successful! |
Preview URL: | https://1775e897.windmill.pages.dev |
Branch Preview URL: | https://hugo-win-773-provision-usern.windmill.pages.dev |
Deploying windmill with
|
Latest commit: |
90c383f
|
Status: | ✅ Deploy successful! |
Preview URL: | https://345819fc.windmill.pages.dev |
Branch Preview URL: | https://hugo-win-773-provision-usern.windmill.pages.dev |
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.
👍 Looks good to me! Reviewed everything up to 90c383f in 1 minute and 46 seconds
More details
- Looked at
21
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. backend/windmill-api/src/users.rs:2047
- Draft comment:
New fieldpreferred_username
added. Ensure its semantics and naming are consistent with other fields. Consider ifdisplayName
should be renamed todisplay_name
for consistency. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. backend/windmill-api/src/users.rs:2044
- Draft comment:
New field 'preferred_username' is added for SSO provisioning. Ensure that this value is validated (e.g. using the VALID_USERNAME regex) and normalized (e.g. lowercased) if needed. Also, consider consistent naming (using snake_case) or apply serde renaming for fields like 'displayName'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Let me analyze this step by step:
- The comment makes multiple suggestions but they are speculative - it says "if needed" and doesn't demonstrate why validation/normalization would be required
- The struct is used for OAuth login info, so the preferred_username likely comes from the OAuth provider and may not need additional validation
- The naming inconsistency point is valid but not critical since this is just a data transfer struct
- The comment is asking the author to "ensure" and "consider" things rather than pointing out definite issues
- The comment is mixing multiple concerns into one comment
The comment raises some potentially valid points about validation and naming consistency. The code does show evidence of username validation being used elsewhere.
However, this is an OAuth data struct where the fields likely come directly from external providers. Making assumptions about validation needs without understanding the OAuth flow could lead to unnecessary complexity.
The comment should be deleted as it makes speculative suggestions without clear evidence of actual issues, and asks the author to "ensure" things rather than pointing out concrete problems.
3. backend/windmill-api/src/users.rs:1160
- Draft comment:
Typo: 'Usermame' should be corrected to 'Username' in the error message when the username does not match the expected pattern. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-api/src/users.rs:2244
- Draft comment:
Typo: Replace 'Usermame' with 'Username' in the error message that verifies the new username format in the rename_user function. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_yzPDG7yxCLIcLBg0
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Important
Add
preferred_username
field toLoginUserInfo
inusers.rs
for SSO provisioning support.preferred_username
field toLoginUserInfo
struct inusers.rs
to support SSO provisioning.ee-repo-ref.txt
to a new commit hash.This description was created by
for 90c383f. It will automatically update as commits are pushed.