[com_fields] Confusing Permissions

[infograf768]

Steps to reproduce the issue

As Super User, create an article custom field.
Also Create a Field Group
Logout and login as Manager or Administrator (Permissions for Edit Value is Not Allowed per default) for BOTH Fields and Field Groups

Expected result

Not sure

Actual result

The Manager or Administrator can edit anything in the field (and save) EXCEPT its Type which is readonly.

					// Rendering the type
					$node = $type->appendXMLFieldTag($field, $fieldset, $form);

					if (!FieldsHelperInternal::canEditFieldValue($field))
					{
						$node->setAttribute('disabled', 'true');
					}

![screen shot 2016-11-02 at 08 39 14](https://cloud.githubusercontent.com/assets/869724/19920255/ea0d2b2c-a0d7-11e6-93d3-3fa81d581ee6.png

The Manager or Administrator can edit anything in the Field Group (and save)

Additional comments

This new permission tip is confusing. What is Edit Value really for?

[laoneo]

Please have a look here joomla-projects/custom-fields#75 it is describing what this rule does.

[infograf768]

It is not obvious at the least (at reading the tip which therefore should be modified) and we do not have specific permissions to access the Fields and Fields Groups to create/edit/ etc.)
For example, the menus and/or sidebar, should only display with specific permissions.
Modifying a field should also have specific permissions, etc.

[laoneo]

This edit value permission is used when the user edits an article. It defines who can edit a field in the article and not when the user is editing a field itself.

[infograf768]

Which means the tip is wrong on one hand as not enough descriptive.
On the other hand, many permissions are not implemented which should be, as I said above.

[laoneo]

What would you then see for a descriptive text?

[infograf768]

Well it would first clearly state that it concerns custom fields.
But it is still not clear to me what is exactly the "value" for you, even after reading joomla-projects/custom-fields#75

But my question does not only concern this new Permission. It also concerns the general permissions of com_fields, I mean
I.e. all where it is necessary, add
$canDo = JHelperContent::getActions('com_fields');

then for example

// Set the actions for new and existing records.
        if ($isNew)
        {
            // For new records, check the create permission.
            if ($canDo->get('core.create'))
            {
                JToolbarHelper::apply('filter.apply');
                JToolbarHelper::save('filter.save');
                JToolbarHelper::save2new('filter.save2new');
            }
etc.

and also display or not the menus Fields and Field Groups with $user->authorise
etc.

As far as I understood your code, it checks sometimes for the permissions in Users, Content, Contact, but never com_fields own permissions

[infograf768]

To clarify a bit more if necessary:
A user who can create, edit or/and edit own Articles may NOT be authorized to edit create edit own Fields or Field Groups. He may not be able to deal at all with Fields and Field Groups.
I imagine for example that a Manager should not even be authorized to access to these managers.
The problem is that we are redirected to Articles Permissions when we display the Fields Manager for example.

[laoneo]

The permissions should always being considered from the component the field belongs to for example here https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_fields/views/fields/view.html.php#L94. Where did you see code where it isn't?

[infograf768]

It should use also com_fields permissions for which we do NOT have any UI.
As I said a Manager can access Fields and Field Groups as he/she has access to Articles.

[laoneo]

We never had the case to have permissions for two different use cases in the same component on core as we have now. Sonner or later we would come to that point. So we need to find a solution for that problem.

But honestly would that use case exist that somebody has access to the back end and can edit articles but not the custom fields of them, sounds for me very theoretical.

[infograf768]

It is not theoretical at all.
There are reasons why a Manager (for example, but we can have also specific back-end users) has no default access to some stuff in back-end. Editing an article and creating a new field are 2 very different actions.

[brianteeman]

I agree that creating a field must be a different permission to using a field

[Bakual]

But honestly would that use case exist that somebody has access to the back end and can edit articles but not the custom fields of them, sounds for me very theoretical.

I'd say that will happen a lot of times. An admin would set up the custom fields for a site and authors, managers and the like are using them.

Imho, there should be a menu item for com_fields with an own UI where you can manage all the custom fields from all extensions in a central place. As a user I would have expected such a manager. And there, you could have all the global permissions, overrideable on at least group level.

[brianteeman]

@laoneo I did keep telling you that there should be a com_fields menu item at that fields should be created there ;)

[laoneo]

there should be a menu item for com_fields with an own UI where you can manage all the custom fields from all extensions in a central place

Sorry but that discussion was long time ago over. Now it is definitely too late to change it. I'm not going to repeat myself why it should have it's own UI and why not. Sorry but we had this project open for months and now after the merge you want to change the whole architecture of it.

@brianteeman I know and I hopefully gave you enough arguments not to do. I had the impression I could convince you 🙈

[Bakual]

Now it is definitely too late to change it.

It is never to late to do the right thing.

Sorry but we had this project open for months and now after the merge you want to change the whole architecture of it.

