SymfonyCasts stands united with the people of Ukraine
If you liked what you've learned so far, dive in!
Subscribe to get access to this tutorial plus
video, code and script downloads.
Hey! We have this great system where users are actually being authenticated! Now we can start checking for security everywhere we need it. In newAction we’re requiring that you are logged in:
// src/KnpU/CodeBattle/Controller/Api/ProgrammerController.php
// ...
public function newAction(Request $request)
{
if (!$this->isUserLoggedIn()) {
throw new AccessDeniedException();
}
// ...
}
Awesome! In showAction and listAction we are going to leave those anonymous. In updateAction, we do need some extra security. It’s more than just being logged in: we need to check to see if our user is actually the owner of that programmer or not. So we just add some if statement logic: if ($programmer->userId != $this->getLoggedInUser()->id, then throw new AccessDeniedException:
// src/KnpU/CodeBattle/Controller/Api/ProgrammerController.php
// ...
public function updateAction($nickname, Request $request)
{
$programmer = $this->getProgrammerRepository()->findOneByNickname($nickname);
if ($programmer->userId != $this->getLoggedInUser()->id) {
throw new AccessDeniedException();
}
// ...
}
Easy enough!
Since we’re also going to use this in deleteAction let’s go into our BaseController and make this a generic function. Open up the BaseController, create a new protected function enforceProgrammerOwnershipSecurity. Let’s copy the logic in there and don’t forget to add your AccessDeniedException use statement:
// src/KnpU/CodeBattle/Controller/BaseController.php
// ...
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
// ...
protected function enforceProgrammerOwnershipSecurity(Programmer $programmer)
{
if ($this->getLoggedInUser()->id != $programmer->userId) {
throw new AccessDeniedException();
}
}
Perfect, so now go back to our ProgrammerController. It’s a lot easier to just reuse this logic. Let’s also use this down in deleteAction:
// src/KnpU/CodeBattle/Controller/Api/ProgrammerController.php
// ...
public function updateAction($nickname, Request $request)
{
$programmer = $this->getProgrammerRepository()->findOneByNickname($nickname);
// ...
$this->enforceProgrammerOwnershipSecurity($programmer);
// ...
}
public function deleteAction($nickname)
{
$programmer = $this->getProgrammerRepository()->findOneByNickname($nickname);
// ...
$this->enforceProgrammerOwnershipSecurity($programmer);
// ...
}
Now the only other thing that could go wrong, is if the user is not logged in at all and they hit updateAction. Then, we would die inside this function. The problem is that $this->getLoggedInUser would be null and we’ll call the id property on a null object:
// src/KnpU/CodeBattle/Controller/BaseController.php
// ...
protected function enforceProgrammerOwnershipSecurity(Programmer $programmer)
{
if ($this->getLoggedInUser()->id != $programmer->userId) {
throw new AccessDeniedException();
}
}
So before we call this function, we need to make sure the user is at least logged in. If they aren’t, then they are definitely not the owner of this programmer.
So let’s create another function here called enforceUserSecurity. In this case, go back to ProgrammerController and grab the logic right here:
// src/KnpU/CodeBattle/Controller/BaseController.php
// ...
protected function enforceUserSecurity()
{
if (!$this->isUserLoggedIn()) {
throw new AccessDeniedException();
}
}
There we go. And from inside enforceProgrammerOwnershipSecurity we can just make sure that the user is actually logged in:
// src/KnpU/CodeBattle/Controller/BaseController.php
// ...
protected function enforceProgrammerOwnershipSecurity(Programmer $programmer)
{
$this->enforceUserSecurity();
if ($this->getLoggedInUser()->id != $programmer->userId) {
throw new AccessDeniedException();
}
}
And in ProgrammerController, we can do the same thing and save ourselves a little bit of code:
// src/KnpU/CodeBattle/Controller/Api/ProgrammerController.php
// ...
public function newAction(Request $request)
{
$this->enforceUserSecurity();
// ...
}
Between these two new methods, we have a really easy way to go function by function inside of our controller to make sure that we’re enforcing the right type of security.
Because we’re sending our Authorization header in the background of our scenarios, we should be able to run our entire programmer.feature and see it pass:
php vendor/bin/behat features/api/programmer.feature
Perfect! And just like that, we have our entire application locked down.
Yo Miroslav!
Yea, you're 100% right on that security check - it should go *inside* the `if` - exactly like you've shown. Thanks for pointing that out!
About the DELETE for a missing programmer causing a 404, it's subjective. I tend to favor your approach - returning a 404 when the programmer doesn't exist. But others say - if the programmer doesn't exist, then it's kind of "already deleted" - and they return the 204. That's what we did here, mostly to show how you can make those interesting decisions. But as far as I've seen, the standards to force one way or the other.
Cheers back!
is there any important reason I am missing for placing "enforceProgrammerOwnershipSecurity()" in the BaseController? In theory it will be only used in ProgrammerController right?
No, you're thinking correctly - there is nothing right here that makes this necessary. You could think of it as "planning ahead in case", but I usually don't centralize things until I actually need them. In this case, my aim was really to get the enforceUserSecurity method in the BaseController, so enforceProgrammerOwnerSecurity naturally went there too (bit it didn't need to).
Cheers!
// composer.json
{
"require": {
"silex/silex": "~1.0", // v1.3.2
"symfony/twig-bridge": "~2.1", // v2.7.3
"symfony/security": "~2.4", // v2.7.3
"doctrine/dbal": "^2.5.4", // v2.5.4
"monolog/monolog": "~1.7.0", // 1.7.0
"symfony/validator": "~2.4", // v2.7.3
"symfony/expression-language": "~2.4", // v2.7.3
"jms/serializer": "~0.16", // 0.16.0
"willdurand/hateoas": "~2.3" // v2.3.0
},
"require-dev": {
"behat/mink": "~1.5", // v1.5.0
"behat/mink-goutte-driver": "~1.0.9", // v1.0.9
"behat/mink-selenium2-driver": "~1.1.1", // v1.1.1
"behat/behat": "~2.5", // v2.5.5
"behat/mink-extension": "~1.2.0", // v1.2.0
"phpunit/phpunit": "~5.7.0", // 5.7.27
"guzzle/guzzle": "~3.7" // v3.9.3
}
}
Just one small correction - in the video the ownership check in the delete action is done on the wrong place. It should be:
if ($programmer) {
$this->enforceProgrammerOwnershipSecurity($programmer);
$this->delete($programmer);
}
In the way it's done, it would throw runtime error (method argument is not nullable).
Additionally I would suggest adding not found check in delete action (and keep ownership check where it is).
The behat test to be matched:
Scenario: DELETE a missing programmer
When I request "DELETE /api/programmers/DeletedUnitTester"
Then the response status code should be 404
Devil is in the details :) Not sure if it's not again the standards though...
Cheers!