Poprawa jakości kodu

Question

Piszę obecnie swój pierwszy projekt w Spring MVC i mam kilka podstawowych pytań. Ponieważ mój obecny kontroller wygląda tragicznie pod względem jakości

package com.jonki.Controller;
 
import com.jonki.DTO.LoginDTO;
import com.jonki.Entity.User;
import com.jonki.Service.UserService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.access.annotation.Secured;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.authority.AuthorityUtils;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Controller;
import org.springframework.ui.Model;
import org.springframework.web.bind.annotation.*;
 
import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
 
@Controller
public class ApiController {
 
    @Autowired
    private UserService userService;
 
    private Logger logger = LoggerFactory.getLogger(ApiController.class);
 
    @RequestMapping(value = "/login", method = RequestMethod.GET)
    public String login(final Model model,
                        final HttpSession session,
                        final HttpServletRequest request) {
        String usernameFromCookie = checkCookieUsername(request);
 
        if (!usernameFromCookie.equals("")
                        && session.getAttribute("user") != null) {  // if exists cookie and session
 
            logger.info("/login Go to /loginSuccessfully, bacause exists cookie and session");
 
            return "redirect:/loginSuccessfully";
        } else if (!usernameFromCookie.equals("")) {  // if exists just cookie
            User user = userService.getUserByUsername(usernameFromCookie);
            session.setAttribute("user", user);
 
            UsernamePasswordAuthenticationToken authenticationToken = new UsernamePasswordAuthenticationToken(user.getUsername(),
                    null,
                    AuthorityUtils.createAuthorityList("ROLE_USER"));
            SecurityContextHolder.getContext().setAuthentication(authenticationToken);
 
            logger.info("/login Go to /loginSuccessfully, bacause exists cookie and I set session and principal security" + authenticationToken.getPrincipal());
 
            return "redirect:/loginSuccessfully";
        }
        model.addAttribute("loginDTO", new LoginDTO());
 
        logger.info("/login");
        return "login";
    }
 
    @RequestMapping(value = "/loginSuccessfully", method = RequestMethod.GET)
    public String loginSuccessfully(final HttpSession session,
                                    final HttpServletRequest request,
                                    final HttpServletResponse response,
                                    final Model model) {
        final String usernameFromCookie = checkCookieUsername(request);
        final String loginStatusFromCookie = checkCookieLoginStatus(request);
 
        if (usernameFromCookie.equals("")
                && !SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString().equals("anonymousUser")
                && (loginStatusFromCookie.equals("") || loginStatusFromCookie.equals("notLoggedIn"))) {
            System.out.println("1");
            Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal();
            String username = ((org.springframework.security.core.userdetails.User) principal).getUsername();
 
            User user = userService.getUserByUsername(username);
            session.setAttribute("user", user);
 
            Cookie cookieUsername = new Cookie("username", user.getUsername());
            cookieUsername.setMaxAge(3600);
            response.addCookie(cookieUsername);
            Cookie cookieLoginStatus = new Cookie("loginStatus", "loggedIn");
            cookieLoginStatus.setMaxAge(3600);
            response.addCookie(cookieLoginStatus);
 
            logger.info("/loginSuccessfully after /login");
        } else if (usernameFromCookie.equals("")
                        && SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString().equals("anonymousUser")) {
            logger.info("/loginSuccessfully Go to /login, because don't exists cookie and principal security");
            return "redirect:/login";
        } else if(!usernameFromCookie.equals("")
                    && session.getAttribute("user") == null) {
            User user = userService.getUserByUsername(usernameFromCookie);
            session.setAttribute("user", user);
 
            UsernamePasswordAuthenticationToken authenticationToken = new UsernamePasswordAuthenticationToken(user.getUsername(),
                    null,
                    AuthorityUtils.createAuthorityList("ROLE_USER"));
            SecurityContextHolder.getContext().setAuthentication(authenticationToken);
 
            logger.info("/loginSuccessfully I set session and principal security" + authenticationToken.getPrincipal());
        }
 
        logger.info("/loginSuccessfully");
        return "loginSuccessfully";
    }
 
    @Secured("ROLE_USER")
    @RequestMapping(value = "/logout", method = RequestMethod.GET)
    public String logout(final HttpSession session,
                         final HttpServletRequest request,
                         final HttpServletResponse response) {
        session.removeAttribute("user");
        SecurityContextHolder.clearContext();
 
        for (Cookie cookie : request.getCookies()) {
            if (cookie.getName().equalsIgnoreCase("username")) {
                cookie.setMaxAge(0);
                response.addCookie(cookie);
            } else if (cookie.getName().equalsIgnoreCase("loginStatus")) {
                cookie.setValue("notLoggedIn");
                response.addCookie(cookie);
            }
        }
 
        logger.info("Logout");
        return "redirect:/login";
    }
 
    private String checkCookieUsername(final HttpServletRequest request) {
        Cookie[] cookies = request.getCookies();
 
        String username = "";
 
        for (Cookie cookie : cookies) {
            if (cookie.getName().equalsIgnoreCase("username"))
                username = cookie.getValue();
        }
 
        return username;
    }
 
    private String checkCookieLoginStatus(final HttpServletRequest request) {
        Cookie[] cookies = request.getCookies();
 
        String loginStatus = "";
 
        for (Cookie cookie : cookies) {
            if (cookie.getName().equalsIgnoreCase("loginStatus"))
                loginStatus = cookie.getValue();
        }
 
        return loginStatus;
    }
}

