Skip to content

Conversation

@froozeify
Copy link
Member

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

User could edit the hidden input value on a readonly field and changed it value.
At the creation for readonly field we'll match the value based on the one defined in the Template predefined fields if it go one.

In other case it will fallback on the og value or unset it from the input, like it was done in #21134

While doing that I also update some hardcoded value to use it's enum correspondence.

Screenshots (if appropriate):

Screencast.from.2025-10-21.16-21-26.webm

@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch 2 times, most recently from 4abc2d0 to 6def124 Compare October 21, 2025 14:50
@froozeify froozeify marked this pull request as ready for review October 21, 2025 14:52
@cconard96
Copy link
Contributor

👎 Wouldn't this break API uses, or anything else that updates an ITIL Object beyond the case of a regular user directly trying to update using the web interface?

@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch from 6def124 to 7202cc9 Compare October 21, 2025 14:57
@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch from 7202cc9 to 17f19a9 Compare October 21, 2025 14:59
@froozeify froozeify marked this pull request as draft October 22, 2025 07:17
@AdrienClairembault
Copy link
Contributor

AdrienClairembault commented Oct 22, 2025

Wouldn't this break API uses, or anything else that updates an ITIL Object beyond the case of a regular user directly trying to update using the web interface?

For the API, I don't know if we want templates to be applied but I guess it make sense? If a user can't set a field through the UI, he should not be able to do it with the API too.

However, this code should not impact all ->update() operations from PHP code, for example we don't want to prevent a rule value from being applied.

So I guess we need something else than prepareInputForUpdate, maybe a new lifecycle hook that would be executed only for UI updates + both APIs? Maybe prepareUserInputForUpdate?

This is something we kinda already talked about with @cedric-anne, there is a need to have a separate check from prepareInputForUpdate as it currently contains profile based operations for some itemtype (e.g. rights checks that impact the input) that interfer with PHP logic sometimes.

Now, whether this is a good idea to start include this in a bugfixe issue in another question... Tbh I don't mind it because it would have a very low impact since the new method would be empty on CommonDBTM and only implemented in specifics child classes (so no behavior change expect this specific fix).

@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch from 8fa1344 to 65b45b6 Compare October 22, 2025 08:38
@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch from 65b45b6 to 8a9438d Compare October 22, 2025 08:39
@cconard96
Copy link
Contributor

This has always only been a feature that affects the display of fields in the web UI with no backend enforcement and I don't think it is good to change that on a bugfix version.

I'm also not sure how useful the feature is as of GLPI 11 or if it makes sense to continue to feature or phase it out. It has always confused some users. Some thought it was meant to be a sort of service catalog but now that's been done by Forms and enforced simplified users to use Forms for the initial ticket creation. It seems to me that something like Forms makes more sense for controlling fields.
There is one odd feature which I am not sure works 100% anymore where you can restrict statuses that can be used based on template (which is more that it is dependent on category). This seems to me like it doesn't need to be tied to templates necessarily.

For the API, I don't know if we want templates to be applied but I guess it make sense? If a user can't set a field through the UI, he should not be able to do it with the API too.

This is too dependent on us assuming everyone is using the API as their own user to do tasks and not a script doing automation with other systems where all of the fields are expected to be usable. Maybe a specific category is set to use a profile that makes the External ID readonly but a script that creates those kinds of tickets from another system is expected to set that field initially.

When a lot of assumptions on how people use GLPI are involved, I will always lean towards not agreeing on a change for a bugfix version.

So I guess we need something else than prepareInputForUpdate, maybe a new lifecycle hook that would be executed only for UI updates + both APIs? Maybe prepareUserInputForUpdate?

I am against enforcing this for either API. It may break things users are already using and severely limits the usefulness of things we may want to do with the API in the future. Maybe it could be opt-in with a special property in the input from the API if someone really wants to use the behavior for something like predefined fields but even that usefulness seems limited to me.

@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch from e44bb1a to 2e83385 Compare October 22, 2025 11:34
@froozeify froozeify force-pushed the 11.0/ensure-readonly-are-applied-on-itilobject branch from 2e83385 to 3ba3f6d Compare October 22, 2025 11:41
@froozeify
Copy link
Member Author

I've update the code so the readonly is only applied on query from the controllers.

But indeed creating a prepareUserInputForUpdate could be very useful (specially in case like that, where we want to do specific enforcement only on the user input side - and to see if API is seen as an user input), probably in a dedicated PR.

For the choice indeed we either see the API has having the same logic as a frontend user (so enforcing the readonly status) or be more like a system tool and having less restriction so (I think it's the current API logic)

@froozeify froozeify marked this pull request as ready for review October 22, 2025 13:25
if (isset($_POST["add"])) {
$change->check(-1, CREATE, $_POST);

$_POST = $change->enforceReadonlyFields($_POST, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not that be called from something like prepareInputForAdd() rather than duplicated in each front file?

Copy link
Member Author

@froozeify froozeify Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepareInputForAdd is also called by the api and system.

The readonly enforcing, after some discussion, for the moment must only applied on user form submission.

It's up to debate to enforce that also on the api side :#21578 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants