-
Notifications
You must be signed in to change notification settings - Fork 24
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-62443 Add Full Name lookup for SIDs #59
base: master
Are you sure you want to change the base?
Conversation
StringBuilder sb = new StringBuilder(); | ||
for (String sid: new TreeSet<>(sids)) { | ||
try { | ||
sr.loadUserByUsername(sid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a large number of sids this looks like it can be potentially very slow (if it requires and LDAP lookup). Which IIUC will block the page from loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Also, this logi will lead to exceptions when handling group membership SIDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look at it again once #58 is done
I don’t think you need to keep the ID just show the full name of its set, have a look at what it looks like when you use the matrix authorization strategy from the matrix-auth plugin |
Relevant code from matrix-auth (shouldn't be blindly copied, likely lots of legacy there): |
RoleStrategy uses the same method from |
Sure, it makes more sense. I guess I'll have to find something else to work on in the mean time. |
I was wondering about that myself but It might be useful to have some reference to the actual SID in case you have identical full names. |
best to be consistent here I think, it saves space and looks nicer, |
A simple fix to add full name for SID. It looks like this