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

Add a default implementation for SecurityListener #3077

Merged
merged 2 commits into from Oct 21, 2017

Conversation

5 participants
@Wadeck
Copy link
Contributor

Wadeck commented Oct 12, 2017

Proposition to add a default implementation on the SecurityListener class that is currently abstract by simply removing the abstract modifiers on the methods.
Two other possibilities could have been chosen:

  1. using an interface instead of an abstract class, but this implies two things, first we cannot have private static method (the all() method) but it's not really important since other extensions have set their own "all" to public ; the second is that the binary compatibility could be broken due to the change from class to interface.
  2. adding a layer like that was done for Swing, you have the simili interface SecurityListener that stays abstract with abstract method and a new abstract class that implements (empty body) each mandatory methods to let them becomes optional.

Could simplify a bit the #3074 like proposed on this comment

No tests added since it's a modification that do not change behaviors, existing tests suffice to check non-regression.

Submitter checklist

  • Appropriate autotests or explanation to why this change has no tests

Desired reviewers

@reviewbybees

@Wadeck Wadeck requested a review from oleg-nenashev Oct 12, 2017

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Oct 12, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

Looks reasonable to me. Even in the core there are examples where the calls are being implemented just to ignore them:

public static class SecurityListenerImpl extends SecurityListener {
@Override
protected void authenticated(@Nonnull UserDetails details) {
}
@Override
protected void failedToAuthenticate(@Nonnull String username) {
}
@Override
protected void loggedIn(@Nonnull String username) {
try {
// user should have been created but may not have been saved for some realms
// but as this is a callback of a successful login we can safely create the user.
User u = User.getById(username, true);
LastGrantedAuthoritiesProperty o = u.getProperty(LastGrantedAuthoritiesProperty.class);
if (o==null)
u.addProperty(o=new LastGrantedAuthoritiesProperty());
Authentication a = Jenkins.getAuthentication();
if (a!=null && a.getName().equals(username))
o.update(a); // just for defensive sanity checking
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Failed to record granted authorities",e);
}
}
@Override
protected void failedToLogIn(@Nonnull String username) {
// while this initially seemed like a good idea to avoid allowing wrong impersonation for too long,
// doing this means a malicious user can break the impersonation capability
// just by failing to login. See ApiTokenFilter that does the following, which seems better:
/*
try {
Jenkins.getInstance().getSecurityRealm().loadUserByUsername(username);
} catch (UserMayOrMayNotExistException x) {
// OK, give them the benefit of the doubt.
} catch (UsernameNotFoundException x) {
// Not/no longer a user; deny the API token. (But do not leak the information that this happened.)
chain.doFilter(request, response);
return;
} catch (DataAccessException x) {
throw new ServletException(x);
}
*/
// try {
// User u = User.getById(username,false);
// LastGrantedAuthoritiesProperty o = u.getProperty(LastGrantedAuthoritiesProperty.class);
// if (o!=null)
// o.invalidate();
// } catch (IOException e) {
// LOGGER.log(Level.WARNING, "Failed to record granted authorities",e);
// }
}
@Override
protected void loggedOut(@Nonnull String username) {
}
}
. It would be great to validate and cleanup such code as a part of the pull request.

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Oct 16, 2017

I also modify the inner class like requested @oleg-nenashev, just letting in place the empty method full of comment.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 20, 2017

I am about merging it to the next weekly if there is no negative feedback

@jglick

jglick approved these changes Oct 20, 2017

Copy link
Member

jglick left a comment

Yep, follows our usual policy for listeners.

Newly written listeners can be interfaces, since in Java 8 these can have default and static methods.

@oleg-nenashev oleg-nenashev merged commit 9c5c14a into jenkinsci:master Oct 21, 2017

1 check failed

continuous-integration/jenkins/pr-head This commit has test failures
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 21, 2017

@Wadeck thanks!

bkmeneguello pushed a commit to bkmeneguello/jenkins that referenced this pull request Oct 23, 2017

Add a default implementation for SecurityListener (jenkinsci#3077)
* - add a default implementation doing nothing for the different event on SecurityListener.

* - remove the useless non-implemented method in this inner class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment