Overview

The experience_builder.api.config.auto-save.get route returns data of js_component and xb_asset_library entities without any authentication. This has been the case since #3518836: experience_builder.api.config.list route should not require 'administer themes' permission landed.

Proposed resolution

Restrict access properly.

User interface changes

n/a

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

balintbrews created an issue. See original summary.

penyaskito’s picture

Component: … to be triaged » Internal HTTP API
Issue tags: +beta blocker
Parent issue: » #3452581: [META] XB Permissions
balintbrews’s picture

Title: `api.config.auto-save.get` route returns data without any authentication » `api.config.get` route returns data without any authentication

I just realized that I meant to open the issue for api.config.get, that's where I originally discovered the problem. I haven't tested api.config.auto-save.get, so let's be sure to check that, too.

wim leers’s picture

I just realized that I meant to open the issue for api.config.get, that's where I originally discovered the problem.

That shouldn't be the case:

experience_builder.api.config.get:
  path: '/xb/api/v0/config/{xb_config_entity_type_id}/{xb_config_entity}'
  defaults:
    _controller: 'Drupal\experience_builder\Controller\ApiConfigControllers::get'
  requirements:
    _xb_http_eligible_config_entity: 'TRUE'
    _entity_access: 'xb_config_entity.view'
    # Allow a subset: `Component` config entities are read-only via the internal HTTP API.
    xb_config_entity_type_id: '(pattern|js_component|xb_asset_library)'

AFAICT this is simply questioning #3508694: Permissions for XB config entity types's introduction of explicit access for all?

IOW: authentication & authorization are present, authorization is simply granted very broadly.


I suspect what we want is the _user_is_logged_in thing #3508694: Permissions for XB config entity types did for experience_builder.api.config.list:

experience_builder.api.config.list:
  path: '/xb/api/v0/config/{xb_config_entity_type_id}'
  defaults:
    _controller: 'Drupal\experience_builder\Controller\ApiConfigControllers::list'
  requirements:
    # Any internal HTTP API-eligible XB config entity type.
    _xb_http_eligible_config_entity: 'TRUE'
    # But deter users from trying to access other config entities via this route: this ensures 404 responses rather than
    # 403.
    # @see \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testNonXbConfigEntity()
    xb_config_entity_type_id: '(component|pattern|js_component|xb_asset_library)'
    # The UI client can fetch all the components the current user has access too, regardless of which kind of XB-enabled
    # entity you're editing.
    _user_is_logged_in: 'TRUE'
  options:
    # Indicate that external applications may use this route. For authentication, see `modules/xb_oauth/README.md`.
    xb_external_api: true
  methods: [GET]

Because here's the thing: @lauriii wanted XB to both:

  • only have a single simple "admin permission" per config entity type: JavaScriptComponent::ADMIN_PERMISSION etc.
  • NOT have a catch-all "use XB" permission — see #3452581: [META] XB Permissions's functional requirements

See:

  1. my write-up about about this at #3508694-14: Permissions for XB config entity types.2
  2. my very detailed write-up about the consequences of this at #3508694-19: Permissions for XB config entity types.2
  3. confirmed by @lauriii at #3508694-20: Permissions for XB config entity types
wim leers’s picture

Title: `api.config.get` route returns data without any authentication » `api.config.get` route returns data without any authentication due to
Status: Active » Needs review
Issue tags: +Needs tests

Either we must do what I just pushed based on #4, or, and probably that's the safer option, \Drupal\experience_builder\EntityHandlers\VisibleWhenDisabledXbConfigEntityAccessControlHandler should be narrows to only logged in users rather than the current

      // We always allow viewing these entities, even if disabled.
      'view' => AccessResult::allowed(),

wim leers’s picture

Assigned: Unassigned » penyaskito
wim leers’s picture

Title: `api.config.get` route returns data without any authentication due to » `api.config.get` route returns data without any authentication due to broad `view` access

Actually, I think we can do both, but perhaps that means the first commit is now obsolete. Did that, now VisibleWhenDisabledXbConfigEntityAccessControlHandlerTest is (understandably!) failing.

Updated cacheability expectations of integration tests; those are passing now. But, this MR still needs to expand assertAuthenticationAndAuthorization() to facilitate testing GETting an unauthorized individual config entity (i.e. the new test coverage that this issue needs).

Leaving that for somebody else to finish 😇

wim leers’s picture

Status: Needs review » Needs work

#3530590: Expand functional test coverage for ContentCreatorVisibleXbConfigEntityAccessControlHandler is in, which means we kinda don't need this test coverage here anymore … except that it would be good to generalize to all XB config entity types, instead of just Pattern.

penyaskito’s picture

Assigned: penyaskito » wim leers
Status: Needs work » Needs review
Issue tags: -Needs tests

Added tests for anonymous GET at assertAuthenticationAndAuthorization based on openapi examples, so we can generalize to all XB config entity types.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs review » Reviewed & tested by the community

VERY nice work here, @penyaskito!

  • wim leers committed 2c6a01bb on 0.x
    Issue #3531913 by penyaskito, wim leers, balintbrews: `api.config.get`...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +openapi

Status: Fixed » Closed (fixed)

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