i chciałbym go trochę rozpisać na osobne klasy. Czy jest sens tworzenia tylko osobnej klasy dla np. ustawiania sesji czy dodawania ciasteczek, albo np. ustawiania obiektu typu UsernamePasswordAuthenticationToken. If'y raczej muszą zostać, aby zabezpieczyć przed użytkownikami używającymi strony jak się chce.

Answer 1

• Wydzielić warstwę dla logiki biznesowej
• Wydzielić warstwę dla zastosowania chociażby commandów. 

 

Comments

A co z blokami warunkowymi? Chyba muszą tu zostać. Użytkownik wpisując sobie w adresie url daną podstronę przechodzi tam i muszę sprawdzać czy jest zalogowany czy nie. A może np. wyłączył przeglądarkę, a potem znowu ją włączył i muszę mu znowu nadać sesję. Czyli muszę wydzielić tzw. logikę biznesową i stworzyć klasy typu service dla np. sesji?

---

A teraz trochę lepiej się prezentuje?

package com.jonki.Controller;
 
import com.jonki.DTO.LoginDTO;
import com.jonki.Entity.User;
import com.jonki.Service.CookieService;
import com.jonki.Service.SecurityUserService;
import com.jonki.Service.UserService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.access.annotation.Secured;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.authority.AuthorityUtils;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Controller;
import org.springframework.ui.Model;
import org.springframework.web.bind.annotation.*;
 
import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
 
@Controller
public class ApiController {
 
    @Autowired
    private UserService userService;
    @Autowired
    private CookieService cookieService;
    @Autowired
    private SecurityUserService securityUserService;
 
    private Logger logger = LoggerFactory.getLogger(ApiController.class);
 
    @RequestMapping(value = "/login", method = RequestMethod.GET)
    public String login(final Model model,
                        final HttpSession session,
                        final HttpServletRequest request) {
        String usernameFromCookie = cookieService.getValueCookie(request, "username");
        securityUserService.setContext(SecurityContextHolder.getContext());
 
        if (cookieService.isCookie(request, "username")
                        && session.getAttribute("user") != null) {
 
            return "redirect:/loginSuccessfully";
        } else if (cookieService.isCookie(request, "username")) {
            User user = userService.getUserByUsername(usernameFromCookie);
            session.setAttribute("user", user);
 
            securityUserService.createUsernamePasswordAuthenticationToken(user.getUsername(),
                                                                    null,
                                                AuthorityUtils.createAuthorityList("ROLE_USER"));
            return "redirect:/loginSuccessfully";
        }
        model.addAttribute("loginDTO", new LoginDTO());
 
        return "login";
    }
 
    @RequestMapping(value = "/loginSuccessfully", method = RequestMethod.GET)
    public String loginSuccessfully(final HttpSession session,
                                    final HttpServletRequest request,
                                    final HttpServletResponse response,
                                    final Model model) {
        final String usernameFromCookie = cookieService.getValueCookie(request, "username");
        final String loginStatusFromCookie = cookieService.getValueCookie(request, "loginStatus");
        securityUserService.setContext(SecurityContextHolder.getContext());
 
        if (!cookieService.isCookie(request, "username")
                && !securityUserService.isAnonymousUser()
                && (loginStatusFromCookie.equals("") || loginStatusFromCookie.equals("notLoggedIn"))) {
            String username = securityUserService.getUsername();
 
            User user = userService.getUserByUsername(username);
            session.setAttribute("user", user);
 
            cookieService.addCookie(response, "username", user.getUsername());
            cookieService.addCookie(response, "loginStatus", "loggedIn");
        } else if (!cookieService.isCookie(request, "username")
                        && securityUserService.isAnonymousUser()) {
            return "redirect:/login";
        } else if(cookieService.isCookie(request, "username")
                    && session.getAttribute("user") == null) {
            User user = userService.getUserByUsername(usernameFromCookie);
            session.setAttribute("user", user);
 
            securityUserService.createUsernamePasswordAuthenticationToken(user.getUsername(),
                                                                 null,
                                                    AuthorityUtils.createAuthorityList("ROLE_USER"));
        }
 
        return "loginSuccessfully";
    }
 
    @Secured("ROLE_USER")
    @RequestMapping(value = "/logout", method = RequestMethod.GET)
    public String logout(final HttpSession session,
                         final HttpServletRequest request,
                         final HttpServletResponse response) {
 
        session.removeAttribute("user");
        SecurityContextHolder.clearContext();
        cookieService.removeCookie(request, response, "username");
        cookieService.changeLoginStatusInCookie(request, response, "loginStatus");
 
        return "redirect:/login";
    }
}

Nadal straszą mnie te if'y, ale nie ma na nie rady. Muszą być, aby sprawdzać czy są ciasteczka, sesja i obiekt z Spring Security. Takie sprawdzanie czy wszystko jest OK jest bardzo męczące i muszę te warunki ładować do każdego mapowania. A może źle to wszystko projektuje? Chodzi o to, że np. jak użytkownik ot tak wpisz sobie /loginSuccessfully, wtedy jeśli nie ma konkretnego ciasteczka i obiektu SecurityContext to przekierowuję go do /login, natomiast jeśli jest np. tylko ciasteczko, tzn., że użytkownik był zalogowany, tylko wyłączył przeglądarkę i automatycznie wykasowała się sesja i SpringContext, więc na nowo to ustawiam według jego nazwy w ciasteczku i takie warunki muszę ciągle dawać na wszystkie sposobności niepoprawnego używania strony. Możesz mi powiedzieć, czy robię to źle czy dobrze?