Problem

The main configuration form is looking for Bynder domains under getbynder.com only, this doesn't always happen.
Also, the Oauth token validation is only accepting 32 char long tokens when at least 40 char long should be accepted as well.

Proposed resolution

Change the validateForm function to accept all the necessary values.

Comments

jfs.csantos created an issue. See original summary.

Bynder’s picture

Removed validation for Bynder domain names and for token character length as they are not fixed formats.

jfs.csantos’s picture

jfs.csantos’s picture

Assigned: jfs.csantos » slashrsm
Status: Needs work » Needs review
slashrsm’s picture

Status: Needs review » Needs work
  1. +++ b/src/Form/BynderConfigurationForm.php
    @@ -414,13 +414,6 @@ class BynderConfigurationForm extends ConfigFormBase {
    -    foreach (['consumer_secret', 'token_secret'] as $name) {
    -      if (strlen(trim($credentials[$name])) !== 32 || !ctype_alnum(trim($credentials[$name]))) {
    -        $form_state->setError($form[$name], $this->t('@label needs to be 32 characters long and contain only letters and numbers.', [
    -          '@label' => $form[$name]['#title']->render(),
    -        ]));
    -      }
    -    }
     
    

    Shouldn't we leave this check and just allow longer strings?

  2. +++ b/src/Form/BynderConfigurationForm.php
    @@ -440,10 +433,6 @@ class BynderConfigurationForm extends ConfigFormBase {
    -    if (!(substr(trim($credentials['account_domain']), 0, 8) === 'https://') || !(substr(trim($credentials['account_domain']), -15) === '.getbynder.com/')) {
    -      $form_state->setError($form['account_domain'], $this->t('Account domain expect a value in the format ":url".', [':url' => 'https://some-name.getbynder.com/']));
    -    }
    -
    

    I think that we should still check if the provided string is a valid domain.

jfs.csantos’s picture

I'll leave the check for alphanumeric , the length one is kind of irrelevant because we can have either 23 or 40 char tokens live, and one test environments they can be even simpler. As long as it's a required field it should be fine, if we change the token formats in the future we can update the validation as well.

Changed the domain validation to check for url.

jfs.csantos’s picture

Status: Needs work » Needs review
slashrsm’s picture

Patch in #6 is missing the description change from #2. I think that we'll also need to update \Drupal\Tests\bynder\FunctionalJavascript\ConfigurationFormTest.

slashrsm’s picture

Status: Needs review » Needs work
slashrsm’s picture

Assigned: slashrsm » jfs.csantos
Priority: Major » Normal
jfs.csantos’s picture

Updated patch #6 descriptions.

jfs.csantos’s picture

Assigned: jfs.csantos » slashrsm
Status: Needs work » Needs review
slashrsm’s picture

Re-uploading the patch. Hoping that tesbot will grab it.

Status: Needs review » Needs work

The last submitted patch, 13: bynder-config-form-validation-fix-2862587-11.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
StatusFileSize
new5.84 KB
new4.19 KB

Let's see if this fixes the tests.

  • slashrsm committed 8226a77 on 8.x-1.x
    Issue #2862587 by slashrsm, jfs.csantos, Bynder: Bynder configuration...
slashrsm’s picture

Status: Needs review » Fixed

Committed.

Status: Fixed » Closed (fixed)

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