Authentication user with login form #34

kevinjhappy · 2018-11-15T11:48:11Z

Bases of security bundle implemented, with the login form, the route is /login.
No users can connect yet

tdutrion

Use spaces rather than tab (4 spaces indentation). Your indentation is totally off everywhere.
Do not create getters and setters unless needed, and do not use setXXX but rather a verb, an intent, saying in which case this is used, encapsulating the change, hence not leaking the attributes you have.
Never ever commit comments

composer.json

src/Controller/SecurityController.php

src/Entity/User.php

src/Repository/UserRepository.php

tdutrion · 2018-11-15T14:30:37Z

src/Repository/UserRepository.php

+use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Symfony\Bridge\Doctrine\RegistryInterface;
+
+/**


Remove this, see discussion with @ayumiesan. She can help you with that

ok I see the discussion you had with @ayumiesan about using @method as annotation

src/Repository/UserRepository.php

src/Security/LoginFormAuthenticator.php

kevinjhappy · 2018-11-15T16:49:23Z

Fix your feedback, hoping I didn't miss some.
But still an issue with my route, when I access to /Login, I go to homePage, and didn't figure it out why yet

kevinjhappy · 2018-11-22T22:41:32Z

Login form is now working and complete, can you review this one please @tdutrion ?

Manage Special Sponsorships (DarkmiraTour#30) * Manage Special Sponsorships * install paramconverter, rename specialBenefit, remove setters * Create Factory for SpecialBenefit * Manage Value Objects for specialBenefit * Add RepositoryInterface * Define return type array to findAll methods * Rename speficic method in entity * last refacto change composer.lock Update default command of the nodejs container Add missing packages inside node container Update node version tag Create bucket at storage startup Add empty bucket folder Remove unused entrypoint override Class user ready to be create in database and be populate by fixture + login Form working, WIP for authenticate database user login form now working, need to launch UserFixture to set test user Added travis with php-cs-fixer Add phpmd Add phpunit Add roave security-advisories package Remove phpunit container Fix coding style Fix php-cs-fixer config error Fix coding style (DarkmiraTour#47) * Fix coding style * Remplace Twig_Environment by Twig\Environment * Fix PHPMD error (upgrade version) * Fix phpunit permission [Fixes DarkmiraTour#38] Move docker container config in docker/ Create Readme Add Dabatase specifications remove Minio configuration Change title display Add Note fix UserFixture by adding setPassword add admin User into fixture fix coding style fix coding style add Logout, add role check on menus, options and routes delete gitignore

tdutrion

Please find out why there's a ton of files that shouldn't be in your PR in that diff. Also, remove the sensio extra bundle from this PR please.

.php_cs.dist

tdutrion · 2018-11-26T06:37:48Z

config/packages/security.yaml

+
+    encoders:
+        App\Entity\User:
+            algorithm: bcrypt


Why not argon2i with libsodium?

tdutrion · 2018-11-26T06:38:03Z

config/packages/security.yaml

+    encoders:
+        App\Entity\User:
+            algorithm: bcrypt
+            cost: 10


Why not using the default cost?

No reason in particular, I wanted to use PHP default configuration and not Symfony's, I can change that

Changed to argon2i without further configuration, as default config is memory_cost: 1024 / time_cost: 2 / threads: 2

src/Controller/Login/Security.php

src/Controller/Organisation/Create.php

tdutrion · 2018-11-26T06:41:24Z

src/Controller/Organisation/Delete.php

@@ -13,7 +13,11 @@
 use Symfony\Component\Routing\RouterInterface;
 use Symfony\Component\Security\Csrf\CsrfToken;
 use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
+use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted;


Please do not use annotations and the sensio extra bundle.

Changes have been made, the extra bundle with IsGranted is gone and replaced by matching routes denied without the right role. But for my knowledge, why is it best not to use this Extra Bundle? Is it to avoid adding annotations?

kevinjhappy · 2018-11-26T10:02:25Z

@tdutrion The other files that are not part of the project come from my squashing of all my commits from the beginning, as I made 2 merge from develop during this issue, git took the changes as his own, I was surprised also to see those files included in the squashed commit, and I am also surprised there are still conflicts as I made sure I fix them all, but I was careful with my rebase using squash. I might have done it wrong then. I can revert my squashed commit and do it again if needed

Co-Authored-By: kevinjhappy <kevin.nadin@gmail.com>

…orithm to argon2i

…ent-manager into develop

tdutrion

I would probably just checkout bin/console to remove the chmod (do it in another PR if needed) and .php_cs.dist (the previous comment did not take into account the fact this file should not be there at all).

tdutrion · 2018-11-26T22:06:36Z

config/packages/security.yaml

+    # Note: Only the *first* access control that matches will be used
+
+    access_control:
+        - { path: ^/organisations/create, roles: ROLE_ADMIN }


https://symfony.com/doc/current/security.html#securing-url-patterns-access-control
You can use a pattern for multiple routes as shown in this example.

tdutrion · 2018-11-26T22:06:48Z

config/packages/security.yaml

+            # form_login: true
+            # https://symfony.com/doc/current/security/form_login_setup.html
+
+    # Easy way to control access for large sections of your site


Remove comments

Suggested change

# Easy way to control access for large sections of your site

tdutrion · 2018-11-26T22:06:57Z

config/packages/security.yaml

+            # https://symfony.com/doc/current/security/form_login_setup.html
+
+    # Easy way to control access for large sections of your site
+    # Note: Only the *first* access control that matches will be used


Suggested change

# Note: Only the *first* access control that matches will be used

tdutrion · 2018-11-26T22:07:07Z

config/packages/security.yaml

+            logout:
+                path:   app_logout
+
+            # activate different ways to authenticate


Suggested change

# activate different ways to authenticate

tdutrion · 2018-11-26T22:07:15Z

config/packages/security.yaml

+                path:   app_logout
+
+            # activate different ways to authenticate
+


Suggested change

tdutrion · 2018-11-26T22:40:24Z

src/Security/LoginFormAuthenticator.php

+
+    public function checkCredentials($credentials, UserInterface $user)
+    {
+        return $this->passwordEncoder->isPasswordValid($user, $credentials['password']);


What if credentials is not an array or $credentials['password'] does not exist or is not of the right type?

tdutrion · 2018-11-26T22:41:20Z

src/Security/LoginFormAuthenticator.php

+        return new RedirectResponse($this->router->generate('login_success'));
+    }
+
+    protected function getLoginUrl()


Why would this be protected? Is that forced by the AbstractFormLoginAuthenticator?

Yes it's the only abstract function of AbstractFormLoginAuthenticator and is with protected visibility, nevertheless, I can pass it public as this is a larger visibility

tdutrion · 2018-11-26T22:41:42Z

src/Security/LoginFormAuthenticator.php

+
+class LoginFormAuthenticator extends AbstractFormLoginAuthenticator
+{
+    use TargetPathTrait;


Please refrain from using traits.

Ok I will check that, for my knowledge is there a specific reason like a good practice or other for not using traits?

tdutrion · 2018-11-26T22:43:11Z

templates/security/login.html.twig

+
+{% block content %}
+<div class="container">
+    <form method="post">


Why don't you use symfony forms and twig form helpers?

Didn't know they existed, I made the implementation

tdutrion · 2018-11-26T22:44:15Z

templates/specialBenefit/index.html.twig

+        {% if is_granted('ROLE_ADMIN') %}
+            <div class="col col-6">
+                <p class="text-right">
+                    <a class="btn btn-primary" href="{{ path('special_benefit_create') }}">Create new</a>


Suggested change

<a class="btn btn-primary" href="{{ path('special_benefit_create') }}">Create new</a>

<a class="btn btn-primary" href="{{ path('special_benefit_create') }}">Add new benefit</a>

…ent-manager into develop

[WIP] Authentication user with login form

e58e0bb

tdutrion requested changes Nov 15, 2018

View reviewed changes

kevinjhappy changed the title ~~[WIP] Authentication user with login form~~ Authentication user with login form Nov 22, 2018

kevinjhappy changed the title ~~Authentication user with login form~~ [WIP] Authentication user with login form Nov 23, 2018

kevinjhappy changed the title ~~[WIP] Authentication user with login form~~ Authentication user with login form Nov 23, 2018

kevinjhappy force-pushed the develop branch from c0a4e1e to f610181 Compare November 23, 2018 15:30

tdutrion changed the base branch from develop to master November 26, 2018 05:42

tdutrion changed the base branch from master to develop November 26, 2018 05:42

tdutrion changed the base branch from develop to master November 26, 2018 06:34

tdutrion changed the base branch from master to develop November 26, 2018 06:34

tdutrion requested changes Nov 26, 2018

View reviewed changes

tdutrion and others added 5 commits November 26, 2018 11:06

Update .php_cs.dist

f359d2a

Co-Authored-By: kevinjhappy <kevin.nadin@gmail.com>

Update src/Controller/Login/Security.php

d023c7f

Co-Authored-By: kevinjhappy <kevin.nadin@gmail.com>

Merge branch 'develop' into develop

17fc964

Using firewall config instead of IsGranted annotations and change alg…

020e90f

…orithm to argon2i

Merge branch 'develop' of https://github.com/kevinjhappy/community-ev…

5ed4d14

…ent-manager into develop

kevinjhappy mentioned this pull request Nov 26, 2018

[PENDING] Add admin management #23

Open

Remove text editor specific configuration files

6a848bd

tdutrion requested changes Nov 26, 2018

View reviewed changes

kevinjhappy added 5 commits November 28, 2018 15:34

add UserManager and adapt UserRepository, fix control_access and routes

ddab63e

Merge branch 'develop' of https://github.com/kevinjhappy/community-ev…

bf82b24

…ent-manager into develop

Fix coding style

417a212

Fix coding style

aa645eb

Merge branch 'develop' into develop

ce154bd

cgrandval added enhancement New feature or request good first issue Good for newcomers labels Nov 30, 2018

cgrandval added the user label Nov 30, 2018

cgrandval assigned kevinjhappy Nov 30, 2018

Merge branch 'develop' into develop

f7de2af

tdutrion approved these changes Dec 10, 2018

View reviewed changes

tdutrion merged commit 8f7efb5 into DarkmiraTour:develop Dec 10, 2018

lbassin mentioned this pull request Dec 13, 2018

Authentification #24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication user with login form #34

Authentication user with login form #34

kevinjhappy commented Nov 15, 2018 •

edited by tdutrion

Loading

tdutrion left a comment

tdutrion Nov 15, 2018

kevinjhappy Nov 15, 2018 •

edited

Loading

kevinjhappy commented Nov 15, 2018

kevinjhappy commented Nov 22, 2018

tdutrion left a comment

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

kevinjhappy Nov 26, 2018

kevinjhappy Nov 26, 2018

tdutrion Nov 26, 2018

kevinjhappy Nov 26, 2018

kevinjhappy commented Nov 26, 2018

tdutrion left a comment

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

tdutrion Nov 26, 2018

kevinjhappy Nov 27, 2018 •

edited

Loading

tdutrion Nov 26, 2018

kevinjhappy Nov 27, 2018 •

edited

Loading

tdutrion Nov 26, 2018

kevinjhappy Nov 27, 2018

tdutrion Nov 26, 2018

	<a class="btn btn-primary" href="{{ path('special_benefit_create') }}">Create new</a>
	<a class="btn btn-primary" href="{{ path('special_benefit_create') }}">Add new benefit</a>

Authentication user with login form #34

Authentication user with login form #34

Conversation

kevinjhappy commented Nov 15, 2018 • edited by tdutrion Loading

tdutrion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjhappy Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

kevinjhappy commented Nov 15, 2018

kevinjhappy commented Nov 22, 2018

tdutrion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjhappy commented Nov 26, 2018

tdutrion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjhappy Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjhappy Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjhappy commented Nov 15, 2018 •

edited by tdutrion

Loading

kevinjhappy Nov 15, 2018 •

edited

Loading

kevinjhappy Nov 27, 2018 •

edited

Loading

kevinjhappy Nov 27, 2018 •

edited

Loading