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-56005] Add user property-change listener #3901

Closed
wants to merge 14 commits into from
Closed

[JENKINS-56005] Add user property-change listener #3901

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1fe3770
[JENKINS-56005] Add user property-change listener
davidolorundare Feb 17, 2019
f4465d1
[JENKINS-56008] Update security-realm with listener
davidolorundare Feb 17, 2019
124b84c
[JENKINS-56008] Add user property-change unit test
davidolorundare Feb 17, 2019
43b50ff
Merge branch 'master' of https://github.com/jenkinsci/jenkins
davidolorundare Mar 7, 2019
49400e5
Refactor securitylistener methods
davidolorundare Mar 11, 2019
13c8864
[JENKINS-56005] Add userpropertylistener interface
davidolorundare Mar 11, 2019
87b44db
[JENKINS-56008] Add passwordpropertylistener class
davidolorundare Mar 12, 2019
5de70ac
[JENKINS-56008] Refactor private-security realm
davidolorundare Mar 12, 2019
d0a19f0
[JENKINS-56170] Add apitokenpropertylistener class
davidolorundare Mar 12, 2019
d1382a6
[JENKINS-56170] Update apitokenproperty methods
davidolorundare Mar 12, 2019
97bfcc3
[JENKINS-56008] Update private-security realm unittest
davidolorundare Mar 12, 2019
cc75d2c
[JENKINS-56170] Update apitokenproperty unittests
davidolorundare Mar 12, 2019
4466180
Merge branch 'master' of https://github.com/jenkinsci/jenkins
davidolorundare Mar 20, 2019
4e1c33f
Refactor userpropertylistener methods
davidolorundare Mar 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,14 +717,18 @@ public Details newInstance(StaplerRequest req, JSONObject formData) throws FormE
if(!Util.fixNull(pwd).equals(Util.fixNull(pwd2)))
throw new FormException("Please confirm the password by typing it twice","user.password2");

User user = Util.getNearestAncestorOfTypeOrThrow(req, User.class);
UserProperty p = user.getProperty(Details.class);
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved

String data = Protector.unprotect(pwd);
if(data!=null) {
String prefix = Stapler.getCurrentRequest().getSession().getId() + ':';
if(data.startsWith(prefix))
SecurityListener.fireUserPropertyUpdated(p, user.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ missing {} in the if, so you are breaking the statement there.

That's exactly the main reason why I hate the if without brackets 👿

If that code bug was not discovered by your tests => there is a missing path in your coverage.

Copy link
Contributor Author

@davidolorundare davidolorundare Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

do you advise using the if for only one-liners; or to just never use a non-bracketed if at all ?

Like;

                if((data!=null) && (p != null)) {
                    String prefix = Stapler.getCurrentRequest().getSession().getId() + ':';
                    if(data.startsWith(prefix)) return Details.fromHashedPassword(data.substring(prefix.length()));
                }

versus;

                if((data!=null) && (p != null)) {
                    String prefix = Stapler.getCurrentRequest().getSession().getId() + ':';
                    if(data.startsWith(prefix)) {
                        return Details.fromHashedPassword(data.substring(prefix.length()));
                    }
                }

Copy link
Contributor

Choose a reason for hiding this comment

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

I can only advice to never use a if without the associated {}, to avoid situation where someone wants to add a line, but due to the missing brackets will break the code flow like here.

And the one-liner is worse from my PoV due to the poor readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perks of using Python over Java :)

Copy link
Member

Choose a reason for hiding this comment

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

I only use one line ifs for things like:

if (foo == null) return null;

or

if (bar) return false;

or

if (baz) return;

Basically, only for control flow type things. Remember, forgetting your brackets can cause security problems (see the old Apple bug). If you auto-format your code before committing, you can see if you missed brackets based on how it indents things.

return Details.fromHashedPassword(data.substring(prefix.length()));
}

