-
Notifications
You must be signed in to change notification settings - Fork 31
AMM-1239 : Role based broken access control #96
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
Changes from 9 commits
bd187b2
abd507b
2bca084
ca91a89
58a9826
7be5c2d
ee52ddf
9e395ed
9917e0f
20542c6
d1cec38
a2bc624
9d74754
13b5013
9be3aa1
c690b3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -228,6 +228,21 @@ public List<Frequency> getFrequency() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public List<V_GetUserlangmapping> getLanguageByUserId(Integer userId) throws ECDException { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return v_GetUserlangmappingRepo.findByUserId(userId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public String getUserRole(Long userId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (null == userId || userId <= 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new IllegalArgumentException("Invalid User ID : " + userId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| String role = roleRepo.getRoleNamebyUserId(userId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (null == role || role.trim().isEmpty()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new ECDException("No role found for userId : " + userId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return role; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (Exception e) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new ECDException("Failed to retrieverole for usedId : " + userId + " error : " + e.getMessage()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix typos in exception message. The implementation correctly follows the previous suggestions for input validation and error handling. However, there are typos in the exception message that need correction: - throw new ECDException("Failed to retrieverole for usedId : " + userId + " error : " + e.getMessage());
+ throw new ECDException("Failed to retrieve role for userId : " + userId + " error : " + e.getMessage());Additionally, consider preserving the original exception as a cause for better debugging: - throw new ECDException("Failed to retrieve role for userId : " + userId + " error : " + e.getMessage());
+ throw new ECDException("Failed to retrieve role for userId : " + userId, e);π Committable suggestion
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //gender master | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package com.iemr.ecd.utils.advice.exception_handler; | ||
|
|
||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| import org.springframework.security.access.AccessDeniedException; | ||
| import org.springframework.security.web.access.AccessDeniedHandler; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Map; | ||
|
|
||
| @Component | ||
| public class CustomAccessDeniedHandler implements AccessDeniedHandler { | ||
|
|
||
| private static final ObjectMapper mapper = new ObjectMapper(); | ||
| @Override | ||
| public void handle(HttpServletRequest request, | ||
| HttpServletResponse response, | ||
| AccessDeniedException accessDeniedException) throws IOException { | ||
| response.setStatus(HttpServletResponse.SC_FORBIDDEN); // 403 | ||
| response.setContentType("application/json"); | ||
| Map<String, String> errorResponse = Map.of("error" , "Forbidden", | ||
| "message","Access denied"); | ||
| response.getWriter().write(mapper.writeValueAsString(errorResponse)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,23 @@ | ||||||
| package com.iemr.ecd.utils.advice.exception_handler; | ||||||
|
|
||||||
| import java.io.IOException; | ||||||
|
|
||||||
| import org.springframework.security.core.AuthenticationException; | ||||||
| import org.springframework.security.web.AuthenticationEntryPoint; | ||||||
| import org.springframework.stereotype.Component; | ||||||
|
|
||||||
| import jakarta.servlet.http.HttpServletRequest; | ||||||
| import jakarta.servlet.http.HttpServletResponse; | ||||||
|
|
||||||
| @Component | ||||||
| public class CustomAuthenticationEntryPoint implements AuthenticationEntryPoint { | ||||||
|
|
||||||
| @Override | ||||||
| public void commence(HttpServletRequest request, | ||||||
| HttpServletResponse response, | ||||||
| AuthenticationException authException) throws IOException { | ||||||
| response.setStatus(HttpServletResponse.SC_UNAUTHORIZED); // 401 | ||||||
| response.setContentType("application/json"); | ||||||
| response.getWriter().write("{\"error\": \"Unauthorized\", \"message\": \"" + authException.getMessage() + "\"}"); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security vulnerability: Information disclosure and JSON injection risk. This has the same security issues as
Apply the same fix pattern: - response.getWriter().write("{\"error\": \"Unauthorized\", \"message\": \"" + authException.getMessage() + "\"}");
+ response.getWriter().write("{\"error\": \"Unauthorized\", \"message\": \"Authentication required\"}");Or use a JSON library for proper serialization as suggested in the π Committable suggestion
Suggested change
π€ Prompt for AI AgentsPotential information leakage and JSON injection vulnerability. Similar to the Apply the same security fixes as recommended for -response.getWriter().write("{\"error\": \"Unauthorized\", \"message\": \"" + authException.getMessage() + "\"}");
+response.getWriter().write("{\"error\": \"Unauthorized\", \"message\": \"Authentication required\"}");Or use a proper JSON library for safe serialization. π€ Prompt for AI Agents |
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| package com.iemr.ecd.utils.mapper; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; | ||
| import org.springframework.security.core.GrantedAuthority; | ||
| import org.springframework.security.core.authority.SimpleGrantedAuthority; | ||
| import org.springframework.security.core.context.SecurityContextHolder; | ||
| import org.springframework.stereotype.Component; | ||
| import org.springframework.web.filter.OncePerRequestFilter; | ||
|
|
||
| import com.iemr.ecd.service.masters.MasterServiceImpl; | ||
| import com.iemr.ecd.utils.constants.Constants; | ||
| import com.iemr.ecd.utils.redis.RedisStorage; | ||
|
|
||
| import io.jsonwebtoken.Claims; | ||
| import io.jsonwebtoken.io.IOException; | ||
| import jakarta.servlet.FilterChain; | ||
| import jakarta.servlet.ServletException; | ||
| import jakarta.servlet.http.Cookie; | ||
| import jakarta.servlet.http.HttpServletRequest; | ||
| import jakarta.servlet.http.HttpServletResponse; | ||
| @Component | ||
| public class RoleAuthenticationFilter extends OncePerRequestFilter { | ||
| Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
|
|
||
| @Autowired | ||
| private JwtUtil jwtUtil; | ||
|
|
||
| @Autowired | ||
| private RedisStorage redisService; | ||
|
|
||
| @Autowired | ||
| private MasterServiceImpl userService; | ||
|
|
||
| @Override | ||
| protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) | ||
| throws ServletException, IOException, java.io.IOException { | ||
| try { | ||
| String jwtFromCookie = getJwtTokenFromCookies(request); | ||
| String jwtFromHeader = request.getHeader(Constants.JWT_TOKEN); | ||
|
|
||
| String jwtToken = jwtFromCookie != null ? jwtFromCookie : jwtFromHeader; | ||
| if(null == jwtToken || jwtToken.trim().isEmpty()) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } | ||
| Claims extractAllClaims = jwtUtil.extractAllClaims(jwtToken); | ||
| if(null == extractAllClaims) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } | ||
| Object userIdObj = extractAllClaims.get("userId"); | ||
| String userId = userIdObj != null ? userIdObj.toString() : null; | ||
|
|
||
| String authRole = redisService.getUserRoleFromCache(Long.valueOf(userId)); | ||
| if (authRole == null) { | ||
| String role = userService.getUserRole(Long.valueOf(userId)); | ||
| authRole = "ROLE_" + role.toUpperCase(); | ||
| redisService.cacheUserRole(Long.valueOf(userId), authRole); | ||
| } | ||
|
|
||
| List<GrantedAuthority> authorities = List.of(new SimpleGrantedAuthority(authRole)); | ||
|
|
||
| UsernamePasswordAuthenticationToken auth = new UsernamePasswordAuthenticationToken(userId, null, | ||
| authorities); | ||
| SecurityContextHolder.getContext().setAuthentication(auth); | ||
| } catch (Exception e) { | ||
| logger.error("Authentication filter error for request {}: {}", request.getRequestURI(), e.getMessage()); | ||
| SecurityContextHolder.clearContext(); | ||
| } finally { | ||
| filterChain.doFilter(request, response); | ||
| } | ||
|
|
||
| } | ||
| private String getJwtTokenFromCookies(HttpServletRequest request) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this method seems to be appearing in a lot of files. Can't we reuse?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I have updated |
||
| Cookie[] cookies = request.getCookies(); | ||
| if (cookies != null) { | ||
| for (Cookie cookie : cookies) { | ||
| if (cookie.getName().equalsIgnoreCase(Constants.JWT_TOKEN)) { | ||
| return cookie.getValue(); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package com.iemr.ecd.utils.mapper; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π οΈ Refactor suggestion Move configuration class to appropriate package. Configuration classes should typically be placed in a Move this class to π€ Prompt for AI Agents |
||
|
|
||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.security.config.annotation.method.configuration.EnableMethodSecurity; | ||
| import org.springframework.security.config.annotation.web.builders.HttpSecurity; | ||
| import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; | ||
| import org.springframework.security.config.http.SessionCreationPolicy; | ||
| import org.springframework.security.web.SecurityFilterChain; | ||
| import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; | ||
| import org.springframework.security.web.csrf.CookieCsrfTokenRepository; | ||
|
|
||
| import com.iemr.ecd.utils.advice.exception_handler.CustomAccessDeniedHandler; | ||
| import com.iemr.ecd.utils.advice.exception_handler.CustomAuthenticationEntryPoint; | ||
|
|
||
| @Configuration | ||
| @EnableMethodSecurity | ||
| @EnableWebSecurity | ||
| public class SecurityConfig { | ||
| private final RoleAuthenticationFilter roleAuthenticationFilter; | ||
| private final CustomAuthenticationEntryPoint customAuthenticationEntryPoint; | ||
| private final CustomAccessDeniedHandler customAccessDeniedHandler; | ||
|
|
||
| public SecurityConfig(RoleAuthenticationFilter roleAuthenticationFilter, | ||
| CustomAuthenticationEntryPoint customAuthenticationEntryPoint, | ||
| CustomAccessDeniedHandler customAccessDeniedHandler) { | ||
| this.roleAuthenticationFilter = roleAuthenticationFilter; | ||
| this.customAuthenticationEntryPoint = customAuthenticationEntryPoint; | ||
| this.customAccessDeniedHandler = customAccessDeniedHandler; | ||
| } | ||
|
|
||
| @Bean | ||
| public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { | ||
| http | ||
| .csrf(csrf -> csrf.csrfTokenRepository(CookieCsrfTokenRepository.withHttpOnlyFalse())) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. withHttpOnlyFalse |
||
| .sessionManagement(session -> session.sessionCreationPolicy(SessionCreationPolicy.STATELESS)) | ||
| .authorizeHttpRequests(auth -> auth | ||
| .requestMatchers("/user/*").permitAll() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this matching mean?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the requestURL coming like 'user/userAuhenticate' it will give the permission it will not check the role. |
||
| .anyRequest().authenticated() | ||
| ).exceptionHandling(ex -> ex | ||
| .authenticationEntryPoint(customAuthenticationEntryPoint) | ||
| .accessDeniedHandler(customAccessDeniedHandler) | ||
| ) | ||
| .addFilterBefore(roleAuthenticationFilter, UsernamePasswordAuthenticationFilter.class); | ||
|
|
||
| return http.build(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,13 +21,16 @@ | |
| */ | ||
| package com.iemr.ecd.utils.redis; | ||
|
|
||
| import java.time.Duration; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.data.redis.connection.RedisConnection; | ||
| import org.springframework.data.redis.connection.RedisStringCommands.SetOption; | ||
| import org.springframework.data.redis.connection.lettuce.LettuceConnectionFactory; | ||
| import org.springframework.data.redis.core.RedisTemplate; | ||
| import org.springframework.data.redis.core.types.Expiration; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
|
|
@@ -94,4 +97,24 @@ public void updateConcurrentSessionObject(String value) { | |
|
|
||
| } | ||
| } | ||
| @Autowired | ||
| private RedisTemplate<String, String> redisTemplate; | ||
|
|
||
| public void cacheUserRole(Long userId, String role) { | ||
| try { | ||
| redisTemplate.opsForValue().set("role:" + userId, role, Duration.ofHours(1)); | ||
| }catch (Exception e) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indent/spacing |
||
| logger.warn("Failed to cache role for user {} : {} ", userId, e.getMessage()); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| public String getUserRoleFromCache(Long userId) { | ||
| try { | ||
| return redisTemplate.opsForValue().get("role:" + userId); | ||
| } catch (Exception e) { | ||
| logger.warn("Failed to retrieve cached role for user {} : {} ", userId, e.getMessage()); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't follow where this hasRole is checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify In RoleAuthenticationFilter line no 66