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

[JENKINS-29867] - Permissions engine + global option for disabling the Injected vars #57

Merged

Conversation

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Aug 9, 2015

https://issues.jenkins-ci.org/browse/JENKINS-29867

In several cases Jenkins users may expose passwords as a plain text in "Injected variables" action (e.g. password reassigning for a non-sensitive variable). It may make sense to add an option, which completely prohibits the exposure of environment variables.

Depends on jenkinsci/envinject-lib#6

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Aug 9, 2015

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.

pom.xml Outdated
@@ -38,7 +38,7 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<maven.compiler.source>1.6</maven.compiler.source>
<maven.compiler.target>1.6</maven.compiler.target>
<envinject.lib.version>1.22</envinject.lib.version>
<envinject.lib.version>1.23-SNAPSHOT</envinject.lib.version>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 9, 2015

Author Member

Should be bumped before the release

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Aug 9, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

initialContext = hudson.security.ACL.impersonate(auth);
return item.hasPermission(permission);
} catch (UsernameNotFoundException ex) {
throw new AssertionError("The specified user is not found", ex);

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

use fail(...) instead of throwing assertion error

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

fail(...) does not allow to attach the nested exception. In any case, it just wraps the throw call, so I don't see an issue with the current approach

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

so why not just don't catch it?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

I'm not sure about it. Missing username at this stage is an impossible state meaning that something is borked by a parallel thread. I'd prefer to fail the test immediately in such case => Runtime Assertion error.

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

no, assertion error is for test failure - e.g. it should be as you expect in this case, but it not. Any unexpected failures causes test to be "broken" test. So it should be just any other exception.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

OK

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 11, 2015

Author Member

Fixed it

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 11, 2015

Member

thanks!

@@ -46,6 +47,9 @@ public void overrideAll(Map<String, String> all) {
}

public String getIconFileName() {
if (!EnvInjectPlugin.canViewInjectedVars(build)) {
return null;

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 10, 2015

Member

This method would require @CheckForNull

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

This annotation is being inherited from the Action::getIconFileName(), hence there is no need to refer it again. BTW the @Override annotation would be useful to make it more clear

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

Added annotations

}

/**
* Gets the default configuration of {@link DockerTraceabilityPlugin}

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 10, 2015

Member

DockerTraceabilityPlugin?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

Thanks for catching! It's a copy-paste issue

@@ -32,7 +39,7 @@ public EnvInjectVarList(Map<String, String> envMap) {
public Map<String, String> getEnvMap() {
return envVars;
}

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 10, 2015

Member

whitespaces typo

private FreeStyleProject p;
private CaptureEnvironmentBuilder capture;

@Before

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 10, 2015

Member

It probably works here, but I saw weird things when using @Before/@After within JenkinsRule.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

It would be great if you submit them as JIRA issues if these things are reproducible

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

before and after should work fine with rule

This comment has been minimized.

Copy link
@amuniz

amuniz Aug 10, 2015

Member

I don't remember exactly the under what circumstances it failed (will file the issue next time I see it. Probably it was @BeforeClass/@AfterClass). Nevermind, if it's working here, it's working.

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

@amuniz JFYI @BeforeClass/@AfterClass executed "inside" @ClassRule but "around" all @Rule executions.

Same with @Before/@After - they executed "inside" of @Rule execution (e.g. after rule starts and before rule ends)

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

@Before does not with RestartableJenkinsRule, perhaps that is what you meant. Should work fine with JenkinsRule.

protected XmlFile getConfigXml() {
// We use a new configuration file, because the old one is occupied by EnvInjectNodeProperty
// (ideally needs a migration of data files)
return new XmlFile(Jenkins.XSTREAM, new File(getJenkinsInstance().getRootDir(),"envinject2.xml"));

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

think file name should not contain index as it very confusing. Can it be named as envinject-global or envinject-plugin-configuration?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

any name is confusing, because we already have envinject.xml. Probably I'll change the name to org.jenkinsci.plugins.envinject.EnvInjectPlugin.xml to follow the common pattern. Do you agree?

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

why not envinject-global-plugin-configuration.xml? With that name envinject.xml looks like some tmp file

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

global passwords are also the global configuration :(

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

if envinject-plugin-configuration.xml disliked too (why?), so common pattern is ok for me.


@Override
public void configure(StaplerRequest req, JSONObject formData) throws IOException, ServletException, Descriptor.FormException {
EnvInjectPluginConfiguration _configuration = req.bindJSON(EnvInjectPluginConfiguration.class, formData);

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 10, 2015

Member

think underlines in names can be replaced with short form of the word - for example config. Underlines is recomended to use only for restricted java identifiers (package, class...)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 10, 2015

Author Member

agreed

@@ -45,20 +46,38 @@ public void overrideAll(Map<String, String> all) {
return UnmodifiableMap.decorate(envMap);
}

@Override

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 11, 2015

Member

CheckForNull?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 11, 2015

Author Member

No need for overrides. Supermethods should do it newer APIs


@Override
public String getDisplayName() {
return "N/A";

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 11, 2015

Member

Wrote normal name, difficult identify in descriptor list when debugging

}

@Extension
public static final DescriptorImpl DESCRIPTOR = new DescriptorImpl();

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 11, 2015

Member

Why not use @Extension under Descriptor class?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 11, 2015

Author Member

There was a reason to do it , but I do not recall it

<j:set var="descriptor" value="${instance.descriptor}" />
<st:include from="${descriptor}" page="${descriptor.configPage}" optional="false" />
</f:section>
</j:jelly>

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 11, 2015

Member

remove NL

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 12, 2015

Author Member

removed the config at all after @jglick's comment

@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented Aug 11, 2015

Please fix new line endings, put annotations and squash before merge, then 👍

* @author Oleg Nenashev
*/
@ExportedBean
public class EnvInjectPlugin extends Plugin {

This comment has been minimized.

Copy link
@jglick

jglick Aug 11, 2015

Member

Using Plugin this way is deprecated. Use GlobalConfiguration instead. (Or just add a global.jelly view to some existing Describable if appropriate.)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 11, 2015

Author Member

Yes, makes sense to migrate to GlobalConfiguration. I still like the centralized Plugin-based approach, but the most of its functionality is being covered by new extension points

This comment has been minimized.

Copy link
@oleg-nenashev

void configure(EnvInjectPluginConfiguration configuration) throws IOException {
this.configuration = configuration;
VIEW_INJECTED_VARS.setEnabled(configuration.isEnablePermissions());

This comment has been minimized.

Copy link
@jglick

jglick Aug 11, 2015

Member

This sucks but pending JENKINS-17200 there may be no better way.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 12, 2015

Author Member

IMO all this Permission/PermissionGroup enabling/disabling stuff should be provided by Jenkins core. Keeping it on plugins level increases the entropy in any case

This comment has been minimized.

Copy link
@jglick

jglick Aug 12, 2015

Member

@oleg-nenashev agreed, which is why I think a proper solution to JENKINS-17200 is important. Not sure about the design yet. Anyway out of scope here.

@oleg-nenashev oleg-nenashev force-pushed the oleg-nenashev:JENKINS-29867-hide-envvars branch from d237fd1 to 2790255 Aug 12, 2015

@Override
public void start() throws Exception {
load();

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 12, 2015

Member

what for you call load?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 12, 2015

Author Member

Yes, no need to do it here

return null;
}

ExtensionList<EnvInjectPluginConfiguration> extensionList =

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 12, 2015

Member

can be simplified to

EnvInjectPluginConfiguration.all().get(EnvInjectPluginConfiguration.class)
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 12, 2015

@lanwen done

@lanwen

This comment has been minimized.

Copy link
Member

lanwen commented Aug 13, 2015

👍

<b>Warning!</b> If you enable this option, users may lose an access to
environment variables list. A manual reconfiguration of permissions will be
required.
</div>

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 13, 2015

Member

add new lines

@oleg-nenashev oleg-nenashev force-pushed the oleg-nenashev:JENKINS-29867-hide-envvars branch from 0a5fd20 to b424107 Aug 13, 2015
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 13, 2015

@KostyaSha done & squashed

@KostyaSha

This comment has been minimized.

Copy link
Member

KostyaSha commented Aug 13, 2015

👍

@oleg-nenashev oleg-nenashev changed the title [JENKINS-29867] - Permissions engine + global option for disabling te Injected vars [JENKINS-29867] - Permissions engine + global option for disabling the Injected vars Aug 13, 2015

@Override
public void start() throws Exception {
VIEW_INJECTED_VARS.setEnabled(getConfiguration().isEnablePermissions());

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

BTW you could just add an @Initializer to EnvInjectPluginConfiguration and then kill this whole class.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Author Member

I still prefer to keep permissions in the XYZPlugin class instead of its configuration

VIEW_INJECTED_VARS.setEnabled(getConfiguration().isEnablePermissions());
}

//TODO: Delete after upgrading the core dependency

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

Polite to note the minimum version in such comments, for easy grepping when doing an upgrade.

@Override
protected XmlFile getConfigFile() {
return new XmlFile(Jenkins.XSTREAM, new File(getJenkinsInstance().getRootDir(),
"envinject-plugin-configuration.xml"));

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

Unnecessary override.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Author Member

The plugin declares envinject.xml in EnvInjectNodeProperty (which is a problematic approach BTW), so I finally decided to create a new config file. Probably GlobalConfiguration sets another default value

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

It does.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2015

Author Member

IIRC @lanwen was against the approach with a full package/class name. I also agree with it since we have "envinject.xml". It makes sense to merge configurations to a common name later in envinject-2.0

public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
final boolean newEnablePermissions = json.getBoolean("enablePermissions");
final boolean newHideInjectedVars = json.getBoolean("hideInjectedVars");
return configure(newHideInjectedVars, newEnablePermissions);

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

Probably suffices to

req.bindJSON(this,json);

(adding save() to setters).

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 13, 2015

Member

Where do you see setters?

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 13, 2015

Member

bindJSON unreally difficult to debug as it contains many hacks.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Author Member

bindJSON() didn't work for me, and I had to use a straightforward approach (may depend on the Jenkins core version)

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

bindJSON() didn't work for me

Probably because you forgot the setters.

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

Just because of bindJSON you must create setters with save() in each.

This is proper style for a configuration object anyway. Useful from Groovy scripting, etc.

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Aug 13, 2015

Member

and then synchronise everything, useless complexity for 2 fields.

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

Synchronization would be overkill.

I do not really care how you choose to implement, just noting the normal pattern used in contemporary GlobalConfiguration subclasses FYI.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2015

Author Member

Probably it's version-dependant. I'm against setters, so let's leave the implementation as is till we bump the core dependency.

<f:checkbox checked="${it.enablePermissions}"/>
</f:entry>
<f:entry field="hideInjectedVars" title="${%hideInjectedVars.title}">
<f:checkbox checked="${it.hideInjectedVars}"/>

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

checked unnecessary here—the current value is loaded according to field.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 14, 2015

Author Member

IIRC there were issues in old Jenkins cores. BTW, I can check & cleanup it

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2015

Author Member

Yes, works well wit the current baseline

if (initialContext != null) {
SecurityContextHolder.setContext(initialContext);
}
}

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

BTW can be written more concisely with an effectively final variable:

Authentication auth = user.impersonate();
SecurityContext initialContext = ACL.impersonate(auth);
try {
    return EnvInjectPlugin.canViewInjectedVars(run);
} finally {
    SecurityContextHolder.setContext(initialContext);
}

(There are also two newer overloads with closures, but neither are suitable for Java 8 lambdas.)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2015

Author Member

fixed it

@jglick

This comment has been minimized.

Copy link
Member

jglick commented Aug 13, 2015

🐝

}

@DataBoundConstructor
public EnvInjectPluginConfiguration(boolean hideInjectedVars, boolean enablePermissions) {

This comment has been minimized.

Copy link
@jglick

jglick Aug 13, 2015

Member

AFAIK this will never be called and should be removed. It is not a Describable that may be instantiated, it is a singleton.

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 13, 2015

Member

and how works unmarshalling then? (and what about fact that GlobalConfig is Describable?)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 14, 2015

Author Member

unmarshalling works via load(), which is being called by the constructor during the singleton instatination. Seems DatatBoundConstructor should be removed

This comment has been minimized.

Copy link
@jglick

jglick Aug 14, 2015

Member

what about fact that GlobalConfig is Describable?

I guess it needs an associated Descriptor for display purposes.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 14, 2015

retriggering the build to get the artifact

@oleg-nenashev oleg-nenashev force-pushed the oleg-nenashev:JENKINS-29867-hide-envvars branch from b424107 to 40a2c53 Aug 16, 2015
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 16, 2015

@reviewbybees done
I've fixed the minor code quality issues discovered by @jglick + commented on the config location topic. Since I had a bee from him before that, I'm using my maintainer's role to go forward and to release the 0.92-beta-1 version. There will be a short timeframe before the full release in order to collect the feedback, so I can process additional feedback if it appears.

oleg-nenashev added a commit that referenced this pull request Aug 16, 2015
[JENKINS-29867] - Permissions engine + global option for disabling the Injected vars
@oleg-nenashev oleg-nenashev merged commit cff077f into jenkinsci:master Aug 16, 2015
1 check passed
1 check passed
Jenkins This pull request looks good
Details
@recena

This comment has been minimized.

Copy link
Contributor

recena commented Aug 17, 2015

@oleg-nenashev You should add your info in the developers section (pom.file) if you are going to be maintainer.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

oleg-nenashev commented Aug 17, 2015

@recena done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.