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

[FIXED JENKINS-34881] - handle non-default security settings for new installs #2364

Merged
merged 4 commits into from Jun 2, 2016
Merged

[FIXED JENKINS-34881] - handle non-default security settings for new installs #2364

merged 4 commits into from Jun 2, 2016

Conversation

kzantow
Copy link
Contributor

@kzantow kzantow commented May 20, 2016

Some applications may provide an init script or otherwise configure security before the setup wizard runs. This correctly handles these situations by avoiding presenting the unlock screen and avoiding the 'create user' step in the setup wizard.

This addresses: https://issues.jenkins-ci.org/browse/JENKINS-34881

@reviewbybees esp. @oleg-nenashev

@@ -193,7 +188,7 @@ private static InstallState getDefaultInstallState() {
// Edge case: used Jenkins 1 but did not save the system config page,
// the version is not persisted and returns 1.0, so try to check if
// they actually did anything
if (!j.getItemMap().isEmpty() || !mayBeJenkins2SecurityDefaults(j) || !j.getNodes().isEmpty()) {
Copy link
Contributor Author

@kzantow kzantow May 20, 2016

Choose a reason for hiding this comment

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

We cannot rely on security settings for determining if this is an upgrade or new install; scripts may set up security.

Any better suggestions to handle this case are welcome! :)

@stephenc
Copy link
Member

🐝 AIUI (but if the security settings are present do we force a login to admin role before starting the wizard? (Remember the security realm could be something like Google apps authentication so presenting a username/password form would be a spectacular fail)

@kzantow
Copy link
Contributor Author

kzantow commented May 20, 2016

@stephenc we use the default behavior of the security realm / auth strategy, e.g. if you set LegacySecurityRealm, with anyone can do anything in an init script, it would bypass login altogether. Maybe I need some guidance what should happen here?

@stephenc
Copy link
Member

before you start the wizard you need to either:

  • have the default settings in play; or
  • be logged in as an administrator

So basically the page that starts the flow needs to require admin permissions if not using the new admin lockdown mode.

I think it is ok to block at that login screen before continuing if the wizard is going to be displayed.

@kzantow
Copy link
Contributor Author

kzantow commented May 20, 2016

@stephenc thanks for the feedback, it was already redirecting to the admin page, but it seemed unreliable; adding the extra checkPermission here seems to do the trick in both cases

@oleg-nenashev
Copy link
Member

@kzantow the behavior is strange. At least my SSO engine does not work well

  1. I get the admin lockdown screen with TMP password on startup
  2. When I enter the TMP password and submit, I get the login error
  3. After that...
  • If I click on the "login" hyperlink, I get the user/password dialog. SSO does not work
  • When I refresh the page, SSO works well. Jenkins displays the plugin selection screen

🐜 , seems redirect to an errorLogin by permission check is a wrong approach. Maybe page refresh? It should require login for the current permission check from what I see

@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label May 21, 2016
@oleg-nenashev
Copy link
Member

@kzantow gentle bump

@kzantow
Copy link
Contributor Author

kzantow commented May 26, 2016

@oleg-nenashev I tracked down the issue that the token-based login page was not forwarding the 'from' parameter; please have a look at this again

@oleg-nenashev
Copy link
Member

The change looks good. Testing it

@oleg-nenashev
Copy link
Member

@kzantow
the last change didn't help at east in my case :(

@oleg-nenashev
Copy link
Member

Retested the change again with jenkins-2.7. Everything works fine. Maybe it was a glitch caused by other fixes OR maybe I've just built the stuff improperly.

Works like a charm for me 🐝

@oleg-nenashev
Copy link
Member

@reviewbybees done

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jun 2, 2016
@oleg-nenashev
Copy link
Member

I am about merging this PR on the evening if there is no extra feedback

@oleg-nenashev oleg-nenashev merged commit 723dfca into jenkinsci:master Jun 2, 2016
oleg-nenashev added a commit that referenced this pull request Jun 4, 2016
olivergondza pushed a commit that referenced this pull request Jun 19, 2016
…ew installs (#2364)

* [FIXED JENKINS-34881] - handle non-default security settings for new installs

* Ensure permissions

* Initial security authentication token should still follow redirects

(cherry picked from commit 723dfca)
samatdav pushed a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016
…ew installs (jenkinsci#2364)

* [FIXED JENKINS-34881] - handle non-default security settings for new installs

* Ensure permissions

* Initial security authentication token should still follow redirects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
3 participants