Fix/sso account linking security#532
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| class SsoController extends Controller | ||
| { | ||
| private const LINK_INTENT_TTL_MINUTES = 10; |
|
|
||
| $fresh = LegacyUser::find($authUser->id); | ||
|
|
||
| if ($fresh->sso_sub !== null && $fresh->sso_sub !== $intent['sso_sub']) { |
| } | ||
|
|
||
| DB::transaction(function () use ($fresh, $intent) { | ||
| $fresh->sso_sub = $intent['sso_sub']; |
|
|
||
| DB::transaction(function () use ($fresh, $intent) { | ||
| $fresh->sso_sub = $intent['sso_sub']; | ||
| $fresh->sso_provider = $intent['sso_provider'] ?? null; |
| DB::transaction(function () use ($fresh, $intent) { | ||
| $fresh->sso_sub = $intent['sso_sub']; | ||
| $fresh->sso_provider = $intent['sso_provider'] ?? null; | ||
| $fresh->sso_id_token = $intent['sso_id_token'] ?? null; |
| * Returns null when the token is missing, malformed, or the payload is not valid JSON. | ||
| */ | ||
| protected function buildIdpLogoutUrl(?string $idpIdToken, string $redirectUri): ?string | ||
| protected function decodeIdTokenPayload(?string $idToken): ?array |
| protected function decodeIdTokenPayload(?string $idToken): ?array | ||
| { | ||
| if (! $idpIdToken) { | ||
| if (! $idToken) { |
| /** | ||
| * OIDC: the email claim is only trustworthy when email_verified is strictly true. | ||
| */ | ||
| protected function isEmailVerified(?array $idTokenPayload): bool |
| * The intent carries everything the link endpoint needs to stamp the row | ||
| * once the user has proven legacy-account possession via password. | ||
| */ | ||
| protected function storeLinkIntent(LegacyUser $emailMatch, $socialiteUser, Tenant $tenant): string |
| return $token; | ||
| } | ||
|
|
||
| protected function linkIntentCacheKey(string $token): string |
| @@ -18,8 +18,20 @@ | |||
| $jwt = new JWT($instance->jwt_key, $db, $crypt, $syslog); | |||
|
|
|||
| if ($_SERVER['REQUEST_METHOD'] === 'GET') { | |||
There was a problem hiding this comment.
praise: very nice handling and thanks for the useful inline comments
| 'password' => self::PASSWORD, | ||
| ], ['aula-instance-code' => self::INSTANCE_CODE]); | ||
|
|
||
| $response->assertOk()->assertJson(['success' => true]); |
There was a problem hiding this comment.
issue: how is this test case different from the one above? (we're not checking anything different in the assertion and the tenant is the same like above, the only diff is the email+username of the user, but sso_sub is absent in both tests)
| if ($tenant && $tenant->sso_required) { | ||
| return response()->json([ | ||
| 'success' => false, | ||
| 'error_code' => 3, |
There was a problem hiding this comment.
issue: fmpov (but let's discuss this), adding error_code and error is redundant.. i think we should decide to use only one of them to make it less confusing..
my understanding is that by using error_code in the legacy login controller you want to be interoperable with FE that works with v1 API.. if that assumption is correct, i'd argue we should force the FE to use v2 API for login and then we don't have to use error_code.. in fact, even if we extend the v1 login flow with error_code: 3, we'd still need to update FE to know how to interpret the value 3 correctly, so we might as well make it work with error slugs..
pls let me know if i got it right there..
|
|
||
| $response->assertOk(); | ||
| $response->assertJson(['success' => false, 'error_code' => 3, 'error' => 'tenant_requires_sso']); | ||
| $this->assertArrayNotHasKey('JWT', $response->json()); |
There was a problem hiding this comment.
suggestion(nitpick): not sure if this would work, but i noticed this function:
| $this->assertArrayNotHasKey('JWT', $response->json()); | |
| $response->assertJsonMissingPath('$.JWT'); |
3 participants
Context
Fix #495, #496, #497
Checklist