Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use only Subject to check permissions, not SecurityManager #58

Closed
wants to merge 1 commit into from

2 participants

Carl-Eric Menzel Martin Grigorov
Carl-Eric Menzel

I changed the auth strategy to only use the Subject to check permissions - asking the SecurityManager itself is not necessary.

By using only the Subject testing is simplified, because only the subject needs to be mocked.

This is the fix for the 1.4 branch.

Martin Grigorov
Owner

There are a lot of formatting noise which makes the code review very hard.

Martin Grigorov
Owner

Closing this PR as outdated.
Please open a new one against master branch if the improvement is still applicable.

Martin Grigorov martin-g closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 1, 2011
  1. Don't need SecurityManager to check Permissions, Subject is enough. T…

    Carl-Eric Menzel authored
    …his simplifies testing.
This page is out of date. Refresh to see the latest.
173 ...y/wicket-shiro/src/main/java/org/wicketstuff/shiro/annotation/AnnotationsShiroAuthorizationStrategy.java
View
@@ -19,9 +19,7 @@
import java.lang.annotation.Annotation;
import org.apache.shiro.SecurityUtils;
-import org.apache.shiro.mgt.SecurityManager;
import org.apache.shiro.subject.Subject;
-import org.apache.shiro.util.ThreadContext;
import org.apache.wicket.Component;
import org.apache.wicket.authorization.Action;
import org.apache.wicket.authorization.IAuthorizationStrategy;
@@ -29,100 +27,99 @@
import org.slf4j.LoggerFactory;
import org.wicketstuff.shiro.ShiroAction;
-public class AnnotationsShiroAuthorizationStrategy implements IAuthorizationStrategy
-{
- private static final Logger LOG = LoggerFactory.getLogger(AnnotationsShiroAuthorizationStrategy.class);
+public class AnnotationsShiroAuthorizationStrategy implements IAuthorizationStrategy {
+ private static final Logger LOG = LoggerFactory
+ .getLogger(AnnotationsShiroAuthorizationStrategy.class);
- /**
- * @param <T>
- * @param clazz
- * @return null if ok, or the Annotation that failed
- */
- protected ShiroSecurityConstraint checkInvalidInstantiation(final Annotation[] annotations,
- final ShiroAction action)
- {
- if (annotations == null)
- return null;
+ /**
+ * @param <T>
+ * @param clazz
+ * @return null if ok, or the Annotation that failed
+ */
+ protected ShiroSecurityConstraint checkInvalidInstantiation(final Annotation[] annotations,
+ final ShiroAction action) {
+ if (annotations == null) {
+ return null;
+ }
- for (final Annotation annotation : annotations)
- // Check Permissions
- if (annotation instanceof ShiroSecurityConstraint)
- {
- final ShiroSecurityConstraint constraint = (ShiroSecurityConstraint)annotation;
- if (action == constraint.action())
- {
- final SecurityManager sm = ThreadContext.getSecurityManager();
- final Subject subject = SecurityUtils.getSubject();
- switch (constraint.constraint())
- {
- case HasRole : {
- if (!sm.hasRole(subject.getPrincipals(), constraint.value()))
- return constraint;
- break;
- }
+ for (final Annotation annotation : annotations) {
+ // Check Permissions
+ if (annotation instanceof ShiroSecurityConstraint) {
+ final ShiroSecurityConstraint constraint = (ShiroSecurityConstraint) annotation;
+ if (action == constraint.action()) {
+ final Subject subject = SecurityUtils.getSubject();
+ switch (constraint.constraint()) {
+ case HasRole: {
+ if (!subject.hasRole(constraint.value())) {
+ return constraint;
+ }
+ break;
+ }
- case HasPermission : {
- if (!sm.isPermitted(subject.getPrincipals(), constraint.value()))
- return constraint;
- break;
- }
+ case HasPermission: {
+ if (!subject.isPermitted(constraint.value())) {
+ return constraint;
+ }
+ break;
+ }
- case IsAuthenticated : {
- if (!subject.isAuthenticated())
- return constraint;
- break;
- }
+ case IsAuthenticated: {
+ if (!subject.isAuthenticated()) {
+ return constraint;
+ }
+ break;
+ }
- case LoggedIn : {
- if (subject.getPrincipal() == null)
- return constraint;
- break;
- }
- }
- }
- } // end if KiSecurityConstraint
- return null;
- }
+ case LoggedIn: {
+ if (subject.getPrincipal() == null) {
+ return constraint;
+ }
+ break;
+ }
+ }
+ }
+ } // end if KiSecurityConstraint
+ }
+ return null;
+ }
- public <T extends Component> ShiroSecurityConstraint checkInvalidInstantiation(
- final Class<T> componentClass)
- {
- ShiroSecurityConstraint fail = checkInvalidInstantiation(componentClass.getAnnotations(),
- ShiroAction.INSTANTIATE);
- if (fail == null)
- fail = checkInvalidInstantiation(componentClass.getPackage().getAnnotations(),
- ShiroAction.INSTANTIATE);
- return fail;
- }
+ public <T extends Component> ShiroSecurityConstraint checkInvalidInstantiation(
+ final Class<T> componentClass) {
+ ShiroSecurityConstraint fail = checkInvalidInstantiation(componentClass.getAnnotations(),
+ ShiroAction.INSTANTIATE);
+ if (fail == null) {
+ fail = checkInvalidInstantiation(componentClass.getPackage().getAnnotations(),
+ ShiroAction.INSTANTIATE);
+ }
+ return fail;
+ }
- /**
- * {@inheritDoc}
- */
- public boolean isActionAuthorized(final Component component, final Action action)
- {
+ /**
+ * {@inheritDoc}
+ */
+ public boolean isActionAuthorized(final Component component, final Action action) {
- final ShiroAction _action = action.getName().equals(Action.RENDER) ? ShiroAction.RENDER
- : ShiroAction.ENABLE;
+ final ShiroAction _action = action.getName().equals(Action.RENDER) ? ShiroAction.RENDER
+ : ShiroAction.ENABLE;
- final Class<? extends Component> clazz = component.getClass();
- ShiroSecurityConstraint fail = checkInvalidInstantiation(clazz.getAnnotations(), _action);
- if (fail == null)
- fail = checkInvalidInstantiation(clazz.getPackage().getAnnotations(), _action);
- return fail == null;
- }
+ final Class<? extends Component> clazz = component.getClass();
+ ShiroSecurityConstraint fail = checkInvalidInstantiation(clazz.getAnnotations(), _action);
+ if (fail == null) {
+ fail = checkInvalidInstantiation(clazz.getPackage().getAnnotations(), _action);
+ }
+ return fail == null;
+ }
- /**
- * {@inheritDoc}
- */
- public <T extends Component> boolean isInstantiationAuthorized(final Class<T> componentClass)
- {
- final Annotation fail = checkInvalidInstantiation(componentClass);
- if (fail != null)
- {
- LOG.info("Unauthorized Instantiation :: component={} reason={} subject={}",
- new Object[] { componentClass, fail, SecurityUtils.getSubject() });
- return false;
- }
- return true;
- }
+ /**
+ * {@inheritDoc}
+ */
+ public <T extends Component> boolean isInstantiationAuthorized(final Class<T> componentClass) {
+ final Annotation fail = checkInvalidInstantiation(componentClass);
+ if (fail != null) {
+ LOG.info("Unauthorized Instantiation :: component={} reason={} subject={}",
+ new Object[] { componentClass, fail, SecurityUtils.getSubject() });
+ return false;
+ }
+ return true;
+ }
}
Something went wrong with that request. Please try again.