What is the “correct” or “safe” way to update a pe

Question

Given a very simple object:

class User {
    private Integer id;
    private String name;

    public User() {}

    public Integer getId() { return id; }
    public String getName() { return name; }

    public void setName(String name) { this.name = name; }
}

and a very simple controller action:

@RequestMapping(value="/edit/{id}/**", method=RequestMethod.POST)
public String editFromForm(@PathVariable("id") Integer id, @Valid User user, BindingResult bindingResult, Model model) {
    // If we have errors, don't save
    if(bindingResult.hasErrors()) {
        // Put what they did in the model and send it back
        model.addAttribute(user);

        return "users/edit";
    } else {
        userDAO.save(user);
    }

    // Show them the updated page on success
    return "redirect:/users/" + user.getId() + "/" + user.getName();
}

and a very simple form:

<sf:form method="POST" modelAttribute="user">
    <label for="user_name">Name:</label>
    <sf:input path="name" id="user_name" />
    <input type="submit" value="save" /><sf:errors path="name" cssClass="error" />              
</sf:form>

How should I be updating the entity in the database? Currently (since saveOrUpdate() is the actual hibernate call behind my DAO's save() method, a new object is persisted instead of updating the existing one because the id field is not being set on the object created from the form submission.

A couple of possible solutions have come to me, but I am not sure which is best in terms of keeping things clean and also secure (so that a malicious user cannot just fire in edits to whatever object Id's they wish).

1. Insert the id from the URL parameter into the object coming from the model binder
2. Have a hidden id field in the form and let the model binder attach the id

In both of those cases, there is no check in place to make sure that the object is still the same one, such as a checksum of some sort. How do others deal with this? Are there any clear example that walk through this issue?

Another issue that comes up is that I'd rather not need a setId() method since Hibernate is managing all of the id's. From what I have been able to determine, the Spring MVC model binder can only bind a field if it has the expected getter and setter. Is there some other way to apply the new state, such as getting the current User from the db by the URL's id parameter and then applying the new state to it, but without having to explicitly code all of the field copies?

I am sure there is a fairly simple, straightforward way to handle this, but my heat-addled brain does not seem to be able to come up with a suitable solution.

I'm fairly new to Spring + Hibernate so forgive me if this is one of those mundane, highly covered topics, but I have not been able to find any clear example that covers my very simple situation. If this has been covered sufficiently elsewhere, please point me in the right direction.

Answer 1

I agree 100% on what @matt b says. I have worked with similar products where at the end of the day you don't have any way of knowing if the user is editing the right object. Your only option is to make sure the user is permitted to do so. If you have a user who is editing their profile then obviously they shouldn't be editting any other person.

Now the easiest way to do what you are trying to do is actually niether. YOu don't want to use the url or hidden attributes. YOu should be able to use @ModelAttribute to do this. Here is an example

  @RequestMapping(method = RequestMethod.GET)
    public ModelAndView setupForm(@RequestParam("petId") int petId) {
        User user = ...
        model.addAttribute("user", user);
        return model;
    }

Then you should be able to do

  @RequestMapping(method = RequestMethod.POST)
    public ModelAndView submit(@ModelAttribute("user") User user) {
       ...
    }

There is no setters or hidden fields. All the loading and binding is handled via Spring. More documentation here.

Answer 2

Still have the same User POJO

class User {
    private Integer id;
    private String name;

    public User() {}

    public Integer getId() { return id; }
    public String getName() { return name; }

    public void setName(String name) { this.name = name; }
}


@RequestMapping("/create/userCreate.do")
    public String createUser(Model model) {
        User user = new User();
        model.addAttribute(user);

        return "userCreate";
    }

Here I don't send the Id or any parameter, just pass the User form model attribute

@RequestMapping(value="/edit/userEdit.do", method=RequestMethod.POST)
public String editFromForm(@ModelAttribute("user") @Valid User user, BindingResult bindingResult, Model model) {
    // If we have errors, don't save
    if(bindingResult.hasErrors()) {
        // Put what they did in the model and send it back
        model.addAttribute(user);

        return "users/edit";
    } else {
        userDAO.saveOrUpdate(user);
    }

    // Show them the updated page on success
    return "redirect:/users/" + user.getId() + "/" + user.getName();
}

Here, the User object is bind user name and id. If there is no Id then the object will be saved and if the user object already is having Id then it will be updated. You don't have to pass id or parameter and make it visible on the address bar. Sometimes I don't like the idea of showing parameters to the users. I hope that helps

<%@taglib uri="http://www.springframework.org/tags" prefix="spring"%>
<%@taglib uri="http://www.springframework.org/tags/form" prefix="form"%>
<%@taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c"%>

<head>
<title> User Edit
</title>
</head>
<body>
    <form:form method="post" action="userEdit.do" commandName="user">
        <form:hidden path="id" />
                    <form:label path="username">User name:</form:label>

                <form:input path="username" type="text" maxlength="20" />
                <form:errors path="username" />
    </form:form>
</body>
</html>

Answer 3

Im a bit late, but maybe it will be useful to someone. Today I solved the same problem. I think your problem was there:

} else {
    userDAO.save(user);
}

If it was hibernate session save, then new id was generated each time it was called. I used

session.saveOrUpdate(user) 

instead of

session.save(user)

Answer 4

A couple of possible solutions have come to me, but I am not sure which is best in terms of keeping things clean and also secure (so that a malicious user cannot just fire in edits to whatever object Id's they wish).

Neither of the two approaches you mention will really handle a user who is attempting to edit objects that the user is not authorized to. At the end of the day, the user submitting the form needs to tell you which object they are submitting data for - whether it is in the URL parameter or in a hidden form parameter. I would say which of the two you choose is a matter of style and personal preference, really.

But what you need to be doing regardless of choice is to verify that the currently-logged-in user is authorized to change the object when processing the form submission. This would mean you need to check that this user is entitled to edit the current object ID, using whatever logic comprises "is allowed to do this" for your application.