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

Login Form Target URL defined with Environment Variable #12

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

fpmosley
Copy link

When configuring Form-based Authentication, a user is unable to specify an environment variable in the Login Form Target URL field. This pull request allows an environment variable to be specified and expanded in that field.

This is a fix for JENKINS-47038.

@@ -557,6 +557,10 @@ private void checkParams(AbstractBuild<?, ?> build, BuildListener listener) thro
if (this.evaluatedTargetURL == null || this.evaluatedTargetURL.isEmpty()) throw new IllegalArgumentException("STARTING POINT (URL) IS MISSING, PROVIDED [ " + this.evaluatedTargetURL + " ]");
else Utils.loggerMessage(listener, 1, "(EXP) STARTING POINT (URL) = [ {0} ]", this.evaluatedTargetURL);

this.evaluatedLoginURL = envVars.expand(this.evaluatedLoginURL);
if (this.evaluatedLoginURL == null || this.evaluatedLoginURL.isEmpty()) throw new IllegalArgumentException("LOGIN FORM TARGET (URL) IS MISSING, PROVIDED [ " + this.evaluatedLoginURL + " ]");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This validation should be performed only if authMode is true, otherwise it requires a login URL even if no authentication is enabled.

@JordanGS
Copy link
Member

JordanGS commented Mar 24, 2018

Looking over this pull request during the week. There was definitely a reason this was not a feature during initial development, will update once i go over my notes.

Please squash the commits into one.

@akennealy
Copy link

Any update on this PR? We're currently blocked on this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants