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

Removes unnecessary header x-miq-group #1360

Merged
merged 1 commit into from
Jan 18, 2018
Merged

Removes unnecessary header x-miq-group #1360

merged 1 commit into from
Jan 18, 2018

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Jan 17, 2018 •
edited
Loading

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

The sister has been slain, no longer a blocker, this pr is the winner ⚔️
This pr has a sister, ManageIQ/manageiq-api#287 , another way to solve the above bz

In the past, was used for group switching: http://manageiq.org/docs/reference/latest/api/overview/authorization

This is now done through the api, this code is no longer needed and in fact creates the issue (bz above), tl;dr - special characters in headers are bad and not to spec

PS: Totally tested group switching and logging in with invalid groups, everything behaves identical to master

@AllenBW
Removes unnecessary header x-miq-group
d49cab3 
In the past, was used for group switching: http://manageiq.org/docs/reference/latest/api/overview/authorization

This is now done through the api, this code is no longer needed and in fact creates the issue here: https://bugzilla.redhat.com/show_bug.cgi?id=1531626

tl;dr - special characters in headers are bad
@AllenBW AllenBW requested a review from himdel as a code owner January 17, 2018 17:16
@AllenBW AllenBW added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 17, 2018
@miq-bot miq-bot added the wip label Jan 17, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2018

Checked commit AllenBW@d49cab3 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@jntullo jntullo mentioned this pull request Jan 17, 2018
Merged
@jntullo
Copy link

jntullo commented Jan 17, 2018

I really like this approach, thanks @AllenBW for doing this / testing! If this is removed from the SUI, i think it will be a good idea to revisit the group header at all in the API.

@AllenBW AllenBW changed the title [WIP]BZ#1531626-Removes unnecessary header x-miq-group [WIP]Removes unnecessary header x-miq-group Jan 17, 2018
@AllenBW AllenBW changed the title [WIP]Removes unnecessary header x-miq-group Removes unnecessary header x-miq-group Jan 17, 2018
@AllenBW AllenBW removed the wip label Jan 17, 2018
@h-kataria
Copy link
Contributor

@jntullo does this mean ManageIQ/manageiq-api#287 should be closed

@jntullo
Copy link

jntullo commented Jan 17, 2018

@h-kataria no, we will still fix the API issue, in the event that users are still using this and have a special character in the group name

@himdel
Copy link
Contributor

himdel commented Jan 18, 2018 •
edited
Loading

Well ... the good thing is, this looks like it fixes the accent bug, and definitely doesn't make things worse :).

Given this is a blocker, merging.

The bad thing is: group switching is completely broken in SUI.
Switching between super-administrator and user doesn't update the menus, you have to switch the group, log out and log in again for the group switch to take effect in the UI.
It does seem to switch the group for API.

himdel
himdel approved these changes Jan 18, 2018
View reviewed changes
Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

👍

@himdel himdel merged commit 9c9a5d8 into ManageIQ:master Jan 18, 2018
@himdel
Copy link
Contributor

himdel commented Jan 18, 2018 •
edited
Loading

@AllenBW can you please look into fixing that? The original implementation used to cause the whole app to reload as if you were logging in, for the changes in the UI to take effect. (Or is it that we're caching rbac even after group switching?)

EDIT: fixed in #1363

@AllenBW AllenBW deleted the bug/master/#1531626-x-group-removal branch January 18, 2018 13:06
simaishi pushed a commit that referenced this pull request Jan 18, 2018
@himdel @simaishi
Merge pull request #1360 from AllenBW/bug/master/#1531626-x-group-rem…
1d0be64 
…oval

Removes unnecessary header x-miq-group
(cherry picked from commit 9c9a5d8)

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

Gaprindashvili backport details:

$ git log -1
commit 1d0be6466282425bcd290da4cec8bbde34511f0b
Author: Martin Hradil <himdel@seznam.cz>
Date:   Thu Jan 18 12:17:17 2018 +0100

    Merge pull request #1360 from AllenBW/bug/master/#1531626-x-group-removal
    
    Removes unnecessary header x-miq-group
    (cherry picked from commit 9c9a5d89e4ee82b3c1b58c85d32d828a4db41a6e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536047

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel approved these changes

Assignees

@himdel himdel

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

Successfully merging this pull request may close these issues.
6 participants
@AllenBW @miq-bot @jntullo @h-kataria @himdel @simaishi