You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Kevin Sweeney <ke...@apache.org> on 2015/04/21 23:37:06 UTC
Review Request 33411: Add typed Shiro permissions SPI.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/
-----------------------------------------------------------
Review request for Aurora, Joshua Cohen and Bill Farner.
Repository: aurora
Description
-------
Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
Diffs
-----
config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
Diff: https://reviews.apache.org/r/33411/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 33411: Add typed Shiro permissions SPI.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/#review81082
-----------------------------------------------------------
Ship it!
Master (d10d2d1) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On April 21, 2015, 9:37 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33411/
> -----------------------------------------------------------
>
> (Updated April 21, 2015, 9:37 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
>
> To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
>
>
> Diffs
> -----
>
> config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
> src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
> src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
>
> Diff: https://reviews.apache.org/r/33411/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 33411: Add typed Shiro permissions SPI.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/#review81174
-----------------------------------------------------------
Ship it!
Ship It!
- Joshua Cohen
On April 21, 2015, 11 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33411/
> -----------------------------------------------------------
>
> (Updated April 21, 2015, 11 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
>
> To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
>
>
> Diffs
> -----
>
> config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
> src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
> src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
>
> Diff: https://reviews.apache.org/r/33411/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 33411: Add typed Shiro permissions SPI.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/#review81088
-----------------------------------------------------------
Ship it!
Master (8ba1b11) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On April 21, 2015, 11 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33411/
> -----------------------------------------------------------
>
> (Updated April 21, 2015, 11 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
>
> To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
>
>
> Diffs
> -----
>
> config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
> src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
> src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
>
> Diff: https://reviews.apache.org/r/33411/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 33411: Add typed Shiro permissions SPI.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/
-----------------------------------------------------------
(Updated April 21, 2015, 4 p.m.)
Review request for Aurora, Joshua Cohen and Bill Farner.
Changes
-------
Josh's feedback.
Repository: aurora
Description
-------
Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
Diffs (updated)
-----
config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
Diff: https://reviews.apache.org/r/33411/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 33411: Add typed Shiro permissions SPI.
Posted by Kevin Sweeney <ke...@apache.org>.
> On April 21, 2015, 3:27 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java, line 60
> > <https://reviews.apache.org/r/33411/diff/1/?file=938796#file938796line60>
> >
> > Is this used anywhere now?
Nope, removed.
> On April 21, 2015, 3:27 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java, line 65
> > <https://reviews.apache.org/r/33411/diff/1/?file=938796#file938796line65>
> >
> > Same here.
Removed.
> On April 21, 2015, 3:27 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/spi/Permissions.java, line 86
> > <https://reviews.apache.org/r/33411/diff/1/?file=938799#file938799line86>
> >
> > s/org.apache.aurora.scheduler.spi.Permissions//
Done.
> On April 21, 2015, 3:27 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/spi/Permissions.java, line 93
> > <https://reviews.apache.org/r/33411/diff/1/?file=938799#file938799line93>
> >
> > Can we just use Domain.values() and save on creating the EnumSet?
Fixed.
> On April 21, 2015, 3:27 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/spi/Permissions.java, lines 130-133
> > <https://reviews.apache.org/r/33411/diff/1/?file=938799#file938799line130>
> >
> > Fits on one line?
Yep.
> On April 21, 2015, 3:27 p.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java, line 55
> > <https://reviews.apache.org/r/33411/diff/1/?file=938801#file938801line55>
> >
> > Any reason not to skip the indirection and use THRIFT_AURORA_ADMIN directly?
DOMAIN is used in assertions later, the constant is here for DRYness.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/#review81076
-----------------------------------------------------------
On April 21, 2015, 2:37 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33411/
> -----------------------------------------------------------
>
> (Updated April 21, 2015, 2:37 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
>
> To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
>
>
> Diffs
> -----
>
> config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
> src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
> src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
>
> Diff: https://reviews.apache.org/r/33411/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 33411: Add typed Shiro permissions SPI.
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33411/#review81076
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java
<https://reviews.apache.org/r/33411/#comment131302>
Is this used anywhere now?
src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java
<https://reviews.apache.org/r/33411/#comment131305>
Same here.
src/main/java/org/apache/aurora/scheduler/spi/Permissions.java
<https://reviews.apache.org/r/33411/#comment131309>
s/org.apache.aurora.scheduler.spi.Permissions//
src/main/java/org/apache/aurora/scheduler/spi/Permissions.java
<https://reviews.apache.org/r/33411/#comment131306>
Can we just use Domain.values() and save on creating the EnumSet?
src/main/java/org/apache/aurora/scheduler/spi/Permissions.java
<https://reviews.apache.org/r/33411/#comment131307>
Fits on one line?
src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java
<https://reviews.apache.org/r/33411/#comment131308>
Any reason not to skip the indirection and use THRIFT_AURORA_ADMIN directly?
- Joshua Cohen
On April 21, 2015, 9:37 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33411/
> -----------------------------------------------------------
>
> (Updated April 21, 2015, 9:37 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Working on an implementation of a Shiro Realm to replace the old CapabilityValidator I realized we're missing some information with the new API. This patch allows a Realm implementation to optionally introspect a permission check for Aurora-specific information about exactly what's being attempted, while maintaining compatibility with Shiro realms that don't know anything about Aurora, such as the included IniRealm.
>
> To do this I've added a new SPI package, and documented backwards-compatibility considerations with it. The idea is that a third party can write an Aurora-aware Shiro Realm module against a stable ABI and gets one release to update it if we want to make changes to it.
>
>
> Diffs
> -----
>
> config/findbugs/excludeFilter.xml 0bff71c33dff8c92fdf5e841c04ee1460c50937b
> src/main/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityModule.java 0265e2a1e4cad3f569501521f1bf25d7caa9da44
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptor.java 7a124cccad2e02e7c4f16c3c7fc8bd662bcb5360
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java fde6c84e297d35b3e10f12a95e12dfb7ab1b477f
> src/main/java/org/apache/aurora/scheduler/spi/Permissions.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/spi/package-info.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingInterceptorTest.java 16f2da5207ce2a3f866fa3a51c02c11d1b58a439
> src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptorTest.java 781cf5add1555187757a0254a96f201bc74a1e27
>
> Diff: https://reviews.apache.org/r/33411/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>