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.

Sign up for GitHub

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

Jump to bottom

Handle group names with encoded special characters #16998

Merged
merged 2 commits into from
Feb 16, 2018
Merged

Handle group names with encoded special characters #16998

merged 2 commits into from
Feb 16, 2018

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented Feb 13, 2018

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1469589

Group names in the headers at index X-REMOTE-USER-GROUPS need to
be decoded in order to handle special characters.

Steps for Testing/QA

  1. Configure appliance to use SAML or other external auth.
  2. Create user who has a custom group like "SR-APP-EPM-Membre-équipe"
  3. Add custom group to MiQ and assign role.
  4. log in with user.

@jvlcek
Handle group names with encoded special characters
5963f38 
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1469589

Group names in the headers at index X-REMOTE-USER-GROUPS need to
be decoded in order to handle special characters.
@jvlcek
Copy link
Member Author

jvlcek commented Feb 13, 2018

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Feb 13, 2018
@jvlcek
Copy link
Member Author

jvlcek commented Feb 13, 2018

@miq-bot add_label authentication

@jvlcek
Copy link
Member Author

jvlcek commented Feb 13, 2018

@miq-bot add_label gaprindashvili/yes

@jvlcek
Copy link
Member Author

jvlcek commented Feb 13, 2018

@miq-bot assign @abellotti

@jvlcek
Copy link
Member Author

jvlcek commented Feb 13, 2018

@jntullo and @abellotti Please review.

Thank you! JoeV

@jntullo
Copy link

jntullo commented Feb 14, 2018

LGTM! 👍

abellotti
abellotti reviewed Feb 14, 2018
View reviewed changes
app/models/authenticator/httpd.rb Outdated
@@ -120,7 +120,7 @@ def user_details_from_headers(username, request)
:lastname => request.headers['X-REMOTE-USER-LASTNAME'],
:email => request.headers['X-REMOTE-USER-EMAIL'],
:domain => request.headers['X-REMOTE-USER-DOMAIN']}
[user_attrs, (request.headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:,]/)]
[user_attrs, (CGI.unescape(request.headers['X-REMOTE-USER-GROUPS']) || '').split(/[;:,]/)]
Copy link
Member

Choose a reason for hiding this comment

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

we need to check that the request.headers['X-REMOTE-USER-GROUPS'] is present? first before we can do the CGI.unescape, otherwise, ugly stack trace.

Copy link
Member Author

Choose a reason for hiding this comment

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

@abellotti Good catch. I had the check || '' on the wrong side of the )
I just posted the commit withe the fix and an additional test in the spec to
confirm the handling of nil groups.

Thannk you! JoeV

@miq-bot
Copy link
Member

miq-bot commented Feb 15, 2018

Checked commits jvlcek/manageiq@5963f38~...a023437 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

app/models/authenticator/httpd.rb

@abellotti
Copy link
Member

LGTM!! Thanks @jvlcek for doing the update. will merge when 🍏

@abellotti abellotti added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 16, 2018
@abellotti abellotti merged commit dfc9c90 into ManageIQ:master Feb 16, 2018
simaishi pushed a commit that referenced this pull request Mar 7, 2018
@abellotti @simaishi
Merge pull request #16998 from jvlcek/bz1469589_group_special_chars
bec8219 
Handle group names with encoded special characters
(cherry picked from commit dfc9c90)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552792
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit bec82196b1ea93e53b37a77f6b225928b6689b4a
Author: Alberto Bellotti <[email protected]>
Date:   Fri Feb 16 13:20:24 2018 -0500

    Merge pull request #16998 from jvlcek/bz1469589_group_special_chars
    
    Handle group names with encoded special characters
    (cherry picked from commit dfc9c903ab05ff3868f49db28159d634acb442b8)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1552792

@jvlcek jvlcek deleted the bz1469589_group_special_chars branch May 1, 2018 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abellotti abellotti abellotti approved these changes

Assignees

@abellotti abellotti

Labels
bug core/authentication gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jvlcek @jntullo @miq-bot @abellotti @simaishi