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.

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

Bug 1921258: use sessionStorage for user-settings when impersonating #7958

Merged

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Jan 27, 2021

Analysis / Root cause:
When logged in as kubeadmin and impersonating a user, the access to the user-settings configmap for kubeadmin returns a 403 which puts the user settings hook in a bad state and will update whenever another tab is opened for the kubeadmin user and their settings are updated.

Solution Description:
When impersonating a user, use session storage as the backing store instead of configmap or local storage.

Screen shots / Gifs for design review:
No UI change.

Unit test coverage report:
Added unit test which checks for session storage usage when impersonating.

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. component/shared Related to console-shared labels Jan 27, 2021
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2021
@christianvogt christianvogt force-pushed the impersonate-settings branch 3 times, most recently from 843e955 to dc240f4 Compare January 28, 2021 22:22
@christianvogt christianvogt changed the title [WIP] use sessionStorage for user-settings when impersonating Bug 1921258: use sessionStorage for user-settings when impersonating Jan 28, 2021
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 28, 2021
@openshift-ci-robot
Copy link
Contributor

@christianvogt: This pull request references Bugzilla bug 1921258, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1921258: use sessionStorage for user-settings when impersonating

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/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 28, 2021
@invincibleJai
Copy link
Member

@christianvogt Verified it i no longer observed Switch in perspective when tried on one window with impersonate and another with kubeadmin.

Storing in session works as expected in chrome however in safari it doesn't seems to work as expected. That looks like a separate issue LMK if needed will log one

Jan-29-2021 17-17-32

@christianvogt
Copy link
Contributor Author

@invincibleJai thanks. I cannot test safari until i update my OS since my safari is too old and doesn't support ResizeObserver and the console errors out ;/

@invincibleJai
Copy link
Member

@invincibleJai thanks. I cannot test safari until i update my OS since my safari is too old and doesn't support ResizeObserver and the console errors out ;/

i verified again and it works fine in Safari as well earlier i was trying to look in storage and din't seem to get it reflect there then but seems fine now

image

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 29, 2021

@christianvogt: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/ceph-storage-plugin d0f4605 link /test ceph-storage-plugin

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@christianvogt
Copy link
Contributor Author

@invincibleJai that's for the review and test.
I did just notice from your screenshot that you are using the dev setup where you choose to store settings in local storage instead of configmaps. When we enable this setting, the user-settings key in the local storage is always set to console-user-settings as oppose to console-user-settings-<userid>. I fixed the key when impersonating.

@invincibleJai
Copy link
Member

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit eb649d7 into openshift:master Jan 29, 2021
@openshift-ci-robot
Copy link
Contributor

@christianvogt: All pull requests linked via external trackers have merged:

Bugzilla bug 1921258 has been moved to the MODIFIED state.

In response to this:

Bug 1921258: use sessionStorage for user-settings when impersonating

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/test-infra repository.

@spadgett spadgett added this to the v4.7 milestone Feb 1, 2021
@christianvogt christianvogt deleted the impersonate-settings branch June 2, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants