trebol-ecommerce / trebol-backend-monolith

Monolithic eCommerce backend web application that exposes a RESTful API.
MIT License
16 stars 21 forks source link

Restrictions: An user should not be able to delete their own account #14

Closed bglamadrid closed 3 years ago

bglamadrid commented 3 years ago

When a user tries to delete any account using the Data API, if the target account is their own, the action should be denied.

trangntt-016 commented 3 years ago

Hi @bglamadrid ,

I'm interested in this feature. Could you please assign this to me?

bglamadrid commented 3 years ago

Okay @trangntt-016 I'm assigning to you. Have fun! And don't hesitate to ask any questions you may have.

trangntt-016 commented 3 years ago

Hi @bglamadrid Thanks for giving me a chance. To solve this issue, I think Im going to add a Principal to the delete public void delete(Principal principal, @PathVariable String name) and then check if name is the current user. However, adding Principal will affect delete () in other classes such as Seller. I wonder if there are any better approach to make fewer changes. Also, I see there are some getters and setters, do you mind if I replace them with @Data?

bglamadrid commented 3 years ago

@trangntt-016

However, adding Principal will affect delete () in other classes such as Seller. I wonder if there are any better approach to make fewer changes.

In the DataUsersController, you can try creating that method as-new, let it do its checks and then call the former method when all checks are passed. You'll also have to move relevant annotations to the new method, so that the REST API is mapped and restricted accordingly.

Also, I see there are some getters and setters, do you mind if I replace them with @DaTa?

I'm not sure which ones are you referring to, can you elaborate please?

trangntt-016 commented 3 years ago

@bglamadrid

In the DataUsersController, you can try creating that method as-new, let it do its checks and then call the former method when all checks are passed. You'll also have to move relevant annotations to the new method, so that the REST API is mapped and restricted accordingly.

Thank you for your instruction. I think it's a good solution 😄 . However, one question came into my mind is that is it still okay to throw an exception (User cannot delete herself) in the controller (because we cannot modify the crudService.delete(String name), right?)

I'm not sure which ones are you referring to, can you elaborate please?

public class UserRole
  implements Serializable {

  private static final long serialVersionUID = 1L;

  @Id
  @GeneratedValue(strategy = GenerationType.IDENTITY)
  @Basic(optional = false)
  @Column(name = "user_role_id")
  private Long id;
  @Basic(optional = false)
  @Size(min = 1, max = 50)
  @Column(name = "user_role_name")
  private String name;

  public UserRole(UserRole source) {
    this.id = source.id;
    this.name = source.name;
  }

  public UserRole() { }

  public Long getId() {
    return id;
  }

  public void setId(Long id) {
    this.id = id;
  }

  public String getName() {
    return name;
  }

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

Not sure but I think all those getters() and setters() can be done with @Data 💭 🤔 ❔

bglamadrid commented 3 years ago

@bglamadrid

In the DataUsersController, you can try creating that method as-new, let it do its checks and then call the former method when all checks are passed. You'll also have to move relevant annotations to the new method, so that the REST API is mapped and restricted accordingly.

Thank you for your instruction. I think it's a good solution 😄 . However, one question came into my mind is that is it still okay to throw an exception (User cannot delete herself) in the controller (because we cannot modify the crudService.delete(String name), right?) Yeah, the idea is to cancel the action AND warn the user. Raising some exceptions will in turn make the API respond with proper status codes an message to the consumer. There's a @ControllerAdvice class around that maps these exceptions to said status codes and messages.

public class UserRole
  implements Serializable {

  private static final long serialVersionUID = 1L;

  @Id
  @GeneratedValue(strategy = GenerationType.IDENTITY)
  @Basic(optional = false)
  @Column(name = "user_role_id")
  private Long id;
  @Basic(optional = false)
  @Size(min = 1, max = 50)
  @Column(name = "user_role_name")
  private String name;

  public UserRole(UserRole source) {
    this.id = source.id;
    this.name = source.name;
  }

  public UserRole() { }

  public Long getId() {
    return id;
  }

  public void setId(Long id) {
    this.id = id;
  }

  public String getName() {
    return name;
  }

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

Not sure but I think all those getters() and setters() can be done with @DaTa 💭 🤔 ❔

Oh I see what you mean by @Data now; the lombok project's annotation-based code generation. I had forgotten it existed!

While it does help a lot to reduce lines of code, I'm not sure it's a good moment to add it since the API, and for instance, this backend, are subject to change in the foreseeable future. And it will include changes in some of these classes. Plus, I'm not sure how lombok abides when it comes to navigating method calls for one of those getters/setters.

trangntt-016 commented 3 years ago

While it does help a lot to reduce lines of code, I'm not sure it's a good moment to add it since the API, and for instance, this backend, are subject to change in the foreseeable future. And it will include changes in some of these classes. Plus, I'm not sure how lombok abides when it comes to navigating method calls for one of those getters/setters.

Yeah thanks for your insight @bglamadrid, I need to think about this before using @Data in the future