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

Code improvements #228

Merged
merged 15 commits into from
Oct 22, 2021
Merged

Code improvements #228

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2ab1906
Code clean-up
offa Sep 21, 2021
6d569ef
Add missing nullability annotations
offa Sep 21, 2021
9f92648
Replace deprecated API usages
offa Sep 21, 2021
ada1a72
Remove JSR 305 annotations
offa Sep 21, 2021
1bfdf46
Create user to preserve behaviour.
offa Sep 29, 2021
05586ee
Merge thrown exceptions.
offa Sep 29, 2021
f78c470
Revert descriptor changes.
offa Sep 29, 2021
a8148b8
Use getConstructor()
offa Oct 1, 2021
86daf65
Fix parameter of getById()
offa Oct 1, 2021
b2b718e
Revert to newInstance()
offa Oct 1, 2021
e25451e
Adjust field + constructor for store
Wadeck Oct 1, 2021
3a61456
Merge branch 'master' into offa_dev
Wadeck Oct 1, 2021
d458d3a
Organize CCTest imports
Wadeck Oct 1, 2021
f66b935
Revert changes suggested in the review
offa Oct 1, 2021
e9cf377
Merge branch 'master' of https://github.com/jenkinsci/credentials-plu…
jglick Oct 22, 2021
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
2 changes: 1 addition & 1 deletion src/main/java/com/cloudbees/plugins/credentials/CredentialsDescriptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ public static <T extends ModelObject> T findContextInPath(@NonNull StaplerReques
if (pathSegments.size() >= 2) {
String firstSegment = pathSegments.get(0);
if ("user".equals(firstSegment)) {
User user = User.get(pathSegments.get(1));
User user = User.getById(pathSegments.get(1), false);
offa marked this conversation as resolved.
Show resolved Hide resolved
if (type.isInstance(user) && CredentialsProvider.hasStores(user)) {
// we have a winner
return type.cast(user);
Expand Down
7 changes: 5 additions & 2 deletions src/main/java/com/cloudbees/plugins/credentials/CredentialsNameProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import com.cloudbees.plugins.credentials.common.IdCredentials;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;

import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand Down Expand Up @@ -89,13 +91,14 @@ Result name(@NonNull Credentials credentials, @NonNull Class<?> clazz) {
NameWith nameWith = clazz.getAnnotation(NameWith.class);
if (nameWith != null) {
try {
CredentialsNameProvider nameProvider = nameWith.value().newInstance();
CredentialsNameProvider nameProvider = nameWith.value().getDeclaredConstructor().newInstance();
offa marked this conversation as resolved.
Show resolved Hide resolved
String name = nameProvider.getName(credentials);
if (!name.isEmpty()) {
LOGGER.fine(() -> "named `" + name + "` from " + nameProvider);
return new Result(name, nameWith.priority());
}
} catch (ClassCastException | InstantiationException | IllegalAccessException e) {
} catch (ClassCastException | InstantiationException | IllegalAccessException
| NoSuchMethodException | InvocationTargetException e) {
offa marked this conversation as resolved.
Show resolved Hide resolved
// ignore
}
}
Expand Down
23 changes: 11 additions & 12 deletions src/main/java/com/cloudbees/plugins/credentials/CredentialsParameterDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import com.cloudbees.plugins.credentials.common.StandardCredentials;
import com.cloudbees.plugins.credentials.common.StandardListBoxModel;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.model.Descriptor;
import hudson.model.Item;
import hudson.model.ParameterDefinition;
import hudson.model.ParameterValue;
Expand Down Expand Up @@ -117,6 +117,7 @@ public static class DescriptorImpl extends ParameterDescriptor {
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return Messages.CredentialsParameterDefinition_DisplayName();
Expand All @@ -125,29 +126,27 @@ public String getDisplayName() {
public ListBoxModel doFillCredentialTypeItems() {
ListBoxModel result = new ListBoxModel();
result.add("Any", StandardCredentials.class.getName());
for (Descriptor<Credentials> d : CredentialsProvider.allCredentialsDescriptors()) {
if (!(d instanceof CredentialsDescriptor)) {
for (CredentialsDescriptor d : CredentialsProvider.allCredentialsDescriptors()) {
offa marked this conversation as resolved.
Show resolved Hide resolved
if (d == null) {
offa marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
CredentialsDescriptor descriptor = (CredentialsDescriptor) d;
if (StandardCredentials.class.isAssignableFrom(descriptor.clazz)) {
result.add(descriptor.getDisplayName(), descriptor.clazz.getName());
if (StandardCredentials.class.isAssignableFrom(d.clazz)) {
result.add(d.getDisplayName(), d.clazz.getName());
}
}
return result;
}

private Class<? extends StandardCredentials> decodeType(String credentialType) {
for (Descriptor<Credentials> d : CredentialsProvider.allCredentialsDescriptors()) {
if (!(d instanceof CredentialsDescriptor)) {
for (CredentialsDescriptor d : CredentialsProvider.allCredentialsDescriptors()) {
if (d == null) {
offa marked this conversation as resolved.
Show resolved Hide resolved
continue;
}
CredentialsDescriptor descriptor = (CredentialsDescriptor) d;
if (!StandardCredentials.class.isAssignableFrom(descriptor.clazz)) {
if (!StandardCredentials.class.isAssignableFrom(d.clazz)) {
continue;
}
if (credentialType.equals(descriptor.clazz.getName())) {
return descriptor.clazz.asSubclass(StandardCredentials.class);
if (credentialType.equals(d.clazz.getName())) {
return d.clazz.asSubclass(StandardCredentials.class);
}
}
return StandardCredentials.class;
Expand Down
151 changes: 72 additions & 79 deletions src/main/java/com/cloudbees/plugins/credentials/CredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,84 +695,80 @@ public static boolean hasStores(final ModelObject context) {
*/
public static Iterable<CredentialsStore> lookupStores(final ModelObject context) {
final ExtensionList<CredentialsProvider> providers = all();
return new Iterable<CredentialsStore>() {
public Iterator<CredentialsStore> iterator() {
return new Iterator<CredentialsStore>() {
private ModelObject current = context;
private Iterator<CredentialsProvider> iterator = providers.iterator();
private CredentialsStore next;

public boolean hasNext() {
return () -> new Iterator<CredentialsStore>() {
private ModelObject current = context;
private Iterator<CredentialsProvider> iterator = providers.iterator();
private CredentialsStore next;

public boolean hasNext() {
if (next != null) {
return true;
}
while (current != null) {
while (iterator.hasNext()) {
CredentialsProvider p = iterator.next();
if (!p.isEnabled(context)) {
continue;
}
next = p.getStore(current);
if (next != null) {
return true;
}
while (current != null) {
while (iterator.hasNext()) {
CredentialsProvider p = iterator.next();
if (!p.isEnabled(context)) {
continue;
}
next = p.getStore(current);
if (next != null) {
return true;
}
}
// now walk up the model object tree
// TODO make this an extension point perhaps ContextResolver could help
if (current instanceof Item) {
current = ((Item) current).getParent();
iterator = providers.iterator();
} else if (current instanceof User) {
Jenkins jenkins = Jenkins.get();
Authentication a;
if (jenkins.hasPermission(USE_ITEM) && current == User.current()) {
// this is the fast path for the 99% of cases
a = Jenkins.getAuthentication();
} else {
try {
a = ((User) current).impersonate();
} catch (UsernameNotFoundException e) {
a = null;
}
}
if (current == User.current() && jenkins.getACL().hasPermission(a, USE_ITEM)) {
current = jenkins;
iterator = providers.iterator();
} else {
current = null;
}
} else if (current instanceof Jenkins) {
// escape
current = null;
} else if (current instanceof ComputerSet) {
current = Jenkins.get();
iterator = providers.iterator();
} else if (current instanceof Computer) {
current = Jenkins.get();
iterator = providers.iterator();
} else if (current instanceof Node) {
current = Jenkins.get();
iterator = providers.iterator();
} else {
// fall back to Jenkins as the ultimate parent of everything else
current = Jenkins.get();
iterator = providers.iterator();
}
// now walk up the model object tree
// TODO make this an extension point perhaps ContextResolver could help
if (current instanceof Item) {
current = ((Item) current).getParent();
iterator = providers.iterator();
} else if (current instanceof User) {
Jenkins jenkins = Jenkins.get();
Authentication a;
if (jenkins.hasPermission(USE_ITEM) && current == User.current()) {
// this is the fast path for the 99% of cases
a = Jenkins.getAuthentication();
} else {
try {
a = ((User) current).impersonate();
} catch (UsernameNotFoundException e) {
a = null;
}
}
return false;
}

public CredentialsStore next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
try {
return next;
} finally {
next = null;
if (current == User.current() && jenkins.getACL().hasPermission(a, USE_ITEM)) {
current = jenkins;
iterator = providers.iterator();
} else {
current = null;
}
} else if (current instanceof Jenkins) {
// escape
current = null;
} else if (current instanceof ComputerSet) {
current = Jenkins.get();
iterator = providers.iterator();
} else if (current instanceof Computer) {
current = Jenkins.get();
iterator = providers.iterator();
} else if (current instanceof Node) {
current = Jenkins.get();
iterator = providers.iterator();
} else {
// fall back to Jenkins as the ultimate parent of everything else
current = Jenkins.get();
iterator = providers.iterator();
}
};
}
return false;
}

public CredentialsStore next() {
if (!hasNext()) {
throw new NoSuchElementException();
}
try {
return next;
} finally {
next = null;
}
}
};
}
Expand Down Expand Up @@ -1068,6 +1064,7 @@ public boolean isEnabled(Object context) {
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return StringUtils.join(StringUtils.splitByCharacterTypeCamelCase(getClass().getSimpleName()), ' ');
Expand Down Expand Up @@ -1658,19 +1655,16 @@ public static <C extends Credentials> List<C> trackAll(@NonNull Item item, @NonN
* {@link DoNotUse} in order to ensure that no plugin attempts to call this method. If invoking this method
* from an {@code init.d} Groovy script, ensure that the call is guarded by a marker file such that
*
* @throws IOException if things go wrong.
*/
@Restricted(DoNotUse.class) // Do not use from plugins
public static void saveAll() throws IOException {
public static void saveAll() {
LOGGER.entering(CredentialsProvider.class.getName(), "saveAll");
try {
Jenkins jenkins = Jenkins.get();
jenkins.checkPermission(Jenkins.ADMINISTER);
LOGGER.log(Level.INFO, "Forced save credentials stores: Requested by {0}",
StringUtils.defaultIfBlank(Jenkins.getAuthentication().getName(), "anonymous"));
Timer.get().execute(new Runnable() {
@Override
public void run() {
Timer.get().execute(() -> {
try (ACLContext ctx = ACL.as(ACL.SYSTEM)) {
if (jenkins.getInitLevel().compareTo(InitMilestone.JOB_LOADED) < 0) {
LOGGER.log(Level.INFO, "Forced save credentials stores: Initialization has not completed");
Expand Down Expand Up @@ -1722,7 +1716,7 @@ public void run() {
LOGGER.log(Level.INFO, "Forced save credentials stores: Processing Users ({0} processed)",
count);
}
// HACK ALERT we just want to access the user's stores so we do just enough impersonation
// HACK ALERT we just want to access the user's stores, so we do just enough impersonation
// to ensure that User.current() == user
// while we could use User.impersonate() that would force a query against the backing
// SecurityRealm to revalidate
Expand All @@ -1743,8 +1737,7 @@ public void run() {
} finally {
LOGGER.log(Level.INFO, "Forced save credentials stores: Completed");
}
}
});
});
} finally {
LOGGER.exiting(CredentialsProvider.class.getName(), "saveAll");
}
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/cloudbees/plugins/credentials/CredentialsProviderFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ public static class DescriptorImpl extends CredentialsProviderFilterDescriptor {
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return Messages.CredentialsProviderFilter_None_DisplayName();
Expand Down Expand Up @@ -252,6 +253,7 @@ public static class DescriptorImpl extends CredentialsProviderFilterDescriptor {
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return Messages.CredentialsProviderFilter_Includes_DisplayName();
Expand Down Expand Up @@ -364,6 +366,7 @@ public static class DescriptorImpl extends CredentialsProviderFilterDescriptor {
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return Messages.CredentialsProviderFilter_Excludes_DisplayName();
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/com/cloudbees/plugins/credentials/CredentialsProviderManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public static CredentialsProviderManager getInstanceOrDie() {
* {@inheritDoc}
*/
@Override
public boolean filter(Object context, Descriptor descriptor) {
public boolean filter(Object context, @NonNull Descriptor descriptor) {
Map<CredentialsProviderTypeRestrictionDescriptor, List<CredentialsProviderTypeRestriction>> restrictions =
restrictions();
if (restrictions != null && descriptor instanceof CredentialsDescriptor) {
Expand Down Expand Up @@ -446,6 +446,7 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti
/**
* {@inheritDoc}
*/
@NonNull
@Override
public GlobalConfigurationCategory getCategory() {
return GlobalConfigurationCategory.get(GlobalCredentialsConfiguration.Category.class);
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/cloudbees/plugins/credentials/CredentialsProviderTypeRestriction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package com.cloudbees.plugins.credentials;

import edu.umd.cs.findbugs.annotations.NonNull;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.ExtensionPoint;
Expand Down Expand Up @@ -228,6 +229,7 @@ public boolean filter(List<CredentialsProviderTypeRestriction> restrictions, Cre
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return Messages.CredentialsProviderTypeRestriction_Includes_DisplayName();
Expand Down Expand Up @@ -383,6 +385,7 @@ public boolean filter(List<CredentialsProviderTypeRestriction> restrictions, Cre
/**
* {@inheritDoc}
*/
@NonNull
@Override
public String getDisplayName() {
return Messages.CredentialsProviderTypeRestriction_Excludes_DisplayName();
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/com/cloudbees/plugins/credentials/CredentialsResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import java.util.ArrayList;
Expand Down Expand Up @@ -126,14 +128,14 @@ public static <C extends Credentials> CredentialsResolver<Credentials, C> getRes
// if the reflective instantiation proves a hot point, put a cache in front.
try {
@SuppressWarnings("unchecked")
final CredentialsResolver<Credentials, C> resolver = resolveWith.value().newInstance();
final CredentialsResolver<Credentials, C> resolver = resolveWith.value().getDeclaredConstructor().newInstance();
offa marked this conversation as resolved.
Show resolved Hide resolved
offa marked this conversation as resolved.
Show resolved Hide resolved
if (Credentials.class.isAssignableFrom(resolver.getFromClass())
&& clazz.isAssignableFrom(resolver.getToClass())) {
return resolver;
}
LOGGER.log(Level.SEVERE, "Resolver {0} for type {1} resolves to {2} which is not assignable to {1}",
new Object[]{resolver.getClass(), clazz, resolver.getToClass()});
} catch (InstantiationException | IllegalAccessException e) {
} catch (InstantiationException | IllegalAccessException | NoSuchMethodException | InvocationTargetException e) {
LOGGER.log(Level.WARNING, "Could not instantiate resolver: " + resolveWith.value(), e);
return null;
}
Expand Down
Loading