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

[JENKINS-58495] Do not lose roles when newInstance() is invoked #4

Merged
merged 1 commit into from
Jul 17, 2019
Merged

[JENKINS-58495] Do not lose roles when newInstance() is invoked #4

merged 1 commit into from
Jul 17, 2019

Conversation

AbhyudayaSharma
Copy link
Member

This pull request fixes a bug where the configuration is lost when re-selecting 'Folder Based Authorization' in 'Configure Global Security' page.

Steps to reproduce the bug (fixed by this PR):

  • Select 'Folder based authorization' as the authorization strategy from the Web UI.
  • Add any role
  • Re-select 'Folder based authorization'
  • The added roles would have vanished (apart from the admin role that gets added automatically)

@AbhyudayaSharma AbhyudayaSharma added the bug Something isn't working label Jul 15, 2019
@AbhyudayaSharma AbhyudayaSharma requested a review from a team July 15, 2019 11:28
@AbhyudayaSharma AbhyudayaSharma self-assigned this Jul 15, 2019
@AbhyudayaSharma AbhyudayaSharma changed the title Do not lose roles when newInstance() is invoked [JENKINS-58495] Do not lose roles when newInstance() is invoked Jul 15, 2019
oleg-nenashev
oleg-nenashev approved these changes Jul 16, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I would not really call it a bug TBH. It is a result of splitting the strategy definition and Role management parts. Usually it is not advised, but Role Strategy doe sit due to the big configuration. IMHO it might make sense to consider moving the configuration to the global security page if it allows to retain the UX.

@AbhyudayaSharma
Copy link
Member Author

AbhyudayaSharma commented Jul 16, 2019 •
edited
Loading

IMHO it might make sense to consider moving the configuration to the global security page if it allows to retain the UX.

The POST requests require JSON request body which I believe an HTML <form> cannot send, so I don't think if it is possible to get the data we need from the Configure Global Security page.

@AbhyudayaSharma AbhyudayaSharma merged commit 23366f3 into jenkinsci:master Jul 17, 2019
@AbhyudayaSharma AbhyudayaSharma deleted the new-instance-fix branch July 17, 2019 11:29
@AbhyudayaSharma AbhyudayaSharma mentioned this pull request Jul 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

Assignees

@AbhyudayaSharma AbhyudayaSharma

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AbhyudayaSharma @oleg-nenashev