Editing a user doesn't have constraints for the admin user

[skateman]

In the UI the user editing form (and controller) doesn't allow the user to change the name or the assigned groups of the admin user. However, this is not the case in the API and it is possible to rename the admin user or assign it to certain groups which I think might break RBAC stuff.

Ideally these constraints should be in the DB model, but we don't really do that because that might hurt db seeding so I guess we should mimic the behavior of the UI in the API.

@miq-bot assign @gtanzillo
@miq-bot add_label bug
cc @Fryguy

[Fryguy]

Can you link to specific code...I agree it should move to the model, actually, but it depends on the specifics

[skateman]

https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/views/ops/_rbac_user_details.html.haml#L9

[Fryguy]

I'd be ok with this in the model. I don't think it will affect specs, but if it does we could, worst case, always wrap the validation in an unless Rails.env.test?

[kbrock]

For this case, putting in the model probably make sense.

But we're hitting more and more use cases where we have business logic in the controllers and no good place to duplicate this logic in the api controllers. Not duplicating it would be even better.

I'm ok making a slew of changes across all tests to avoid the one off manner of Rails.env.test?
Specifically with users and requiring tenants and groups, we've (I've) made quite a few changes across the board to make them behave in tests with a test only kludge.

It would be nice if we could keep in mind that we need a more permanent recipe for where we want to put this controller logic in a more sustainable place.

[skateman]

It would be nice if we could keep in mind that we need a more permanent recipe for where we want to put this controller logic in a more sustainable place.

These are more like constrains that should be in the model, at least that's where I'd put them in my own hypothetical Rails app 🤔

[Fryguy]

@skateman I agree. Generally, constraints that apply to all users should be in the model (or even the database)...things like specific value ranges, enums, certain validations. Some things we can't put in the model, particular if they only apply to a subset of users, or if they prevent standard backend usage like deleting records.

In this particular case, while this is a user-specific change, that user in question is well-defined, and I think the model is an appropriate place.

[miq-bot]

This issue has been automatically marked as stale because it has not been updated for at least 3 months.

If you can still reproduce this issue on the current release or on master, please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.