We have permissions in place that will allow users to edit some domain records, but not all.

Finish that implementation. See \Drupal\domain\DomainAccessControlHandler

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KittenDestroyer created an issue. See original summary.

KittenDestroyer’s picture

Status: Active » Patch (to be ported)
FileSize
1.88 KB

I've attached patch in which added access check function and modified condition in DomainAccessControllHandler. Please review.

GitHub - https://github.com/agentrickard/domain/issues/134

zerolab’s picture

Status: Patch (to be ported) » Needs review

Hi KittenDestroyer,

Do mark the issue as "Needs review" when posting a patch.
This will make the drupal.org test runners kick in, and is an indicator for others.

Marking it as "needs review" to see how the d.o. testbot behaves. But the patch needs work.

I suggest you use PHPCS (see instructions) for code style (e.g. The docblock domain_user_has_access() needs a description. The $uid param needs type)

Also, this feels like a circular dependency. field_domain_access is provided by domain_access which depends on domain.

zerolab’s picture

Status: Needs review » Needs work

Heh,

@agentrickard looks like you need to enable tests for 8.x

KittenDestroyer’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

Please review. I've added condition to check if domain_access module enabled. Also I think dependency is not an issue, because if domain_access module is disabled, there are no options to assign users to domains, thus DomainAccessControlHandler is not called.

Also I've added description to domain_user_has_access() function, and type to $uid parameter.

KittenDestroyer’s picture

Assigned: KittenDestroyer » Unassigned
agentrickard’s picture

We can't run tests on the d.o. test runner, as far as I know, because our tests require multiple domains be pointed to the test server. The test runner doesn't allow that, so all tests would always fail.

This is why we use Travis.

@KittenDestroyer

Great! I am actively working in GitHub, so posting there would get quicker review. I'll link this over.

agentrickard’s picture

It would be much cleaner to make the domain_user_has_access() function a method on the DomainAccessControlHandler, since that's where the request is handled.

Drupal in general is moving to methods rather than module functions.

This is also an interesting question because the user assignments are in Domain Access, not Domain proper, which the patch accounts for but is possibly an architecture issue.

The code should be using the getAccessValues() method on the DomainAccessManager to retrieve the user's domain assignments. I wonder also if we should check the getAllValue(), since users can now be assigned to "all affiliates".

agentrickard’s picture

RE: architecture.

The _best_ thing to do here would be to use some form of access plugin, so that there was a default implementation from Domain module, which could be extended by Domain Access.

In the D7 days, this would be a hook, but there may be a "better" way in D8. Perhaps an Event or a Plugin.

agentrickard’s picture

Looking through the code, here's what I think might work:

* The AccessControlHandler is defined in the Domain entity annotation:

 *   handlers = {
 *     "storage" = "Drupal\Core\Config\Entity\ConfigEntityStorage",
 *     "view_builder" = "Drupal\domain\DomainViewBuilder",
 *     "access" = "Drupal\domain\DomainAccessControlHandler",
 *     "list_builder" = "Drupal\domain\DomainListBuilder",
 *     "view_builder" = "Drupal\domain\DomainViewBuilder",

* Domain Access could alter that entity definition and replace the access handler with its own. Using hook_entity_type_alter().
* The new access handler would use the logic of domain assignments, if present, which is sketched out in this patch and needs to conform to OO principles.

agentrickard’s picture

And after a little time, the above makes me wonder: Are we making a mistake by tying the user domain field to Domain Access?

That is, should user domain assignments be handled by Domain module proper?

Or should we split Domain Access editorial assignments and Domain management assignments into two separate fields?

agentrickard’s picture

I think we should split it into separate fields. We never did so in the past because Domain and Domain Access were not separated cleanly.

agentrickard’s picture

If you are still interested, the foundational work for this is in https://github.com/agentrickard/domain/tree/134-access

If provides a "Domain administrator" field on users and the basis for advanced access checks.

I am fairly certain that the code can live in DomainAccessControlHandler, which is available via the EntityTypeManager service.

For usage example, see DomainListBuilder::getOperations().

agentrickard’s picture

Status: Needs review » Fixed

This work is in Alpha7.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.