prompt schema mismatch with parent plugin

I've added my 2c in #3577049: Update.php fails with InvalidArgumentException : The configuration property settings.prompt.login doesn't exist about how OIDC project has handled it; I think they should have namespaced out their new config, and in the least checked the top contribs to make sure there are no clashes.

I'm hoping at least we can change the config name to something unique, per my MR.

This would allow for the existing AAD code to work, and I expect AAD will want to migrate its config to the new system managed by OIDC anyway, so AAD doesnt need its own \Drupal\openid_connect_windows_aad\Plugin\OpenIDConnectClient\WindowsAad::getUrlOptions.

Side commentary

The windows_aad schema does not extend the openid_connect.client.plugin.*, but the plugin class does - as such there could be some unexpected issues with some of the code in the parent class when saving the plugin.

Its funny that OIDC doesnt have an official base-config that its facebook/okta/generic extend, but the module still feels it can dig into any plugin and modify its settings. Perhaps OIDC should be promoting and officiating whats in openid_connect.client.plugin.*.

That way, AAD would convert from:

openid_connect.client.plugin.windows_aad:
  type: mapping

To

openid_connect.client.plugin.windows_aad:
  type: openid_connect.client.plugin.base

Not unlike whats in, `datetime.schema.yml` et al.

Then, it would be more appropriate for OIDC to modify openid_connect.client.plugin.base:prompt

Or should we switch the prompt config namespace in this module?

Apologies everyone... I definitely should've looked into this before making that config change as my own project relies on this one 🤦🏼.

Instead of altering the upstream, I created MR 46 in this project to alter the plugin/schema to match.

Testing the MR in #6 Seems to work fine

/app $ drush updb -y
 ----------------- ----------- --------------- ------------------------------- 
  Module            Update ID   Type            Description                    
 ----------------- ----------- --------------- ------------------------------- 
  openid_connect    30005       hook_update_n   30005 - Update all plugins to  
                                                use the 'login' prompt.        
  openid_connect_   9207        hook_update_n   9207 - Convert prompt setting  
  windows_aad                                   to an array.                   
 ----------------- ----------- --------------- ------------------------------- 

 // Do you wish to run the specified pending updates?: yes.                                                      

>  [notice] Update started: openid_connect_update_30005
>  [notice] Update completed: openid_connect_update_30005
>  [notice] Update started: openid_connect_windows_aad_update_9207
>  [notice] Update completed: openid_connect_windows_aad_update_9207
>  [warning] Message: The OpenID Connect module settings have been updated. Administrators should  
> review the OpenID Connect configuration, particularly the prompt settings for  
> all enabled providers.
> 
 [success] Finished performing updates.

I can confirm too, the MR in #6 works fine! Thank you

I'm not sure which module is causing my issue, after after updating openid_connect and patching openid_connect_windows_aad I end up at Hotel California; I am no longer able to logout. This issue did *not* occur with the previous version of the modules.

The website encountered an unexpected error. Try again later.

Error: Call to a member function getPlugin() on null in openid_connect_windows_aad_openid_connect_redirect_logout_alter() (line 189 of modules/contrib/openid_connect_windows_aad/openid_connect_windows_aad.module).

Drupal\Core\Extension\ModuleHandler->alter() (Line: 88)
Drupal\hook_event_dispatcher\HookEventDispatcherModuleHandler->alter() (Line: 115)
Drupal\openid_connect\Service\LogoutService->getLogoutRedirectResponse() (Line: 437)
Drupal\openid_connect\Controller\OpenIDConnectRedirectController->redirectLogout()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 637)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 741)
Drupal\Core\DrupalKernel->handle() (Line: 19)

#3577436: Incorrect context in hook_redirect_logout_alter

The context variable in the logout redirect alter hook is an array: ['client' => 'windows_aad']

I'm not able to replicate with my configuration the problem described in #10 by @cafuego

Thanks @ericgsmith!

I updated the MR to correctly handle 'none' vs. 'noprompt' I also added an update hook to convert any original settings to the new empty array.

The upstream patch for #3577049: Update.php fails with InvalidArgumentException : The configuration property settings.prompt.login doesn't exist (MR 193) has been updated to help this patch correctly with the settings.

The fix seems to work when using no prompt, as well as when using prompt options.

+1 RTBC once the PHPCS issues are fixed

When visiting /admin/config/people/openid-connect/add/windows_aad, I see this error:

Warning: foreach() argument must be of type array|object, string given in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 113 of core/lib/Drupal/Core/Render/Element/Checkboxes.php).

This is because of the defaultConfiguration having 'prompt' => '<noprompt>',

OpenIDConnectClientBase has '#default_value' => $this->configuration['prompt'] ?? [],, so I set 'prompt' => [], in defaultConfiguration()

This fixes the error for me.

Otherwise the default configuration triggers the "Client promp" may be misconfigured" error on the status page - would this be confusing for users who don't explicitly opt in to this behaviour?

I think it's fine. It's a warning, not an error. But even so, the default of none makes sense to me. If you want an SSO setup across multiple apps, selecting Force Login as the configuration breaks that, as signing into Azure AD / Entra ID elsewhere will not be enough to get a session in Drupal. The entire purpose of having Azure AD as the authentication provider for SSO is so that you can sign in to everything with a single login.

Still, if there is actually a risk for some use-cases, then maybe login is an acceptable default.

Does anyone else have an opinion? 'prompt' => ['login'], is what OpenID Connect specifies as the default...

I changed the default to err on the side of more restrictive (requiring login), as that's also the parent default config. But still, Azure AD (Entra ID) is an SSO provider. If you want it to actually be a single sign-on, instead of signing in multiple times, once for each app, then you would have to select none.

Thanks to all! I updated the contribution records.

After the update hook, I think everyone should review the updated client settings. This is because noprompt was the default in prior versions, but the update process may change it to login. Thanks to everyone for the collaboration in this issue.

I just tagged the parent with 3.0.0-alpha8 which should coincide with this fix.

After the update hook, I think everyone should review the updated client settings. This is because noprompt was the default in prior versions, but the update process may change it to login. Thanks to everyone for the collaboration in this issue.

Note: I attempted to detect the `noprompt` settings from the windows AAD plugin within the new 30006 update hook in the parent. Definitely inspect your config.

I apologize for accidentally stepping on your configuration values. I'll definitely be more careful next time.

@webflo Will there be a release soon?

@solideogloria Yes. I have tagged 2.0.0-beta10.

Thank you