Verify the GSSP response is valid #307
Conversation
MKodde
force-pushed
the
feature/did-set-subject
branch
4 times, most recently
from
a7a5533 to
8dfe2c1
Compare
| { | ||
| $currentSubject = $this->get('subject'); | ||
| if (!empty($currentSubject)) { | ||
| if (strtolower($currentSubject) !== strtolower($subject)) { |
There was a problem hiding this comment.
Indention level can 1 less by using &&
There was a problem hiding this comment.
Thanks, that was a rookie mistake
| @@ -75,6 +75,17 @@ public function sendSecondFactorVerificationAuthnRequest( | |||
| $originalRequestId = $this->responseContext->getInResponseTo(); | |||
| } | |||
|
|
|||
| $subject = $stateHandler->getSubject(); | |||
| if (!empty($subject) && strtolower($subjectNameId) !== strtolower($subject)) { | |||
| */ | ||
| /** @var ResponseValidator */ | ||
| private $responseValidator; | ||
|
|
||
| public function __construct( | ||
| SamlAuthenticationLogger $samlLogger, | ||
| LoaResolutionService $loaResolutionService, |
There was a problem hiding this comment.
Candidate for constructor property promotion
There was a problem hiding this comment.
Yes, when we are upgrading to php8 we surely will do that. For now this code must be PHP72 compat.
| @@ -105,6 +102,8 @@ public function respond(ResponseContext $responseContext) | |||
| } | |||
|
|
|||
| $secondFactor = $this->secondFactorService->findByUuid($selectedSecondFactorUuid); | |||
| $this->responseValidator->validate($request, $secondFactor, $responseContext->getIdentityNameId()); | |||
|
|
|||
There was a problem hiding this comment.
When do we use an extra empty line, is that personal preference?
There was a problem hiding this comment.
No hard requirements here. Only to not have a newline between the last closing parenthesis' of a PHP Class
}
}
Yields: ERROR | [x] The closing brace for the class must go on the next line after the body
In this case I wanted to emphasize the validation method.
| public function __construct( | ||
| SecondFactorTypeService $secondFactorTypeService, | ||
| ProviderRepository $providerRepository, | ||
| PostBinding $postBinding |
There was a problem hiding this comment.
Candidate for constructor property promotion
| */ | ||
| public function validate(Request $request, SecondFactor $secondFactor, string $nameIdFromState) | ||
| { | ||
| $secondFactorType = new SecondFactorType($secondFactor->secondFactorType); |
There was a problem hiding this comment.
Hard coupling, maybe use a factory?
There was a problem hiding this comment.
I think this is acceptable here. Given it's a (very) simple ValueObject.
MKodde
force-pushed
the
feature/did-set-subject
branch
from
8dfe2c1 to
2ea1031
Compare
MKodde
force-pushed
the
feature/did-set-subject
branch
from
2ea1031 to
ba05073
Compare
| $provider->getRemoteIdentityProvider(), | ||
| $provider->getServiceProvider() | ||
| ); | ||
| $nameIdFromResponse = $samlResponse->getNameId()->getValue(); |
There was a problem hiding this comment.
Peer review: rename to subjectNameId from response?
There was a problem hiding this comment.
renamed to: subjectNameIdFromResponse
|
|
||
| use RuntimeException as BaseRuntimeException; | ||
|
|
||
| class RuntimeException extends BaseRuntimeException |
There was a problem hiding this comment.
Peer review feedback: Why not give this a more descriptive name?
There was a problem hiding this comment.
I ended up renaming this Exception to: ReceivedInvalidSubjectNameIdException
| use Surfnet\StepupGateway\SecondFactorOnlyBundle\Exception\RuntimeException; | ||
| use Symfony\Component\HttpFoundation\Request; | ||
|
|
||
| class ResponseValidator |
There was a problem hiding this comment.
Peer review feedback: decouple this?
1. To check the preconditions 2. To verify that the Subject NameID matches the SecondFactor Identifier used to start the authentication.
MKodde
force-pushed
the
feature/did-set-subject
branch
from
ba05073 to
d20d7d3
Compare
Improve the GSSP response handling. The response is now actually read from the POST vars, to verify the preconditions. In addition we verify if the Subject NameID from the response matches with the one found in the AuthNRequest.