That is no argument at all. If there are major issues with the current architecture, it needs to be changed or removed again or we end up with yet another technical debt.

[brianteeman]

@laoneo you convinced me enough to compromise on all aspects that I had considered - i obviously hadnt considered the acl - my fault

I absolutely share your disappointment that people never tested anything in the 9 months + that this has existed. Unfortunately those of us who did do significant testing are not developers so couldnt comment on code architecture.

As I have mentioned in the maintainers group I am very surprised that something as fundamental as the architecture (extending categories) was not reviewed by the PLT 9 months + ago. It would have seem logical to me that this was done before you were asked to spend such a considerable amount of time on this PR. Some of the most vociferous comments here are by people who had the skill and ability to test and were asked numerous times to test but for whatever reason did not. I share you frustration with that and there comments and language

[laoneo]

I'm sure we can solve the ACL issue differently than interrogate the whole architecture. You need to see the whole picture.

@Bakual did you try to integrate it into one of your components? If not, then spend some time to do it and you will see hopefully that it would make sense how it is now.

[Bakual]

@laoneo That's what I currently try to do and why I stumbled over the things.

[infograf768]

I guess, without redoing all under a unique Fields Component, we could at least implement a com_fields menu which would deal only with ACL.
Then it would be a matter of using these ACL where they are needed.

That would not force to redo the whole architecture.
Would only remain to deal with the code added to com_categories, which is indeed an important part of that architecture.

[laoneo]

I suggest to either try to have multiple rules in the component options, or to implement permission inheritance from a field group, when a field is assigned to a group. I would go with option two as it is probably easier to implement.

Like that you can define who can manipulate the fields which belong to this field group. The field group will then inherit from the component options. All would look then consistent how a user would expect it. If the field is not assigned to a group, it inherits the permissions from the component it belongs to directly.

I really would NOT go the way to give com_fields it's own menu item just because of ACL as com_categories also doesn't have one. Was there ever a request to manage categories for all components from it's own menu item?

[Bakual]

Was there ever a request to manage categories for all components from it's own menu item?

I don't know if that was ever requested or not, but com_fields isn't com_categories. It does something completely different.
By the way, categories take the ACL definitions from the component access.xml file. It looks for the category section in there.

Personally I think the fields don't need the permission tab. They could indeed inherit from the field groups and component (3rd party or com_fields, needs to be decision) permissions.
If we get rid of permissions for fields, we could also remove the asset_id column in the database table as it would be no longer used.

[rdeutz]

Sorry but that discussion was long time ago over. Now it is definitely too late to change it. I'm not going to repeat myself why it should have it's own UI and why not. Sorry but we had this project open for months and now after the merge you want to change the whole architecture of it.

I can understand the low level of happiness :-), I did merge it to move forward with com_fields and figure out what could be done better. We need more people looking at it, it is important not repeat the mistakes we made with com_tags. More people also mean more and different ideas and this is a good thing in my opinion.

I don't think anything must be done by you @laoneo we can work together to make it the best possible solution.

[laoneo]

IMO a field does need it's own permission, especially the edit value permission as it can be very likely that setting a field value, when editing an article, should only be possible by an admin.

I don't know if that was ever requested or not, but com_fields isn't com_categories. It does something completely different.

Yes it does something different, but how it should be integrated should be the same way. It should look like that custom fields does belong to the component which is integrating it. At least that was my intention, because I tought that Joomla should be slimm on the Interface (thats why there is plan to remove some core extensions like web links or banners). But perhaps I'm wrong.

By the way, categories take the ACL definitions from the component access.xml file. It looks for the category section in there.

That's why I suggested to inherit the permission from the field group (category) to the field.

I don't think anything must be done by you

No comment here, but thanks for being positive.

[ggppdk]

I think most useful permission is the "Edit value" permission that controls which users can modify the field in article / record form (probably the only permission need to the fields, that is why you cannot remove the assets from them)

• the other "edit, create, etc" permissions will be rarely useful, (i have them in custom software, but i think they are very rarely used, nobody ever asked about them, unlikely for edit-value permission !! which several sites find useful, because it concerns the frontend users (and backend users of course))

[EDIT]
to avoid frustration, default setting for "edit-value" should be granted to ALL usergroups, then advanced users can modify it if needed

[ggppdk]

IMO a field does need it's own permission, especially the edit value permission as it can be very likely that setting a field value

If we get rid of permissions for fields, we could also remove the asset_id column in the database table as it would be no longer used.

But this is true too, if you take the needed "edit-value" permission and you replace it with a multi-value selection of usergroups (not same as ACL but more than enough), then i guess you can remove all asset stuff for the fields

[EDIT]
Title of edit-value is not bad, bad description of it is confusing

[laoneo]

Can we close this one as with the new field groups #13019 ACL is stabilized?