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

[FIXED JENKINS-32844] Options under the drop-down menu at account name in the top bar doesn't work correctly when it contains a slash #2020

Closed
wants to merge 1 commit into from

Conversation

fbelzunc
Copy link
Contributor

@fbelzunc fbelzunc commented Feb 8, 2016

To re-produce:

  1. Install a fresh Jenkins instance with latest bits
  2. Install mock security realm plugin and configure it with an user containing a slash (foo/foo)
  3. log-in
  4. Options under the account name are not displayed and when clicking in the user link you get a HTTP ERROR 404.

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

@reviewbybees

@ghost
Copy link

ghost commented Feb 8, 2016

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.

@daniel-beck
Copy link
Member

Looks like a legitimate test failure.

@daniel-beck daniel-beck added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Feb 9, 2016
@oleg-nenashev
Copy link
Member

@fbelzunc
I doubt this fix resolves all issues with users having slashes. I doubt we really want to support this use-case. Is there any realistic use-case of slashes in user names

@fbelzunc
Copy link
Contributor Author

fbelzunc commented Feb 9, 2016

@daniel-beck Right, my fault for not checking the test on mi side before submitting the pull request. Sorry for your time.

@oleg-nenashev Answering you in-line:

I doubt this fix resolves all issues with users having slashes.

Right, nobody is saying the opposite. I am trying to fix what is explained in JENKINS-32844.

I doubt we really want to support this use-case.

Well, this is something I let the community to decide. My understanding is that as long as RFC 2253 allows forward slash Jenkins should manage this use-case as well.

Is there any realistic use-case of slashes in user names

As long as it is a possibility per RFC 2253 I guess it might be a real use case.

@daniel-beck
Copy link
Member

@fbelzunc No problem, just FYI this isn't a false positive test failure, but needs fixing.

@fbelzunc
Copy link
Contributor Author

@daniel-beck all those tests pass successfully on my laptop. I am closing and re-opening the pull request to re-build

@fbelzunc fbelzunc closed this Feb 11, 2016
@fbelzunc fbelzunc reopened this Feb 11, 2016
@jtnord
Copy link
Member

jtnord commented Feb 18, 2016

Windows usernames will very likely contain a backslash in a multi domain environment DOMAIN1\User and DOMAIN2\User are different users.

@@ -188,7 +188,7 @@ ${h.initPageVariables(context)}
</j:otherwise>
</j:choose>
<span style="white-space:nowrap">
<a href="${rootURL}/user/${user.id}" class="model-link inside inverse"><b>${userName}</b></a>
<a href="${rootURL}/user/me" class="model-link inside inverse"><b>${userName}</b></a>
Copy link
Member

Choose a reason for hiding this comment

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

@fbelzunc What are we expecting to happen with this change? Does "me" resolve to something special on the server-side?

Copy link
Member

Choose a reason for hiding this comment

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

🐛 the url is just ${rootURL}/me

@jtnord
Copy link
Member

jtnord commented Mar 3, 2016

🐛 the url to the current users stuff is just ${rootURL}/me not ${rootURL}/users/me which would actually be the user with the id of "me"

@fbelzunc fbelzunc closed this Mar 7, 2016
@fbelzunc fbelzunc reopened this Mar 7, 2016
@fbelzunc
Copy link
Contributor Author

fbelzunc commented Mar 8, 2016

@daniel-beck @jtnord Now, this is working correctly.

@armfergom
Copy link

LGTM 🐝

@fbelzunc
Copy link
Contributor Author

fbelzunc commented Mar 8, 2016

Squashing to have only 1 commit for cleaning merger purposes

@fbelzunc fbelzunc closed this Mar 8, 2016
@fbelzunc fbelzunc reopened this Mar 8, 2016
@jtnord
Copy link
Member

jtnord commented Mar 8, 2016

@felix - this is only a partial fix. How are users going to get to see other users configurations?

@fbelzunc
Copy link
Contributor Author

fbelzunc commented Mar 8, 2016

@jtnord

  • this is only a partial fix. How are users going to get to see other users configurations?

You are right - it is only a partial fix. Here, I am only trying to fix JENKINS-32844: Options under the drop-down menu at account name in the top bar doesn't work correctly when it contains a slash. Notice, that some of the options on the dropdown menu are also available on the left-menu and they carry you to /me, that's why I think there is nothing wrong with this pull request.

I agree that the "real" fix requires some investigation and time ;-)

@jtnord
Copy link
Member

jtnord commented Mar 8, 2016

@fbelzunc well when I tested this later on when you hit configure etc things don't work so you are unable to do much with this fix :-/

@fbelzunc fbelzunc closed this Mar 9, 2016
@fbelzunc fbelzunc reopened this Mar 9, 2016
@fbelzunc fbelzunc closed this Mar 10, 2016
@fbelzunc fbelzunc reopened this Mar 10, 2016
@fbelzunc
Copy link
Contributor Author

Closing this PR

@fbelzunc fbelzunc closed this May 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging
Projects
None yet
6 participants