fix: encode OpenShift group names in advanced authorization - #1071
nikitapal-12 wants to merge 2 commits into
Conversation
Group names with spaces (common for Active Directory) produced an unencoded Groups API path and failed with URISyntaxException when users opened the dashboard. Fixes https://issues.redhat.com/browse/CRW-13184 Signed-off-by: Nikita Pal <npal@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Hi @nikitapal-12. Thanks for your PR. I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@nikitapal-12 |
|
/retest |
|
@nikitapal-12: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest-required |
|
@nikitapal-12: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@nikitapal-12 thank you for contribution! |
|
@svor |
svor
left a comment
There was a problem hiding this comment.
LGTM
I've added a grop with name NEW GROUP:
apiVersion: user.openshift.io/v1
kind: Group
metadata:
name: NEW GROUP
users:
- cluster-adminEnabled advanced authorization in CheCluster
networking:
auth:
advancedAuthorization:
allowGroups:
- NEW GROUPPatched CheCluster to use che-server image built from the PR:
quay.io/vsvydenk/che-server:next
And I was able to login to the dashboard without errors:
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ibuziuk, nikitapal-12, svor, tolusha, vinokurig The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@nikitapal-12 Please run |
|
New changes are detected. LGTM label has been removed. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughOpenShift authorization checks now encode group names as URL path segments before fetching groups. Tests cover percent-encoding spaces and authorization for members and non-members of a group whose name contains spaces. ChangesOpenShift group lookup
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Group names containing spaces are encoded for the OpenShift lookup; no remaining issue is established that would prevent merging after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@nikitapal-12 please sign ECA |
|
@nikitapal-12 please sign ECA |
Group names with spaces (common for Active Directory) produced an unencoded Groups API path and failed with URISyntaxException when users opened the dashboard.
Fixes https://issues.redhat.com/browse/CRW-13184
What does this PR do?
Screenshot/screencast of this PR
What issues does this PR fix or reference?
How to test this PR?
URISyntaxExceptionfrom the Groups API path.PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedRelease Notes
Reviewers
Reviewers, please comment how you tested the PR when approving it.
Summary by CodeRabbit