Fix special characters in MIQ_GROUP header

[jntullo]

Non-ASCII characters are not permitted in headers. In our case, we have an X_MIQ_GROUP header that specifies the group description. Our group descriptions allow special characters (as in the case SR-APP-EPM-Membre-équipe, which poses an interesting dilemma.

Headers with special characters get encoded incorrectly (ie, SR-APP-EPM-Membre-\xE9quipe), causing the following error, which does not allow the SUI to proceed with login if the user's current group contains a special character:

ActiveRecord::StatementInvalid: PG::CharacterNotInRepertoire: ERROR:  invalid byte sequence for encoding "UTF8": 0xe9 0x71 0x75
: SELECT  "miq_groups".* FROM "miq_groups" INNER JOIN "miq_groups_users" ON "miq_groups"."id" = "miq_groups_users"."miq_group_id" WHERE "miq_groups_users"."user_id" = $1 AND "miq_groups"."description" = $2 LIMIT $3

Gif to demonstrate before behavior (pretty uneventful):

This fix properly encodes it back into UTF8, however, I am on the fence on whether we should allow this, or if group description validation should be changed to not allow special characters (although, may be difficult because groups already exist with special characters).

The following gif demonstrates login working successfully with a user that belongs to a group with a special character:

Thoughts? @imtayadeway @abellotti @gtanzillo
cc: @AllenBW @yrudman
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1531626#c9

@miq-bot add_label bug, blocker, gaprindashvili/yes

[gtanzillo]

Would like @abellotti to review

[jntullo]

After discussion with @AllenBW and @imtayadeway - we think that the SUI does not need this header at all, because they now are able to change the current group via set_current_group, and the group header is always just the user's current_group, which is unnecessary to specify. The related SUI pr is: ManageIQ/manageiq-ui-service#1360

[gtanzillo]

@jntullo Based on your comment, do you think we still need this change to the API?

[jntullo]

@gtanzillo I am not sure. If we decide to keep this header available in the API as a way to authenticate against a different group than the user's current_group, then it would make sense that we should fix this in the event that someone does have a special character in their group name and are using this header.

However, I'm not sure that this header is even needed anymore. Users are now able to change their current group ("action": "set_current_group"), and it seems to make more sense that they are authenticated against their current group, not the one specified in the headers. I'll start a discussion in the API gitter room.

I will remove the BZ reference from the commit, though, as I believe the SUI approach is the better solution to that particular bug.

[abellotti]

Yes, we still need to fix this and keep supporting the header.

[jntullo]

@abellotti pushed up another commit that may be a better approach. Perhaps we should require that the header be properly encoded. I tested by properly escaping the header like so:

req['X_MIQ_GROUP'] = CGI.escape('SR-APP-EPM-Membre-équipe')

and it works as expected.

cc: @imtayadeway

[imtayadeway]

Perhaps we should require that the header be properly encoded

Agree. If we must keep compatibility with the current behavior, we shouldn't be trying to work around clients sending non-ASCII characters.

[abellotti]

Thanks @jntullo for fixing this. Will merge when 🍏

[miq-bot]

Checked commit jntullo@2fcc33b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

[imtayadeway]

🎉 Thanks @jntullo !

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 9cbb5d91f4db9ce0fd855c06ddf7b2dcd1f593d6
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Wed Jan 17 15:28:25 2018 -0500

    Merge pull request #287 from jntullo/bz_1531626
    
    Fix special characters in MIQ_GROUP header
    (cherry picked from commit e89fc56e6a6be50447fe61c210f254843c61eb6d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536047