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

Treat user authorities as roles #13

Merged
merged 2 commits into from May 23, 2016

Conversation

Projects
None yet
3 participants
@bkmeneguello
Copy link
Contributor

bkmeneguello commented Oct 30, 2015

This is useful in combination with other plugins, like LDAP Authentication to treat groups as roles.
Also I've added two tests. One covering the basic working flow and another to check if authorities are being treated as roles.

return true;
}
}
} catch (Exception e) {}

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 30, 2015

Member

There should be logging at least

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Nov 3, 2015

Author Contributor

Sure. I just preserved the original behaviour of doing nothing when the role verification fails. But I admit that using Exception is too wide. I'll narrow it. You sure logging could be a good idea?

}
} // Default handling
else {
return true;
}
} else {
try {
UserDetails userDetails = Jenkins.getInstance().getSecurityRealm().loadUserByUsername(sid);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 30, 2015

Member

Have you evaluated the performance impact? If I understand the code correctly. it's going to cause a huge performance degradation. Caching is required IMO.

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Nov 3, 2015

Author Contributor

I really didn't. I'm using this change in my local environment for evaluation. We have 40 users actively working with Jenkins and, until now, didn't noticed any performance penalty. Our security realm is LDAPSecurityRealm without auth cache. It's performing very well.
But I agree that other security realms may perform poor. Do you suggest some attention points that can be bottlenecks? Thanks.

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Nov 3, 2015

Author Contributor

Do you think this feature should be an opt-in?

@@ -0,0 +1,48 @@
<?xml version='1.0' encoding='UTF-8'?>
<hudson>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 30, 2015

Member

Test resources should be located in src/test/resources, not in main

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Nov 3, 2015

Author Contributor

Ops.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 30, 2015

@bkmeneguello
Hi Bruno,

I'm aware about the performance impact of the change. The change is going to embedded user loading from security realms and sequential role checks for every SID missing in the group. Since it's a common case for the fine-grain security, the overall calculation time may seriously degrade.

Have you evaluated it?

pom.xml Outdated
@@ -77,6 +77,24 @@
<version>1.14</version>
<type>jar</type>
</dependency>
<dependency>

This comment has been minimized.

Copy link
@KostyaSha

KostyaSha Oct 31, 2015

Member

wrong intend

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 31, 2015

Member

It would be better to solve architectural issues before rat-holing into this topic...

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Nov 3, 2015

Author Contributor

Fixed. Thanks!

@bkmeneguello bkmeneguello force-pushed the bkmeneguello:authority-as-role branch 2 times, most recently from 49aeab5 to f95cc99 Nov 3, 2015

@bkmeneguello bkmeneguello force-pushed the bkmeneguello:authority-as-role branch from f95cc99 to 7b5a518 Nov 3, 2015

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Nov 10, 2015

@oleg-nenashev So do you had reviewed my last comments?
I'm using this changed plugin a while in my company and didn't noticed any performance issues. Whe have near 40 users and 60 JOBs with project and general roles.
Do you have any attention point that I should consider in particular?
Thanks.

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Nov 23, 2015

Ok, today I got what you mean. My server become unusable because of a new LDAP ACL that caused slowness.
I'll implement that cache.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Nov 23, 2015

@bkmeneguello
Sorry for the missing response. I'm quite busy with personal TODOs, so my responses may be seriously delayed. Don't hesitate to ping me

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Nov 24, 2015

@oleg-nenashev I've implemented the cache, a simple solution. In your opinion the cache parameters (max-size, ttl and concurrency) should be parameterized in a view? These entire feature should be an opt-in?
Thanks

@bkmeneguello bkmeneguello force-pushed the bkmeneguello:authority-as-role branch from ad084d7 to 54b04a3 Nov 24, 2015

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Jan 2, 2016

bump?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jan 2, 2016

@bkmeneguello Thanks for the reminder. I was extremely busy with some personal things, but hopefully I'll be able to process incoming PRs on this NY break


private final Cache<String, UserDetails> cache = CacheBuilder.newBuilder()
.softValues()
.maximumSize(100)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 2, 2016

Member

Needs to be a global configuration/system property. 100 is a good default value, but I can imagine installation requiring more users

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Jan 6, 2016

Author Contributor

Ok. But this value of 100 is the cache size. Just the 100 LRU entries will be cached.

try {
UserDetails userDetails = cache.getIfPresent(sid);
if (userDetails == null) {
userDetails = Jenkins.getInstance().getSecurityRealm().loadUserByUsername(sid);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 2, 2016

Member

This method may throw UserMayOrMayNotExistException. I suspect it needs to be captured & logged

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Jan 6, 2016

Author Contributor

This exception is a BadCredentialsException, captured below

} catch (BadCredentialsException e) {
LOGGER.log(Level.FINE, "Bad credentials", e);
} catch (DataAccessException e) {
LOGGER.log(Level.FINE, "failed to access the data", e);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 2, 2016

Member

Both exceptions smell like a fatal error. Or not?

This comment has been minimized.

Copy link
@bkmeneguello

bkmeneguello Jan 6, 2016

Author Contributor

Well, maybe can we discuss about this. Since this is a new behaviour, on top of previous one, if the user hasn't the appropriate roles, I've logged the exception but kept the original behaviour.
What you propose with "fatal error"?

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Feb 11, 2016

@oleg-nenashev do you agree with my last replies?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Feb 11, 2016

@bkmeneguello
Sorry, missed the responses. I hope to review it on this weekend

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Feb 11, 2016

Thanks!

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Apr 11, 2016

@oleg-nenashev pls, did you had time to review this PR?

@oleg-nenashev oleg-nenashev self-assigned this Apr 24, 2016

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Apr 24, 2016

@bkmeneguello
Sorry for the delay. I've manually tested it today, looks good to me. I hope to find some time to finally make a release on the next week

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Apr 25, 2016

Thanks!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented May 10, 2016

Looking for the issue to reference it during the merge. It exists AFAIK.
🐌

@oleg-nenashev oleg-nenashev merged commit d957f8f into jenkinsci:master May 23, 2016

1 check passed

Jenkins This pull request looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 12, 2016

Caused performance regressions, hence the feature will be partially disabled in #18

@bkmeneguello

This comment has been minimized.

Copy link
Contributor Author

bkmeneguello commented Jun 14, 2016

absolutely ok

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