From 2251045901875aa815dee43ec467fb1af8d416d0 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Sat, 29 Apr 2017 19:22:50 +0200 Subject: [PATCH 1/4] WIP Signed-off-by: Thomas Citharel --- .../Controller/UserRestController.php | 98 +++++++++++++++++++ .../Resources/config/routing_rest.yml | 5 + src/Wallabag/UserBundle/Entity/User.php | 5 + 3 files changed, 108 insertions(+) create mode 100644 src/Wallabag/ApiBundle/Controller/UserRestController.php diff --git a/src/Wallabag/ApiBundle/Controller/UserRestController.php b/src/Wallabag/ApiBundle/Controller/UserRestController.php new file mode 100644 index 000000000..c5ffbdf1f --- /dev/null +++ b/src/Wallabag/ApiBundle/Controller/UserRestController.php @@ -0,0 +1,98 @@ +validateAuthentication(); + + $serializationContext = SerializationContext::create()->setGroups(['user_api']); + $json = $this->get('serializer')->serialize($this->getUser(), 'json', $serializationContext); + + return (new JsonResponse())->setJson($json); + } + + /** + * Register an user + * + * @ApiDoc( + * requirements={ + * {"name"="username", "dataType"="string", "required"=true, "description"="The user's username"}, + * {"name"="password", "dataType"="string", "required"=true, "description"="The user's password"} + * {"name"="email", "dataType"="string", "required"=true, "description"="The user's email"} + * } + * ) + * @return JsonResponse + */ + // TODO : Make this method (or the whole API) accessible only through https + public function putUserAction($username, $password, $email) + { + if (!$this->container->getParameter('fosuser_registration')) { + $json = $this->get('serializer')->serialize(['error' => "Server doesn't allow registrations"], 'json'); + return (new JsonResponse())->setJson($json)->setStatusCode(403); + } + + if ($password === '') { // TODO : might be a good idea to enforce restrictions here + $json = $this->get('serializer')->serialize(['error' => 'Password is blank'], 'json'); + return (new JsonResponse())->setJson($json)->setStatusCode(400); + } + + + // TODO : Make only one call to database by using a custom repository method + if ($this->getDoctrine() + ->getRepository('WallabagUserBundle:User') + ->findOneByUserName($username)) { + $json = $this->get('serializer')->serialize(['error' => 'Username is already taken'], 'json'); + return (new JsonResponse())->setJson($json)->setStatusCode(409); + } + + if ($this->getDoctrine() + ->getRepository('WallabagUserBundle:User') + ->findOneByEmail($email)) { + $json = $this->get('serializer')->serialize(['error' => 'An account with this email already exists'], 'json'); + return (new JsonResponse())->setJson($json)->setStatusCode(409); + } + + $em = $this->get('doctrine.orm.entity_manager'); + + $userManager = $this->get('fos_user.user_manager'); + $user = $userManager->createUser(); + + $user->setUsername($username); + + $user->setPlainPassword($password); + + $user->setEmail($email); + + $user->setEnabled(true); + $user->addRole('ROLE_USER'); + + $em->persist($user); + + // dispatch a created event so the associated config will be created + $event = new UserEvent($user); + $this->get('event_dispatcher')->dispatch(FOSUserEvents::USER_CREATED, $event); + + $serializationContext = SerializationContext::create()->setGroups(['user_api']); + $json = $this->get('serializer')->serialize($user, 'json', $serializationContext); + + return (new JsonResponse())->setJson($json); + + } + +} diff --git a/src/Wallabag/ApiBundle/Resources/config/routing_rest.yml b/src/Wallabag/ApiBundle/Resources/config/routing_rest.yml index 57d37f4b4..c0283e71f 100644 --- a/src/Wallabag/ApiBundle/Resources/config/routing_rest.yml +++ b/src/Wallabag/ApiBundle/Resources/config/routing_rest.yml @@ -17,3 +17,8 @@ misc: type: rest resource: "WallabagApiBundle:WallabagRest" name_prefix: api_ + +user: + type: rest + resource: "WallabagApiBundle:UserRest" + name_prefix: api_ diff --git a/src/Wallabag/UserBundle/Entity/User.php b/src/Wallabag/UserBundle/Entity/User.php index 3a167de74..1863c966f 100644 --- a/src/Wallabag/UserBundle/Entity/User.php +++ b/src/Wallabag/UserBundle/Entity/User.php @@ -4,6 +4,7 @@ namespace Wallabag\UserBundle\Entity; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\Mapping as ORM; +use JMS\Serializer\Annotation\Groups; use Scheb\TwoFactorBundle\Model\Email\TwoFactorInterface; use Scheb\TwoFactorBundle\Model\TrustedComputerInterface; use FOS\UserBundle\Model\User as BaseUser; @@ -35,6 +36,7 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf * @ORM\Column(name="id", type="integer") * @ORM\Id * @ORM\GeneratedValue(strategy="AUTO") + * @Groups({"user_api"}) */ protected $id; @@ -42,6 +44,7 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf * @var string * * @ORM\Column(name="name", type="text", nullable=true) + * @Groups({"user_api"}) */ protected $name; @@ -49,6 +52,7 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf * @var date * * @ORM\Column(name="created_at", type="datetime") + * @Groups({"user_api"}) */ protected $createdAt; @@ -56,6 +60,7 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf * @var date * * @ORM\Column(name="updated_at", type="datetime") + * @Groups({"user_api"}) */ protected $updatedAt; From 5709ecb36809fb009446a11a758232bbe8f264e4 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 07:56:01 +0200 Subject: [PATCH 2/4] Re-use `NewUserType` to validate registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only ugly things is how we handle error by generating the view and then parse the content to retrieve all errors… Fix exposition fields in User entity --- .../Controller/UserRestController.php | 169 +++++++++++------- src/Wallabag/UserBundle/Entity/User.php | 25 ++- .../Controller/UserRestControllerTest.php | 98 ++++++++++ .../ApiBundle/WallabagApiTestCase.php | 2 +- 4 files changed, 225 insertions(+), 69 deletions(-) create mode 100644 tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php diff --git a/src/Wallabag/ApiBundle/Controller/UserRestController.php b/src/Wallabag/ApiBundle/Controller/UserRestController.php index c5ffbdf1f..a1b78e3ff 100644 --- a/src/Wallabag/ApiBundle/Controller/UserRestController.php +++ b/src/Wallabag/ApiBundle/Controller/UserRestController.php @@ -6,12 +6,14 @@ use FOS\UserBundle\Event\UserEvent; use FOS\UserBundle\FOSUserEvents; use JMS\Serializer\SerializationContext; use Nelmio\ApiDocBundle\Annotation\ApiDoc; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\JsonResponse; +use Wallabag\UserBundle\Entity\User; class UserRestController extends WallabagRestController { /** - * Retrieve user informations + * Retrieve current logged in user informations. * * @ApiDoc() * @@ -21,78 +23,117 @@ class UserRestController extends WallabagRestController { $this->validateAuthentication(); - $serializationContext = SerializationContext::create()->setGroups(['user_api']); - $json = $this->get('serializer')->serialize($this->getUser(), 'json', $serializationContext); + return $this->sendUser($this->getUser()); + } + + /** + * Register an user. + * + * @ApiDoc( + * requirements={ + * {"name"="username", "dataType"="string", "required"=true, "description"="The user's username"}, + * {"name"="password", "dataType"="string", "required"=true, "description"="The user's password"}, + * {"name"="email", "dataType"="string", "required"=true, "description"="The user's email"} + * } + * ) + * + * @todo Make this method (or the whole API) accessible only through https + * + * @return JsonResponse + */ + public function putUserAction(Request $request) + { + if (!$this->container->getParameter('fosuser_registration')) { + $json = $this->get('serializer')->serialize(['error' => "Server doesn't allow registrations"], 'json'); + + return (new JsonResponse())->setJson($json)->setStatusCode(403); + } + + $userManager = $this->get('fos_user.user_manager'); + $user = $userManager->createUser(); + // enable created user by default + $user->setEnabled(true); + + $form = $this->createForm('Wallabag\UserBundle\Form\NewUserType', $user, [ + 'csrf_protection' => false, + ]); + + // simulate form submission + $form->submit([ + 'username' => $request->request->get('username'), + 'plainPassword' => [ + 'first' => $request->request->get('password'), + 'second' => $request->request->get('password'), + ], + 'email' => $request->request->get('email'), + ]); + + if ($form->isSubmitted() && false === $form->isValid()) { + $view = $this->view($form, 400); + $view->setFormat('json'); + + // handle errors in a more beautiful way than the default view + $data = json_decode($this->handleView($view)->getContent(), true)['children']; + $errors = []; + + if (isset($data['username']['errors'])) { + $errors['username'] = $this->translateErrors($data['username']['errors']); + } + + if (isset($data['email']['errors'])) { + $errors['email'] = $this->translateErrors($data['email']['errors']); + } + + if (isset($data['plainPassword']['children']['first']['errors'])) { + $errors['password'] = $this->translateErrors($data['plainPassword']['children']['first']['errors']); + } + + $json = $this->get('serializer')->serialize(['error' => $errors], 'json'); + + return (new JsonResponse())->setJson($json)->setStatusCode(400); + } + + $userManager->updateUser($user); + + // dispatch a created event so the associated config will be created + $event = new UserEvent($user, $request); + $this->get('event_dispatcher')->dispatch(FOSUserEvents::USER_CREATED, $event); + + return $this->sendUser($user); + } + + /** + * Send user response. + * + * @param User $user + * + * @return JsonResponse + */ + private function sendUser(User $user) + { + $json = $this->get('serializer')->serialize( + $user, + 'json', + SerializationContext::create()->setGroups(['user_api']) + ); return (new JsonResponse())->setJson($json); } /** - * Register an user + * Translate errors message. * - * @ApiDoc( - * requirements={ - * {"name"="username", "dataType"="string", "required"=true, "description"="The user's username"}, - * {"name"="password", "dataType"="string", "required"=true, "description"="The user's password"} - * {"name"="email", "dataType"="string", "required"=true, "description"="The user's email"} - * } - * ) - * @return JsonResponse + * @param array $errors + * + * @return array */ - // TODO : Make this method (or the whole API) accessible only through https - public function putUserAction($username, $password, $email) + private function translateErrors($errors) { - if (!$this->container->getParameter('fosuser_registration')) { - $json = $this->get('serializer')->serialize(['error' => "Server doesn't allow registrations"], 'json'); - return (new JsonResponse())->setJson($json)->setStatusCode(403); + $translatedErrors = []; + foreach ($errors as $error) { + $translatedErrors[] = $this->get('translator')->trans($error); } - if ($password === '') { // TODO : might be a good idea to enforce restrictions here - $json = $this->get('serializer')->serialize(['error' => 'Password is blank'], 'json'); - return (new JsonResponse())->setJson($json)->setStatusCode(400); - } - - - // TODO : Make only one call to database by using a custom repository method - if ($this->getDoctrine() - ->getRepository('WallabagUserBundle:User') - ->findOneByUserName($username)) { - $json = $this->get('serializer')->serialize(['error' => 'Username is already taken'], 'json'); - return (new JsonResponse())->setJson($json)->setStatusCode(409); - } - - if ($this->getDoctrine() - ->getRepository('WallabagUserBundle:User') - ->findOneByEmail($email)) { - $json = $this->get('serializer')->serialize(['error' => 'An account with this email already exists'], 'json'); - return (new JsonResponse())->setJson($json)->setStatusCode(409); - } - - $em = $this->get('doctrine.orm.entity_manager'); - - $userManager = $this->get('fos_user.user_manager'); - $user = $userManager->createUser(); - - $user->setUsername($username); - - $user->setPlainPassword($password); - - $user->setEmail($email); - - $user->setEnabled(true); - $user->addRole('ROLE_USER'); - - $em->persist($user); - - // dispatch a created event so the associated config will be created - $event = new UserEvent($user); - $this->get('event_dispatcher')->dispatch(FOSUserEvents::USER_CREATED, $event); - - $serializationContext = SerializationContext::create()->setGroups(['user_api']); - $json = $this->get('serializer')->serialize($user, 'json', $serializationContext); - - return (new JsonResponse())->setJson($json); - + return $translatedErrors; } - } diff --git a/src/Wallabag/UserBundle/Entity/User.php b/src/Wallabag/UserBundle/Entity/User.php index 1863c966f..1ff3046a8 100644 --- a/src/Wallabag/UserBundle/Entity/User.php +++ b/src/Wallabag/UserBundle/Entity/User.php @@ -5,11 +5,10 @@ namespace Wallabag\UserBundle\Entity; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\Mapping as ORM; use JMS\Serializer\Annotation\Groups; +use JMS\Serializer\Annotation\XmlRoot; use Scheb\TwoFactorBundle\Model\Email\TwoFactorInterface; use Scheb\TwoFactorBundle\Model\TrustedComputerInterface; use FOS\UserBundle\Model\User as BaseUser; -use JMS\Serializer\Annotation\ExclusionPolicy; -use JMS\Serializer\Annotation\Expose; use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity; use Symfony\Component\Security\Core\User\UserInterface; use Wallabag\ApiBundle\Entity\Client; @@ -19,23 +18,24 @@ use Wallabag\CoreBundle\Entity\Entry; /** * User. * + * @XmlRoot("user") * @ORM\Entity(repositoryClass="Wallabag\UserBundle\Repository\UserRepository") * @ORM\Table(name="`user`") * @ORM\HasLifecycleCallbacks() - * @ExclusionPolicy("all") * * @UniqueEntity("email") * @UniqueEntity("username") */ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterface { + /** @Serializer\XmlAttribute */ /** * @var int * - * @Expose * @ORM\Column(name="id", type="integer") * @ORM\Id * @ORM\GeneratedValue(strategy="AUTO") + * * @Groups({"user_api"}) */ protected $id; @@ -44,14 +44,30 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf * @var string * * @ORM\Column(name="name", type="text", nullable=true) + * * @Groups({"user_api"}) */ protected $name; + /** + * @var string + * + * @Groups({"user_api"}) + */ + protected $username; + + /** + * @var string + * + * @Groups({"user_api"}) + */ + protected $email; + /** * @var date * * @ORM\Column(name="created_at", type="datetime") + * * @Groups({"user_api"}) */ protected $createdAt; @@ -60,6 +76,7 @@ class User extends BaseUser implements TwoFactorInterface, TrustedComputerInterf * @var date * * @ORM\Column(name="updated_at", type="datetime") + * * @Groups({"user_api"}) */ protected $updatedAt; diff --git a/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php b/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php new file mode 100644 index 000000000..21d59a169 --- /dev/null +++ b/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php @@ -0,0 +1,98 @@ +client->request('GET', '/api/user.json'); + $this->assertEquals(200, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertArrayHasKey('id', $content); + $this->assertArrayHasKey('email', $content); + $this->assertArrayHasKey('name', $content); + $this->assertArrayHasKey('username', $content); + $this->assertArrayHasKey('created_at', $content); + $this->assertArrayHasKey('updated_at', $content); + + $this->assertEquals('bigboss@wallabag.org', $content['email']); + $this->assertEquals('Big boss', $content['name']); + $this->assertEquals('admin', $content['username']); + + $this->assertEquals('application/json', $this->client->getResponse()->headers->get('Content-Type')); + } + + public function testCreateNewUser() + { + $this->client->request('PUT', '/api/user.json', [ + 'username' => 'google', + 'password' => 'googlegoogle', + 'email' => 'wallabag@google.com', + ]); + + $this->assertEquals(200, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertArrayHasKey('id', $content); + $this->assertArrayHasKey('email', $content); + $this->assertArrayHasKey('username', $content); + $this->assertArrayHasKey('created_at', $content); + $this->assertArrayHasKey('updated_at', $content); + + $this->assertEquals('wallabag@google.com', $content['email']); + $this->assertEquals('google', $content['username']); + + $this->assertEquals('application/json', $this->client->getResponse()->headers->get('Content-Type')); + } + + public function testCreateNewUserWithExistingEmail() + { + $this->client->request('PUT', '/api/user.json', [ + 'username' => 'google', + 'password' => 'googlegoogle', + 'email' => 'bigboss@wallabag.org', + ]); + + $this->assertEquals(400, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertArrayHasKey('error', $content); + $this->assertArrayHasKey('username', $content['error']); + $this->assertArrayHasKey('email', $content['error']); + + // $this->assertEquals('fos_user.username.already_used', $content['error']['username'][0]); + // $this->assertEquals('fos_user.email.already_used', $content['error']['email'][0]); + // This shouldn't be translated ... + $this->assertEquals('This value is already used.', $content['error']['username'][0]); + $this->assertEquals('This value is already used.', $content['error']['email'][0]); + + $this->assertEquals('application/json', $this->client->getResponse()->headers->get('Content-Type')); + } + + public function testCreateNewUserWithTooShortPassword() + { + $this->client->request('PUT', '/api/user.json', [ + 'username' => 'facebook', + 'password' => 'face', + 'email' => 'facebook@wallabag.org', + ]); + + $this->assertEquals(400, $this->client->getResponse()->getStatusCode()); + + $content = json_decode($this->client->getResponse()->getContent(), true); + + $this->assertArrayHasKey('error', $content); + $this->assertArrayHasKey('password', $content['error']); + + $this->assertEquals('validator.password_too_short', $content['error']['password'][0]); + + $this->assertEquals('application/json', $this->client->getResponse()->headers->get('Content-Type')); + } +} diff --git a/tests/Wallabag/ApiBundle/WallabagApiTestCase.php b/tests/Wallabag/ApiBundle/WallabagApiTestCase.php index cf9b33471..a67655c86 100644 --- a/tests/Wallabag/ApiBundle/WallabagApiTestCase.php +++ b/tests/Wallabag/ApiBundle/WallabagApiTestCase.php @@ -37,7 +37,7 @@ abstract class WallabagApiTestCase extends WebTestCase $firewallName = $container->getParameter('fos_user.firewall_name'); $this->user = $userManager->findUserBy(['username' => 'admin']); - $loginManager->loginUser($firewallName, $this->user); + $loginManager->logInUser($firewallName, $this->user); // save the login token into the session and put it in a cookie $container->get('session')->set('_security_'.$firewallName, serialize($container->get('security.token_storage')->getToken())); From d069bff4f606be75c47a3415f2a5af13d6f04865 Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 07:56:41 +0200 Subject: [PATCH 3/4] Remove unknown validation_groups The Profile validation_groups does not exist and then for validation to be skipped (like password length) --- src/Wallabag/UserBundle/Controller/ManageController.php | 4 +--- tests/Wallabag/UserBundle/Controller/ManageControllerTest.php | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/Wallabag/UserBundle/Controller/ManageController.php b/src/Wallabag/UserBundle/Controller/ManageController.php index 1c5c86d45..084f2c67e 100644 --- a/src/Wallabag/UserBundle/Controller/ManageController.php +++ b/src/Wallabag/UserBundle/Controller/ManageController.php @@ -33,9 +33,7 @@ class ManageController extends Controller // enable created user by default $user->setEnabled(true); - $form = $this->createForm('Wallabag\UserBundle\Form\NewUserType', $user, [ - 'validation_groups' => ['Profile'], - ]); + $form = $this->createForm('Wallabag\UserBundle\Form\NewUserType', $user); $form->handleRequest($request); if ($form->isSubmitted() && $form->isValid()) { diff --git a/tests/Wallabag/UserBundle/Controller/ManageControllerTest.php b/tests/Wallabag/UserBundle/Controller/ManageControllerTest.php index 44b9a0301..b46256a6e 100644 --- a/tests/Wallabag/UserBundle/Controller/ManageControllerTest.php +++ b/tests/Wallabag/UserBundle/Controller/ManageControllerTest.php @@ -30,8 +30,8 @@ class ManageControllerTest extends WallabagCoreTestCase $form = $crawler->selectButton('user.form.save')->form(array( 'new_user[username]' => 'test_user', 'new_user[email]' => 'test@test.io', - 'new_user[plainPassword][first]' => 'test', - 'new_user[plainPassword][second]' => 'test', + 'new_user[plainPassword][first]' => 'testtest', + 'new_user[plainPassword][second]' => 'testtest', )); $client->submit($form); From fe6461e4aaff5aa2fd846492e3abd9ea38c07a5b Mon Sep 17 00:00:00 2001 From: Jeremy Benoist Date: Tue, 30 May 2017 09:57:57 +0200 Subject: [PATCH 4/4] Avoid side effect on other tests --- .../Controller/UserRestControllerTest.php | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php b/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php index 21d59a169..3f4969a53 100644 --- a/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php +++ b/tests/Wallabag/ApiBundle/Controller/UserRestControllerTest.php @@ -49,12 +49,24 @@ class UserRestControllerTest extends WallabagApiTestCase $this->assertEquals('google', $content['username']); $this->assertEquals('application/json', $this->client->getResponse()->headers->get('Content-Type')); + + // remove the created user to avoid side effect on other tests + // @todo remove these lines when test will be isolated + $em = $this->client->getContainer()->get('doctrine.orm.entity_manager'); + + $query = $em->createQuery('DELETE FROM Wallabag\CoreBundle\Entity\Config c WHERE c.user = :user_id'); + $query->setParameter('user_id', $content['id']); + $query->execute(); + + $query = $em->createQuery('DELETE FROM Wallabag\UserBundle\Entity\User u WHERE u.id = :id'); + $query->setParameter('id', $content['id']); + $query->execute(); } public function testCreateNewUserWithExistingEmail() { $this->client->request('PUT', '/api/user.json', [ - 'username' => 'google', + 'username' => 'admin', 'password' => 'googlegoogle', 'email' => 'bigboss@wallabag.org', ]);