[WIP][JENKINS-58293] Folder based Authorization as a new AuthorizationStrategy #89

AbhyudayaSharma · 2019-07-02T12:37:28Z

Unlike #87 this adds a new @Extension with Authorization Strategy

This is a work in progress pull request

AbhyudayaSharma · 2019-07-03T07:18:28Z

src/main/java/io/jenkins/plugins/rolestrategy/GlobalAclImpl.java

+     *
+     * @param globalRoles set of roles from which to calculate the permissions.
+     */
+    GlobalAclImpl(Set<GlobalRole> globalRoles) {


This is a new implementation for global roles. When the object is created, it pre-computes all permissions available to the sid by going through all of the roles.

Would it make sense to do it in a separate pull request? From what I see, the existing strategy could benefit from it as well

AbhyudayaSharma · 2019-07-03T07:21:21Z

src/main/java/io/jenkins/plugins/rolestrategy/GlobalAclImpl.java

+        for (GlobalRole role : globalRoles) {
+            Set<Permission> impliedPermissions = ConcurrentHashMap.newKeySet();
+
+            role.permissions.parallelStream()


I'm using parallel streams here and a thread-safe set. Please let me know if I need to revert back to single-threaded streams.

AbhyudayaSharma · 2019-07-03T07:23:30Z

src/main/java/io/jenkins/plugins/rolestrategy/GlobalAclImpl.java

+    protected Boolean hasPermission(Sid p, Permission permission) {
+        Set<Permission> permissions = permissionList.get(toString(p));
+        if (permission != null && permissions.contains(permission)) {
+            return true;


hasPermission should now run in constant time (O(1)) because getting a value from a map and checking if something is contained in a set are both O(1) operations.

The POST request is currently not handled properly

src/main/java/io/jenkins/plugins/rolestrategy/GlobalRoleCreationRequest.java

AbhyudayaSharma · 2019-07-04T11:44:57Z

src/main/webapp/js/folder-auth/globalrole.js

+
+    // this is really bad.
+    // See https://github.com/jenkinsci/jenkins/blob/75468da366c1d257a51655dcbe952d55b8aeeb9c/war/src/main/js/util/jenkins.js#L22
+    const oldPrototype = Array.prototype.toJSON;


I've implemented the same fix in the link
What would be the URL to import and use the file directly?

oleg-nenashev · 2019-07-04T22:52:41Z

src/main/java/io/jenkins/plugins/rolestrategy/GlobalAclImpl.java

+     *
+     * @param globalRoles set of roles from which to calculate the permissions.
+     */
+    GlobalAclImpl(Set<GlobalRole> globalRoles) {


Would it make sense to do it in a separate pull request? From what I see, the existing strategy could benefit from it as well

pom.xml

oleg-nenashev · 2019-07-04T22:56:27Z

src/main/java/io/jenkins/plugins/rolestrategy/FolderAuthorizationStrategyManagementLink.java

+    @CheckForNull
+    @Override
+    public String getDisplayName() {
+        return "Folder Authorization Strategy";


Would be nice to make it localizable for the final implementation

src/main/java/io/jenkins/plugins/rolestrategy/GlobalRoleCreationRequest.java

oleg-nenashev

Maybe it even makes sense to consider having a separate plugin for the new strategy.
But the code can be split later if there is such decision.

CC @runzexia @Supun94, reviews are needed

oleg-nenashev · 2019-07-04T22:59:54Z

src/main/java/io/jenkins/plugins/rolestrategy/FolderBasedAuthorizationStrategy.java

+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+@ParametersAreNonnullByDefault


IIRC it is not going to play well for the transient field. SportBugs/FindBugs are not equipped to resolve such deserialization cases IIRC

oleg-nenashev · 2019-07-04T23:00:42Z

src/test/java/io/jenkins/plugins/rolestrategy/GlobalAclImplTest.java

+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class GlobalAclImplTest {


JCasC compatibility autotest here would be really interesting

* Also remove `forEach` from JavaScript as it is not compatible with old browsers * Make the dependency on Cloudbees folder plugin optional * Update Jelly UI * Make GlobalRoleCreationRequest `@Restricted(NoExternalUse.class)`

* Refactor roles and ACLs into their own packages * Update the Jelly UI a bit

AbhyudayaSharma · 2019-07-08T09:10:34Z

The test fails because ConcurrentHashMap.newKeySet was refused to be marshalled:

Caused by: java.lang.UnsupportedOperationException: Refusing to marshal java.util.concurrent.ConcurrentHashMap$KeySetView for security reasons; see https://jenkins.io/redirect/class-filter/
[2019-07-08T08:11:12.508Z] 	at hudson.util.XStream2$BlacklistedTypesConverter.marshal(XStream2.java:546)
[2019-07-08T08:11:12.508Z] 	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
[2019-07-08T08:11:12.508Z] 	at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
[2019-07-08T08:11:12.508Z] 	at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
[2019-07-08T08:11:12.508Z] 	at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
[2019-07-08T08:11:12.508Z] 	at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
[2019-07-08T08:11:12.508Z] 	... 38 more

AbhyudayaSharma · 2019-07-08T09:30:28Z

Would it be fine to create a pull request to Jenkins core and add java.util.concurrent.ConcurrentHashMap$KeySetView here? Otherwise, there's also the ConcurrentSkipListMap that could've been used but it is not whitelisted either.

oleg-nenashev · 2019-07-08T19:36:50Z

Would it be fine to create a pull request to Jenkins core and add java.util.concurrent.ConcurrentHashMap$KeySetView here? Otherwise, there's also the ConcurrentSkipListMap that could've been used but it is not whitelisted either.

You can whitelist it for the plugin .
https://jenkins.io/blog/2018/01/13/jep-200/#making-plugins-compatible-with-jenkins-2-102-or-above

* Also add docs to globalrole.js

src/main/java/io/jenkins/plugins/rolestrategy/roles/AbstractRole.java

src/main/java/io/jenkins/plugins/rolestrategy/FolderAuthorizationStrategyManagementLink.java

* Also make AbstractRole `@Restricted(NoExternalUse.class)` * Add some JavaDoc

src/main/java/io/jenkins/plugins/rolestrategy/FolderAuthorizationStrategyManagementLink.java

jglick · 2019-07-09T14:28:04Z

src/main/resources/META-INF/hudson.remoting.ClassFilter

@@ -0,0 +1 @@
+java.util.concurrent.ConcurrentHashMap$KeySetView


Bad idea. Fix your code to store a simpler type instead, like an ArrayList.

Hi @jglick ! I had wanted to use a Set to avoid duplicates and for faster remove() operations. Also since there were tickets for concurrency issues in the Role Strategy Plugin, this seemed like the perfect option. Could you please let me know why you think this is a bad idea? Also as an alternative, would you approve of CopyOnWriteArraySet? remove()s will be slower but we'll get thread safety and avoid duplicates.

Add a writeReplace() method to convert the class to a serialization proxy, then inside that proxy class create a readResolve() method to convert the proxy class back to the original class. I've come across this exact same whitelisting issue twice so far in the past year.

Edit: here's an example of how I worked around this issue: https://github.com/jenkinsci/support-core-plugin/blob/master/src/main/java/com/cloudbees/jenkins/support/filter/ContentMappings.java

faster remove() operations

This smells wrong. How often are you modifying this set anyway? If it is being persisted in XStream, then any collection overhead would be orders of magnitude less than disk I/O when the result is saved.

Could you please let me know why you think this is a bad idea?

Data stored by a plugin should use simple types. There are good reasons to create a custom ClassFilter whitelist in a plugin; this is not one of them.

http://www.jguru.com/faq/view.jsp?EID=44039 ftr

* Use the CasC incremental * Use AbstractFolders instead of Folders

oleg-nenashev · 2019-07-10T15:16:26Z

https://groups.google.com/forum/#!topic/jenkinsci-dev/WccushweIlw

AbhyudayaSharma · 2019-07-12T02:53:19Z

Unrelated build failure:

[2019-07-11T17:38:10.998Z] win2012-cd60a0 was marked offline: Connection was broken: java.util.concurrent.TimeoutException: Ping started at 1562866450996 hasn't completed by 1562866690996
[2019-07-11T17:38:10.998Z] 	at hudson.remoting.PingThread.ping(PingThread.java:134)
[2019-07-11T17:38:10.998Z] 	at hudson.remoting.PingThread.run(PingThread.java:90)

AbhyudayaSharma · 2019-07-12T07:54:13Z

Hi everyone! We decided to move this code to a new repository: https://github.com/jenkinsci/folder-auth-plugin . For the issues above, I'm creating new JIRA tickets. I would love to hear your feedback there. Thanks!

[WIP] Folder based Authorization as a new AuthorizationStrategy

7ea9bad

AbhyudayaSharma added the work-in-progress label Jul 2, 2019

AbhyudayaSharma requested a review from a team July 2, 2019 12:37

AbhyudayaSharma self-assigned this Jul 2, 2019

Add GlobalRole implementation for folders

a02ea97

AbhyudayaSharma commented Jul 3, 2019

View reviewed changes

Fix GlobalAclImpl's behaviour and add unit test for it

15d7222

AbhyudayaSharma force-pushed the folder-auth-strategy branch from 7274648 to 15d7222 Compare July 3, 2019 07:58

AbhyudayaSharma added 3 commits July 3, 2019 16:47

Add a basic Jelly view to add global role from browser

82de631

The POST request is currently not handled properly

Adding global roles now works from the Web UI

b70972a

Cleanup code

1b35cd1

AbhyudayaSharma commented Jul 4, 2019

View reviewed changes

Fix Findbugs warnings and improve the UI

b3e97b2

oleg-nenashev reviewed Jul 4, 2019

View reviewed changes

oleg-nenashev requested review from runzexia and Supun94 July 4, 2019 23:04

AbhyudayaSharma added 9 commits July 5, 2019 12:20

Allow assigning sids to global roles from web UI

5043e0c

* Also remove `forEach` from JavaScript as it is not compatible with old browsers * Make the dependency on Cloudbees folder plugin optional * Update Jelly UI * Make GlobalRoleCreationRequest `@Restricted(NoExternalUse.class)`

Add a wrapper for Permission to make config.xml smaller

0abbc87

Git ignore .factorypath

cdd82e1

Fix GlobalAclImplTest and add utility method to wrap up Permissions

cee8c32

Implement basic folder authorization

4a61f8b

* Refactor roles and ACLs into their own packages * Update the Jelly UI a bit

Allow adding FolderRoles from web UI

e9babe7

Fix JavaDoc

e3bdf1f

Allow assigning FolderRoles to Sids from Web UI

a518384

Add a unit test for FolderBasedAuthorizationStrategy

c11e7ae

AbhyudayaSharma added 2 commits July 8, 2019 17:04

Use JavaScript to submit 'Add folder role' form

359ef9f

Make Jelly UI look less bad

a5deb46

AbhyudayaSharma added 4 commits July 9, 2019 09:13

Whitelist java.util.concurrent.ConcurrentHashMap$KeySetView

2853ca5

Allow deleting global and folder roles

20f6657

* Also add docs to globalrole.js

Add administrator checks to deleting roles

5fe72ef

Cache ACLs that contain inheritance data

7e18738

oleg-nenashev reviewed Jul 9, 2019

View reviewed changes

src/main/java/io/jenkins/plugins/rolestrategy/roles/AbstractRole.java Show resolved Hide resolved

src/main/java/io/jenkins/plugins/rolestrategy/FolderAuthorizationStrategyManagementLink.java Show resolved Hide resolved

AbhyudayaSharma changed the title ~~[WIP] Folder based Authorization as a new AuthorizationStrategy~~ [WIP][JENKINS-58293] Folder based Authorization as a new AuthorizationStrategy Jul 9, 2019

AbhyudayaSharma added 2 commits July 9, 2019 15:51

Throw errors instead of not doing anything

73956b6

* Also make AbstractRole `@Restricted(NoExternalUse.class)` * Add some JavaDoc

Disallow dangerous permissions

bf50316

AbhyudayaSharma force-pushed the folder-auth-strategy branch from ff05362 to bf50316 Compare July 9, 2019 12:39

jglick reviewed Jul 9, 2019

View reviewed changes

AbhyudayaSharma mentioned this pull request Jul 10, 2019

Allow DataBoundConfigurator to convert and export Sets jenkinsci/configuration-as-code-plugin#943

Merged

5 tasks

Add Support for Configuration-as-Code

ce4ada2

* Use the CasC incremental * Use AbstractFolders instead of Folders

oleg-nenashev requested a review from daniel-beck July 10, 2019 15:14

AbhyudayaSharma closed this Jul 12, 2019

This was referenced Jul 12, 2019

[WIP] Add Folder based authorization #87

Closed

[JENKINS-58485] Do not serialize ConcurrentHashMap$KeySetView jenkinsci/folder-auth-plugin#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][JENKINS-58293] Folder based Authorization as a new AuthorizationStrategy #89

[WIP][JENKINS-58293] Folder based Authorization as a new AuthorizationStrategy #89

AbhyudayaSharma commented Jul 2, 2019

AbhyudayaSharma Jul 3, 2019

oleg-nenashev Jul 4, 2019

AbhyudayaSharma Jul 3, 2019

AbhyudayaSharma Jul 3, 2019

AbhyudayaSharma Jul 4, 2019

oleg-nenashev Jul 4, 2019

oleg-nenashev Jul 4, 2019

oleg-nenashev left a comment

oleg-nenashev Jul 4, 2019

oleg-nenashev Jul 4, 2019

AbhyudayaSharma commented Jul 8, 2019 •

edited

AbhyudayaSharma commented Jul 8, 2019

oleg-nenashev commented Jul 8, 2019

jglick Jul 9, 2019

AbhyudayaSharma Jul 9, 2019

jvz Jul 9, 2019 •

edited

jglick Jul 9, 2019

oleg-nenashev Jul 12, 2019

oleg-nenashev commented Jul 10, 2019

AbhyudayaSharma commented Jul 12, 2019

AbhyudayaSharma commented Jul 12, 2019

		@@ -0,0 +1 @@
		java.util.concurrent.ConcurrentHashMap$KeySetView

[WIP][JENKINS-58293] Folder based Authorization as a new AuthorizationStrategy #89

[WIP][JENKINS-58293] Folder based Authorization as a new AuthorizationStrategy #89

Conversation

AbhyudayaSharma commented Jul 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhyudayaSharma commented Jul 8, 2019 • edited

AbhyudayaSharma commented Jul 8, 2019

oleg-nenashev commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvz Jul 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Jul 10, 2019

AbhyudayaSharma commented Jul 12, 2019

AbhyudayaSharma commented Jul 12, 2019

AbhyudayaSharma commented Jul 8, 2019 •

edited

jvz Jul 9, 2019 •

edited