From cbf63160502dcee3a7a07722d1ed22e449e42eb2 Mon Sep 17 00:00:00 2001 From: Justin Richer Date: Wed, 27 May 2015 12:06:58 -0400 Subject: [PATCH] cleaned up logic on user info interceptor to fix detection of redirects --- .../connect/web/UserInfoInterceptor.java | 77 +++++++++++-------- .../filter/AuthorizationRequestFilter.java | 4 +- 2 files changed, 46 insertions(+), 35 deletions(-) diff --git a/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java b/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java index eecb934b8..f0e55ffc4 100644 --- a/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java +++ b/openid-connect-common/src/main/java/org/mitre/openid/connect/web/UserInfoInterceptor.java @@ -20,7 +20,6 @@ package org.mitre.openid.connect.web; import java.lang.reflect.Type; -import java.security.Principal; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -31,10 +30,13 @@ import org.mitre.openid.connect.service.UserInfoService; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.core.Authentication; import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.handler.HandlerInterceptorAdapter; import org.springframework.web.servlet.view.RedirectView; +import org.springframework.web.servlet.view.UrlBasedViewResolver; +import com.google.common.base.Strings; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.JsonElement; @@ -69,46 +71,57 @@ public class UserInfoInterceptor extends HandlerInterceptorAdapter { // or if there's already a userInfo object in there // TODO: this is a patch to get around a potential information leak from #492 - if (!(modelAndView.getView() instanceof RedirectView)) { - // get our principal from the security context - Principal p = request.getUserPrincipal(); - - if (p instanceof Authentication && !modelAndView.getModel().containsKey("userAuthorities")){ - Authentication auth = (Authentication)p; - modelAndView.addObject("userAuthorities", gson.toJson(auth.getAuthorities())); - } - - if (p instanceof OIDCAuthenticationToken) { - // if they're logging into this server from a remote OIDC server, pass through their user info - OIDCAuthenticationToken oidc = (OIDCAuthenticationToken) p; - if (oidc.getUserInfo() != null) { - modelAndView.addObject("userInfo", oidc.getUserInfo()); - modelAndView.addObject("userInfoJson", oidc.getUserInfo().toJson()); - } else { - modelAndView.addObject("userInfo", null); - modelAndView.addObject("userInfoJson", "null"); - } + if (modelAndView.getView() instanceof RedirectView) { + // don't add them + } else { + if (Strings.isNullOrEmpty(modelAndView.getViewName())) { + // add them + injectUserInfo(modelAndView); } else { - // don't bother checking if we don't have a principal or a userInfoService to work with - if (p != null && p.getName() != null && userInfoService != null) { - - // try to look up a user based on the principal's name - UserInfo user = userInfoService.getByUsername(p.getName()); - - // if we have one, inject it so views can use it - if (user != null) { - modelAndView.addObject("userInfo", user); - modelAndView.addObject("userInfoJson", user.toJson()); - } + if (modelAndView.getViewName().startsWith(UrlBasedViewResolver.FORWARD_URL_PREFIX) || + modelAndView.getViewName().startsWith(UrlBasedViewResolver.REDIRECT_URL_PREFIX)) { + // don't add them + } else { + // add them + injectUserInfo(modelAndView); } } } } - } + private void injectUserInfo(ModelAndView modelAndView) { + Authentication auth = SecurityContextHolder.getContext().getAuthentication(); + if (auth instanceof Authentication && !modelAndView.getModel().containsKey("userAuthorities")){ + modelAndView.addObject("userAuthorities", gson.toJson(auth.getAuthorities())); + } + + if (auth instanceof OIDCAuthenticationToken) { + // if they're logging into this server from a remote OIDC server, pass through their user info + OIDCAuthenticationToken oidc = (OIDCAuthenticationToken) auth; + if (oidc.getUserInfo() != null) { + modelAndView.addObject("userInfo", oidc.getUserInfo()); + modelAndView.addObject("userInfoJson", oidc.getUserInfo().toJson()); + } else { + modelAndView.addObject("userInfo", null); + modelAndView.addObject("userInfoJson", "null"); + } + } else { + // don't bother checking if we don't have a principal or a userInfoService to work with + if (auth != null && auth.getName() != null && userInfoService != null) { + + // try to look up a user based on the principal's name + UserInfo user = userInfoService.getByUsername(auth.getName()); + // if we have one, inject it so views can use it + if (user != null) { + modelAndView.addObject("userInfo", user); + modelAndView.addObject("userInfoJson", user.toJson()); + } + } + } + } } diff --git a/openid-connect-server/src/main/java/org/mitre/openid/connect/filter/AuthorizationRequestFilter.java b/openid-connect-server/src/main/java/org/mitre/openid/connect/filter/AuthorizationRequestFilter.java index 8a7dc32aa..ecf88ffdf 100644 --- a/openid-connect-server/src/main/java/org/mitre/openid/connect/filter/AuthorizationRequestFilter.java +++ b/openid-connect-server/src/main/java/org/mitre/openid/connect/filter/AuthorizationRequestFilter.java @@ -128,7 +128,6 @@ public class AuthorizationRequestFilter extends GenericFilterBean { List prompts = Splitter.on(PROMPT_SEPARATOR).splitToList(Strings.nullToEmpty(prompt)); if (prompts.contains(PROMPT_NONE)) { - logger.info("Client requested no prompt"); // see if the user's logged in Authentication auth = SecurityContextHolder.getContext().getAuthentication(); @@ -137,9 +136,8 @@ public class AuthorizationRequestFilter extends GenericFilterBean { // we're OK, continue without prompting chain.doFilter(req, res); } else { + logger.info("Client requested no prompt"); // user hasn't been logged in, we need to "return an error" - logger.info("User not logged in, no prompt requested, returning error from filter"); - if (client != null && authRequest.getRedirectUri() != null) { // if we've got a redirect URI then we'll send it