User user = Util.getNearestAncestorOfTypeOrThrow(req, User.class);
SecurityListener.fireUserPropertyUpdated(p, user.getId());
// the UserSeedProperty is not touched by the configure page
UserSeedProperty userSeedProperty = user.getProperty(UserSeedProperty.class);
if (userSeedProperty != null) {
Expand Down
19 changes: 19 additions & 0 deletions core/src/main/java/jenkins/security/SecurityListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import hudson.ExtensionList;
import hudson.ExtensionPoint;
import hudson.model.UserProperty;
import hudson.security.AbstractPasswordBasedSecurityRealm;
import hudson.security.SecurityRealm;
import java.util.ArrayList;
Expand Down Expand Up @@ -78,6 +79,16 @@ protected void loggedIn(@Nonnull String username){}
*/
protected void userCreated(@Nonnull String username) {}

/**
* @since TODO
*
* Fired after an existing user object property has been updated.
*
* @param p the user object property
* @param username the username
*/
protected void userPropertyUpdated(@Nonnull UserProperty p, @Nonnull String username){}
Copy link
Member

Choose a reason for hiding this comment

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

I kind of wonder if this would be more appropriate in its own UserPropertyListener or similar. I'm thinking in terms that it may eventually be useful to have event listeners for JobProperty, NodeProperty, and others if I'm forgetting.

Also, I'm not sure if there's precedence in this class already, but I think it might make more sense to flip the ordering of the parameters here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really convinced one way or another. The userPropertyUpdated() method doesn't quite seem to fit the other things in SecurityListener. But then, some of the other items in SecurityListener don't fit well either. The user property is more about users than specifically about security. But so are some of the other things in this file.

I guess when it comes down to it I have a slight preference for separating out a UserPropertyListener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmh.. alright. Question though is for the new UserPropertyListener: it being an abstract-class versus interface ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Wadeck Any thoughts on the creation of a new separate UserPropertyListener ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Separation seems better for the long term, like Matt mentioned, about the other items that could/would require such listeners in the future. And also, it's not directly security related, so not really a reason to be put in SecurityListener, except for the specific case of Details. But I imagine the goal of this listener is to be triggered everytime a property change, in general.

Copy link
Contributor Author

@davidolorundare davidolorundare Feb 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@Wadeck hmh.. abstract versus interface ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will bet on abstract being the most natural way as you are providing some static helper methods (for the event trigger). The interfaces should be used for contract definition.

Copy link
Member

Choose a reason for hiding this comment

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

Interfaces allow static methods as of Java 8. See for example the methods added to the collections API. Also default methods.


/**
* Fired when a user has failed to log in.
* Would be called after {@link #failedToAuthenticate}.
Expand Down Expand Up @@ -115,6 +126,14 @@ public static void fireUserCreated(@Nonnull String username) {
}
}

/** @since TODO */
public static void fireUserPropertyUpdated(@Nonnull UserProperty p, @Nonnull String username) {
LOGGER.log(Level.FINE, "user {0} property updated", username);
for (SecurityListener l : all()) {
l.userPropertyUpdated(p, username);
}
}

/** @since 1.569 */
public static void fireFailedToAuthenticate(@Nonnull String username) {
LOGGER.log(Level.FINE, "failed to authenticate: {0}", username);
Expand Down
47 changes: 47 additions & 0 deletions test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,23 @@

import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
import com.gargoylesoftware.htmlunit.HttpMethod;
import com.gargoylesoftware.htmlunit.Page;
import com.gargoylesoftware.htmlunit.WebRequest;
import com.gargoylesoftware.htmlunit.html.HtmlForm;
import com.gargoylesoftware.htmlunit.html.HtmlPage;
import com.gargoylesoftware.htmlunit.util.NameValuePair;
import com.gargoylesoftware.htmlunit.xml.XmlPage;
import hudson.ExtensionList;
import hudson.model.User;
import hudson.model.UserProperty;
import hudson.remoting.Base64;
import static hudson.security.HudsonPrivateSecurityRealm.CLASSIC;
import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER;
import hudson.security.pages.SignupPage;
import java.io.UnsupportedEncodingException;
import java.net.URL;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
Expand Down Expand Up @@ -342,6 +346,41 @@ public void userCreationWithHashedPasswords() throws Exception {
assertTrue(spySecurityListener.createdUsers.get(0).equals("charlie_hashed"));
}

@Issue("JENKINS-56008")
@Test
public void updateUserPassword() throws Exception {
HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null);
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
j.jenkins.setSecurityRealm(securityRealm);
WebClient wc = j.createWebClient();

spySecurityListener.usersWithPasswordUpdate.clear();
assertTrue(spySecurityListener.usersWithPasswordUpdate.isEmpty());

// new user account creation
SignupPage signup = new SignupPage(wc.goTo("signup"));
signup.enterUsername("debbie");
signup.enterPassword("debbie");
signup.enterFullName(StringUtils.capitalize("debbie user"));
signup.enterEmail("debbie" + "@" + "debbie" + ".com");
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
HtmlPage p = signup.submit(j);

assertEquals(200, p.getWebResponse().getStatusCode());

// execute an http request to change a user's password from their config page
User debbie = User.getById("debbie", false);
URL configPage = wc.createCrumbedUrl(debbie.getUrl() + "/" + "configSubmit");
String formData = "{\"fullName\": \"debbie user\", \"description\": \"\", \"userProperty3\": {\"primaryViewName\": \"\"}, \"userProperty5\": {\"password\": \"admin\", \"$redact\": [\"password\", \"password2\"], \"password2\": \"admin\"}, \"userProperty6\": {\"authorizedKeys\": \"\"}, \"userProperty8\": {\"insensitiveSearch\": true}, \"core:apply\": \"true\"}";

WebRequest request = new WebRequest(configPage, HttpMethod.POST);
request.setAdditionalHeader("Content-Type", "application/x-www-form-urlencoded");
request.setRequestBody("json=" + URLEncoder.encode(formData, StandardCharsets.UTF_8.name()));
davidolorundare marked this conversation as resolved.
Show resolved Hide resolved
Page page = wc.getPage(request);

// ensure user whose password was changed was in fact logged
assertTrue(spySecurityListener.usersWithPasswordUpdate.get(0).equals("debbie"));
assertEquals(200, page.getWebResponse().getStatusCode());
}

private void createFirstAccount(String login) throws Exception {
assertNull(User.getById(login, false));

Expand Down Expand Up @@ -419,6 +458,7 @@ private void selfRegistration(String login) throws Exception {
public static class SpySecurityListenerImpl extends SecurityListener {
private List<String> loggedInUsernames = new ArrayList<>();
private List<String> createdUsers = new ArrayList<String>();
private List<String> usersWithPasswordUpdate = new ArrayList<>();

@Override
protected void loggedIn(@Nonnull String username) {
Expand All @@ -427,6 +467,13 @@ protected void loggedIn(@Nonnull String username) {

@Override
protected void userCreated(@Nonnull String username) { createdUsers.add(username); }

@Override
protected void userPropertyUpdated(@Nonnull UserProperty p, @Nonnull String username){
if (p instanceof HudsonPrivateSecurityRealm.Details) {
usersWithPasswordUpdate.add(username);
}
}
}

@Issue("SECURITY-786")
Expand Down