Adding Generic Oauth2 app and set empty scope then cause system crash

The #3 just a use case. The description provides the simply steps to reproduce. There are only two steps then you get the result of the system crashing.

Hi dravenk,

I confirmed the issue was resolved when I applied MR!43 to the "OpenID Connect / OAuth client" module against Version 3.x-dev in my local.

Please see the screenshots attached for your reference.

For your review.
Thank you.

I was able to write a functional test to confirm this fix is working. During the automated testing, I discovered that the schema for empty scopes is incorrectly storing a string instead of an empty array. I updated the submit configuration form to properly save an array to configuration, but left the other fixes in place for backwards compatibility.

I'm going to create a followup issue for the other provided clients to get those saving the configuration correctly.

If someone has time to review my changes, that'd be great!

It took me a while to review this, but I think I've unraveled everything. Unfortunately, I disagree with most of the fix in the MR. I think it will be easiest for me to write out my thoughts here rather than to only leave comments on the code. For what it's worth, I didn't really even look at the test. I just tried to figure out a plan for the fix.

I verified the bug and what you said the actual problem is. You're right: the scopes is set as a string in the configuration. This is what breaks implode() when loading the form.

What that tells me is that there may be bugged configuration out in the wild. So from my perspective the appropriate fix is to release an update function to fix any bugged config along with the change to the submit function. Instead, the proposed fix just works around bugged config, dealing with it at run time. This just builds technical debt and sounds fragile to me. It would be better to fix the source of the bug.

So my proposal is to create an update function. It should load the config for any generic and okta plugin instances, repair any string scopes, and resave. As far as testing goes, you should be able to add examples of these bugged configs to the fixture that was committed earlier in the week. I don't think this bug needs to be tested in isolation from other updates.

I'm not entirely sure what to tell you about the deprecation of the getClientScopes() return type. I was kind of confused by it, partly because the deprecation notice was added to a specific plugin instance. But if you're intent on doing this, then what you really need to do is deprecate the function at the interface. From what I can tell, deprecating a return type hint like this is very uncommon, possibly frowned upon. The deprecation policy → has this to say:

Generally a return value of method should not changed.

If it really needs to happen, then I don't know how to do it properly. Deprecation notices usually inform the code that calls the function. That isn't really a concern in this case. You control the code that calls the function. The things that really need to be informed are other plugins that implement the interface. The only other example in the deprecation policy about doing something like this so that implementing classes are notified is from the section on Interface and base/abstract class method signature changes → . You aren't doing exactly the same thing, but it's close.

I would consider whether or not the deprecation is even necessary. After all, OpenIDConnectClaims::getScopes() is the only thing that calls getClientScopes(). While it would build technical debt, you could just let getScopes() deal with NULL values, comment it thoroughly, and forget about it.

If you proceed with the deprecation, then I'd do that in a separate issue.

dcam → changed the visibility of the branch 3.x to hidden.

Since I proposed almost entirely changing the approach I'll put my money where my mouth is and write a new MR.

The current test failure is due to externalauth not being enabled in the fixture. But even when it's enabled the updates fail with an error that I couldn't figure out. I'll work on it more later.

I stripped the wrong things out of the fixture additions.

Anyway, please take a look at MR 132 and let me know what you think. If you like that solution, then I figure that you may want to integrate the GenericClientTest from MR 43. There's no reason to let that work go to waste. I just wanted to test my solution to the issue in isolation.