Following our convention we should use save for the service.

saveUser(User user)

If there's a need to separate create and update then we can add two methods, but in this case we don't need that just yet.

I don't think we want to pass the params map to the service. Passing the params map is an anti-pattern because it couples the service to the web layer (params is actually an instance of GrailsParameterMap).

Caveat: I know we do this a bunch in other places but since we're refactoring let's do it properly.

According to the MVC design pattern (https://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller)

Controller
Accepts input and converts it to commands for the model or view.

That means that the controller is allowed (supposed) to do some of the work of processing the request and parameter processing is one of the things. Whether we're binding to a command object or to a domain instance, the controller should be allowed to do some basic manipulation of the model.

One thing I'd like to look into a bit more is how we're supposed to handle password hashing in Grails 3. In Grails 1, it went through a custom codec class (utils.PasswordCodec.groovy). In Grails 3 we should look into using bcrypt or spring security to handle this.

But I think that should be reserved for another ticket since it comes with a lot more complexity (for example. Q: if we use a new method for hashing passwords, what happens to existing users A: they probably won't be able to log in until we hash those passwords as well).

Ok, I get the point, I was also hesitant of doing that, but wanted to avoid any logic in the controller.
Let me know your thoughts of how I should handle that. My prepositions:

1. do this:

if (!params.hasProperty("active")) {
  userInstance.active = false
}
userInstance.password = params?.password?.encodeAsPassword()
userInstance.passwordConfirm = params?.passwordConfirm?.encodeAsPassword()

in the controller layer and pass only userInstance to the addUser method, so:

User saveUser(User userInstance)

2. extract the params that I'm dealing with then, so e.g. params.password like:

User saveUser(User userInstance, String password, String passwordConfirm, Boolean hasActive)

and in the service deal with the logic above

3. create some kind of command class, e.g. called AddUserCommand:

class AddUserCommand {
  String password
  String passwordConfirm
  Boolean active
}

in the controller then bind the properties:

AddUserCommand addUserCommand = new AddUserCommand(params)

and pass it to the service method like:

User saveUser(User userInstance, AddUserCommand userCommand)

and deal with the logic from the first bullet in the service

Let me know your thoughts guys. Happy to hear from you all @awalkowiak @alannadolny @drodzewicz @jmiranda

business logic in controller

avoid any logic in the controller

It's tricky. I don't think I would classify any of that code as "business logic" since it's just glorified data binding. But there's an argument the other way as well.

I found a few discussions, but no definitive answer on that.

• https://softwareengineering.stackexchange.com/questions/316800/should-a-repository-or-the-login-register-service-have-an-encryption-service-as
• https://security.stackexchange.com/questions/49591/on-what-layer-should-password-hashing-and-or-encryption-take-place
• https://stackoverflow.com/questions/30558976/how-to-pass-params-into-services-that-i-get-from-the-gsp-and-i-have-in-my-contro

So we should discuss that in a tech huddle. And we should have the discussion keeping in mind that we may end up outsourcing our security to an external service.

parameter passing

I found a few examples that show parameter manipulation (and persistence) in the controller

• commonsemantics/CsSecurityDashboard/DashboardController.groovy
• frontlinesms/frontlinesms2/SettingsController.groovy
• bodiam/groovyblogs/AccountController.groovy

and a few others that pass the parameters map to the service

• Edenhofer/zenboot/UserController.groovy

zenboot might not be a good example because it does a lot of data persistence in the controller which is what we're trying to avoid.

But I think Grails itself has examples where it passes the full params map to the service without a second thought.

My main beef is that the class has web in the classpath.

grails.web.servlet.mvc.GrailsParameterMap

https://docs.grails.org/latest/api/grails/web/servlet/mvc/GrailsParameterMap.html

I don't want anything related to the web passed to the service.

For now I think we'll use (1). In the future I think (3) might be best, but with a more generic UserCommand class so it can be reused in other places. Option (2) is also a good idea in some places but not with a generic saveUser method. It would be more appropriate for a UserService.updatePassword() method Although we may want something more semantically appropriate.

For example,

• resetPassword() would be appropriate if we were implementing a forgot password feature
• savePassword() might be better if we reused the same code when creating or updating user as well as resetting the password)

And if we want to follow the grails convention it seems we'd pass parameters separately with encoding logic already applied.

securityService.doSomethingToPassword(params.id, params.password.encodeAsPassword(), ...)

or without if we wanted to handle password encoding in the service.

securityService.doSomethingToPassword(params.id, params.password, ...)

Again we need to think beyond just the sliver of code we're looking at and figure out what we're trying to accomplish across the application. In this case I guess we're creating a user. But what does it look like when we update that user, reset the password, upgrade password hashing, etc.

And then what does it look like when we're using spring security plugin OR an external service / API like okta, keycloak, etc.