SSO Overhaul, Authentication Extenders #2059
Conversation
askvortsov1
changed the title
The nomenclature is pretty simple for me at least. SSO is Representative of the whole sign in experience through 3rd party companies/services Auth to me at least describes any way of logging in to the system (AKA are you authorized to access this resource) External auth, I think is something we can get rid of, either that or we need to replace SSO with it depending on if we want to be technically accurate or we want it to be user friendly (SSO being technically accurate) When it comes to driver vs provider the way I see it is that the driver is the implementation between flarum and the 3rd party company/service. Provider to me at least means the 3rd party company/service itself.
The flow looks correct to me, but I would have to test it myself to get a full understanding of how it works.
Yes, I'd rather not let people break their site, even if admins can bypass it (which it appears they can?) I just think it would create a support headache if we don't disable the slider if there are no SSO provider enabled, further I think that the slider should automatically slide to be turned off if the user disables all of the 3rd party providers. Also before you go and update all the bundled 3rd party providers to in theory support this system I would recommend you wait until a decision is made on #2006 |
| @@ -98,6 +98,12 @@ public function handle(Request $request): ResponseInterface | |||
| } | |||
| } | |||
|
|
|||
| if (!$actor->isGuest() && in_array($driverId, $actor->linkedProviders())) { | |||
| // This user has already linked an account from this provider. We don't currently support multiple | |||
There was a problem hiding this comment.
Is this because we're still linking providers by email address? Personally I would prefer to see that the PR also includes a switch to identify users via a relationship between the Flarum user ID and the provider user ID with email strictly as a backup during the transition period.
There was a problem hiding this comment.
Not exactly. The actual "linkage" is done via an identifier that the SsoDriver would return back (as part of the SsoResponse wrapper class). That identifier COULD be an email, but it could also be any other persistent id.
Email Address is still used to identify if an incoming ssoResponse might be linked to an existing user (which we should keep imo). However, this PR also adds support to linking accounts that don't necessarily share an email (see line 124).
That means that theoretically, a user could link an account that doesn't share an email with their flarum account through their user portal, log out, and log in (same provider, different account on provider, where that account shares an email with their flarum account). Without this check, that new account would also be linked, and now they'd have multiple accounts on one provider linking to one flarum account.
We could allow this, but under the UI in my PR, there isn't a mechanism to unlink a specific identifier from a provider, nor to add another identifier from the same provider.
This might be a feature, not a bug: the system should still function properly with this, and I can't think of security issues it could cause. If it's something we want to support, I can remove this check.
There was a problem hiding this comment.
I think for simplicity for now we can wait on multi-identity single provider sso until after stable. I just wanted to verify how this is actually implemented so that we had the option to do so easily in the future.
I agree with this interpretation, I'll standardize this. I'm also going to rename LoginProvider (and the associated table) to SsoProvider, as that more accurately describes what it does (username password login doesn't use LoginProviders afaik.
Done.
One of my main reasons to do this is to give the core dev team an easier way to test the new functionality when reviewing this PR. Besides, even if the extensions are getting moved to FoF, might as well make their lives easier too :D |
askvortsov1
changed the title
| style += providers[provider].buttonColor ? `background-color: ${providers[provider].buttonColor};` : ''; | ||
| style += providers[provider].buttonTextColor ? `color: ${providers[provider].buttonTextColor}` : ''; | ||
| items.add(provider, | ||
| <LogInButton |
There was a problem hiding this comment.
I have not really seen how it looks like for extensions after this change, but I feel they loose some control over their login buttons. They are restricted to only the attributes hard-coded here but might want to customize others.
There was a problem hiding this comment.
That's true, I wanted to make it easier for extensions, and these are the settings that every auth extension uses so far. If extensions wanted to go beyond this, couldn't they just extend the LogIn Buttons itemlist?
|
@askvortsov1 you are on fire :D I just wish that Flarum team would be faster in merging and releasing new versions... |
askvortsov1
force-pushed
the
as/auth_refactor_and_extender
branch
2 times, most recently
from
bb2dac2
to
1ced410
Compare
The getEnabled method returns all extensions (previously) enabled, yet manually uninstalled through composer. This does not reference the exact, current state of the forum. getEnabledExtensions returns a list where the getEnabled list is filtered on the extensions found in the composer installed.json file.
I could not come up with a noun for the new "UpdateTest" for users, so this is easier in terms of consistency.
Forces notifications into a dedicated SendNotificationsJob and passed to the queue. - One static method re-used in the job ::getAttributes, is that okay or use a trait? - Do we want to use this solution and refactor into a better Hub after stable, postpone this implementation or use it in b11?
This separates sending each individual mail, thus hardening the app. There are still many improvements possible in this code, e.g. chaining these commands, making emails just another notification type and listening to the Notify event instead. We can postpone this to a later stable release.
As discussed with @luceos, let's add this once the use case comes up. It might be a left-over from a previous state of this PR anyway.
This gives extension authors time to add the new `getAttributes()` method to their `BlueprintInterface` implementations. The layer itself is easy to remove in beta.14.
Notifications into the queue
This has been deprecated and removed from the contract for a long time, and it will be completely dropped in v5.8, our next upgrade target.
We need to get rid of this god class, as Laravel's Application contract gets even bigger with 5.8. To avoid having to add all these methods, we should try to stop using it where we can.
…l and its disabled while all sso drivers are disabled. Also, fixed switch state not working with both '1' and true while being off with '0' and 'false
…ation into the view method
…ndicator when linking if sso window closed)
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. |
|
This isn't getting reviewed anytime soon, but no reason to close it either. |
|
why this stuff stopped |
Fixes #2043
Fixes Part of #1891
Fixes flarum/issue-archive#380
Fixes flarum/issue-archive#236
Fixes flarum/issue-archive#332
Fixes flarum/issue-archive#223
Fixes flarum/issue-archive#285
Changes proposed in this pull request:
TO ADD AFTER STABLE
Reviewers should focus on:
Screencasts
Disable username/password auth:
https://youtu.be/kN4XT8BwUIQ
Disabling all sso providers force enables user/pass auth
https://youtu.be/OvS1jxHj5QQ
Disabling signup:
https://youtu.be/TnwrDFgCqnM
Auth Panel controls buttons, Registration works
https://youtu.be/EQchPk7oQuc
Linking/unlinking accounts works
https://youtu.be/teHYWFbTzQY
Can't unlink last provider if username/password auth disabled
https://youtu.be/YcMkBNWORgY
SSO registration respects disabled signups:
https://youtu.be/UCYJWyJB07I
Confirmed
composer test).