Extension does not work with Premium Wordpress Extrension

[BullyPanda]

So After updating the forum to .14 and with that changing my auth (Facebook and Discord) extension with this one the forum stopped working and it looks like it's not working with prmium wordpress extension.

[dsevillamartin]

You'll have to enable the WP ext before fof/oauth. To do this easily remove the wordpress ext, disable fof/oauth, install the wordpress ext & make sure its enabled, and enable fof/oauth

[BullyPanda]

We tried it like that right now and sadly it does not work

[luceos]

As this relates to the extension by @clarkwinkelmann I think it's only fair for him to look into this and at least offer a solution, alternatively even PR or push a fix.

[clarkwinkelmann]

This seems to be a problem with much broader impact. I have been made aware of it yesterday when there was discussion related to the same issue with the Google Login extension.

The problem is that extensions have traditionally used /auth/<provider> fot their routes. 3 routes were defined by core, and third-party developers adopted the convention for their own extensions. That's what I'm doing in the Wordpress extension with /auth/wordpress and what the Google extension is doing with /auth/google.

But now in this extension, FoF OAuth defines a catch-all route /auth/{provider}, effectively shadowing existing routes defined by other auth extensions.

At least one extension will need to change something. Since FoF OAuth is breaking other existing extensions which pre-date it, I'm advocating for the change to happen in here.

The options I see:

1. We define each route explicitly and separately
2. We define a single route with a regular expression with each supported provider, something like /auth/{provider:facebook|github}. I'm not sure whether this option is actually possible and whether it solves the duplicate route error.
3. We use a prefix, like /fof-auth/{provider}

[dsevillamartin]

We use a prefix, like /fof-auth/{provider}

This would require all OAuth application settings to be updated.

We could use a middleware. I don't think our route resolver supports special syntax like provider:facebook|github but I might be wrong.

[BullyPanda]

So is someone planning anything to fix this?

[SKevo18]

I have experienced this problem too. It is also related to extension loading order (in what order the routes are defined). I'd advise OAuth to define routes explicitly and to not use wildcards like Clark suggested, as it breaks basically all other auth extensions when the loading order is wrong. I am thinking about creating a discussion to address the problem with loading order in general at Discuss, as it impacts stability of many extensions (today I experienced another loading order error with FoF Filter for example) but in short, my idea is for extensions to define some kind of priority or loading order themselves (eg. an extension can specify to force load before another extension, but that would require a core change).

[matteocontrini]

We define each route explicitly and separately

This is what I would vote for, personally. It's probably the easiest and fastest way to fix this and I don't find it a bad solution either, also considering that the Twitter route is already separated from the other ones

[askvortsov1]

If the regex works, wouldn't that be simpler?

[SKevo18]

If the regex works, wouldn't that be simpler?

/ auth\/(discord|facebook|github|gitlab|twitter) /s

(/g would accept multiple routes, such as "auth/facebookauth/github", /s is for no newline)

This is the best alternative that I've considered. Only picks up one of OAuth's actual routes. You can test it and make it better in a tool like https://regex101.com/.

[askvortsov1]

Unless I'm missing something, what you put here is working for me? https://i.imgur.com/5o5kVP1.png

[SKevo18]

Unless I'm missing something, what you put here is working for me? https://i.imgur.com/5o5kVP1.png

Yes, it is working, I am just not very experienced with RegExp, but it should only catch 1 of OAuth's routes, so other extensions have space for their routes and there is no conflict. I meant that you might have a more efficient setup or different ideas, so feel free to modify mine. But I think that it is safe to replace the current wildcard RegExp.

May I ask why did developers just put a wildcard here? It seems confusing and rather unnecessary to me, but I understand if you had to deliver the extension fast and there was no space to implement a better route system.

Edit: Is this where the routes get defined? https://github.com/FriendsOfFlarum/oauth/blob/d5eca0f3b58fcb7962c6a9b5ad3f0b460460fbf4/src/Controller.php#L24-L27

Because there is another similar function: https://github.com/FriendsOfFlarum/oauth/blob/d5eca0f3b58fcb7962c6a9b5ad3f0b460460fbf4/src/Controllers/AuthController.php#L52-L56

Those two seem to be duplicates. Where are the actual routes defined even?

Edit 2: We could just hard-code the routes, or even let admins pick their route. So, for example:

https://github.com/FriendsOfFlarum/oauth/blob/78f8b05427b509e2877938e920b8166eee560ebf/src/Providers/Discord.php#L39-L46

https://github.com/FriendsOfFlarum/oauth/blob/78f8b05427b509e2877938e920b8166eee560ebf/src/Providers/Discord.php#L44

This line could be changed either to 'redirectUri' => $this->url->to('forum')->route('auth.discord'), (never coded in PHP, so this might be wrong, but the Twitter provider is separate and does the same thing for some reason) or $this->getSetting('redirectUri'), and create a new field for redirectUri. Nothing complicated IMO, and would open admins the ability to customize the routes.

[clarkwinkelmann]

The routes are defined here

https://github.com/FriendsOfFlarum/oauth/blob/d5eca0f3b58fcb7962c6a9b5ad3f0b460460fbf4/extend.php#L45-L47

I'm guessing the reason for the catch-all route is that we don't need to update it when we add new providers. It also provides an easy way for the controller to get the provider name since the router will capture it in a request parameter.

If we register one separate route for each, we also need to edit the router so it looks for the ending of the URL. In beta 15 we will be able to read the route name itself, which will be a nice alternative, but this can't be done in beta 14.

If we use a regex, we can continue to capture the value in a named parameter. But the regex needs to work with FastRoute, the router library, and I have not had time to test this myself yet.

[BullyPanda]

With @clarkwinkelmann fix, I think it's fixed. Right now on beta.14 with dev build and it works. Thanks.