search

krzysiekpiasecki / Gentelella

Welcome to Gentelella - Responsive Bootstrap Admin Application based on the Foundation of Symfony and Gentelella!
http://gentelella.herokuapp.com
MIT License
100 stars 54 forks source link

Added feature: User registration #54

Closed MedUnes closed 6 years ago

MedUnes commented 7 years ago

As asked in this PR, I tried to develop a registration process.

The main idea is to override the SecurityController and RegistrationController, which needs first the AppBundle.php file to be changed so that the class extend the FOSUserBundle class.

Next, I made the login route hold both login and registration logic. For this I changed the SecurityController to pass a RegistrationForm instance to the registration view and made changes to the RegistrationController to pass necessary variables to the view

For further explanations, check comments in the changed files

MedUnes commented 7 years ago

@krzysiekpiasecki , this is relative, depending on the roadmap and the purpose behind the application.

If you aim to build a real dashboard manager, then yes security matters and in this case we might be obliged to develop some additional frontends

If you aim to simply demonstrate how to integrate the template into Symfony's ecosystem, then I do not think the security would matter as the final implementation will care of it.

These are my humble thoughts.

krzysiekpiasecki commented 7 years ago

If you aim to simply demonstrate how to integrate the template into Symfony's ecosystem, then I do not think the security would matter as the final implementation will care of it.

It's a little startup, so security matters. You wrote a registration function for application users, but they shouldn't have a ROLE_ADMIN as a default.

If you aim to simply demonstrate how to integrate the template into Symfony's ecosystem, then I do not think the security would matter as the final implementation will care of it.

Even examples should be secure as possible. Now, we can omit this problem and setting eventually a new issue to quicker publish this PR or ... adding an another dashboard, not under /admin and then forwarding users - not admins - to this page simulating, an user interface.

MedUnes commented 7 years ago

Well, you have to put in consideration that a dashboard is meant for admins, not normal users, so granting admin roles to subscribers would be ok and natural. If you want to seperate normal users and admin users, and redirect the first category to a "simulated dashboard", think about the real permissions they will have access to: if they'll share the same access as the admin ones, then no meaning of this seperation.

The other scenario would be to make signups halted and waiting for approval, this will make sense i.e the super-admin grants admin roles for admins.

krzysiekpiasecki commented 7 years ago

Well, you have to put in consideration that a dashboard is meant for admins, not normal users, so granting admin roles to subscribers would be ok and natural.

Yes, but it shouldn't be done with a common public form. Responsible for granting a ROLE_ADMIN are SUPER_ADMIN or another ROLE_ADMIN by their's declaration. Never by the application itself.

think about the real permissions they will have access to: if they'll share the same access as the admin ones, then no meaning of this seperation.

No, In general they can share others url spaces e.g. /admin only for ROLE_ADMIN the rest is for ROLE_USER.

The other scenario would be to make signups halted and waiting for approval, this will make sense i.e the super-admin grants admin roles for admins.

IMO it's the best solution. Registering a user with a ROLE_USER and not activated yet. This solution doesn't break the application flow or put DOMAIN logic into it (registering as when), but successfully extend the functionality thanks to your #54. What do you think?

MedUnes commented 7 years ago

So what about making registration with email confirmation and waiting for activation? for the first feature, we need an smtp service with credentials to forward emails

krzysiekpiasecki commented 7 years ago

So what about making registration with email confirmation and waiting for activation?

If you make PR 👍

for the first feature, we need an smtp service with credentials to forward emails

Can't we use parameters, which are setting up per environment at composer phase?

    mailer_transport: smtp
    mailer_host: 127.0.0.1
    mailer_user: null
    mailer_password: null

Is not enough to handle this task or am i wrong?

MedUnes commented 7 years ago

that's is enough if the host has mailer daemon, and the application won't rely on gmail or similar MSP. the credentials must include as well:

from_email:
    address: #address from which email confirmation is sent 
    sender_name: #name from which email confirmation is sent
krzysiekpiasecki commented 7 years ago

AFAIK user bundle sends an email after registration by default and just need a little configuration: https://symfony.com/doc/current/bundles/FOSUserBundle/emails.html

Setting a parameters for sending e-mail is a local configuration task when deploying (dev, prod, ...).

MedUnes commented 7 years ago

Yes that's what I meant. Now back to the admin/super-admin idea, how do you expect users being shown in order to be activated/disabled? I might think about integrating EasyAdminBundle to manage admins by super admin If you think this might break the template's GUI/styling, another alternative is to develop a minimal table to activate admins in a separated page.

krzysiekpiasecki commented 7 years ago

I might think about integrating EasyAdminBundle to manage admins by super admin If you think this might break the template's GUI/styling, another alternative is to develop a minimal table to activate admins in a separated page.

EasyAdminBundle is a dependency for handling domain use cases. Giving a ROLE for a user is always domain use case scenario. From application to application the rules will be completely different. Dependencies will vary too.

We need to keep a possible clean setup for this application.

I know with UserBundle is quite the same, but User is a part of this application stack and registering is a function of this stack.

I think we should register a new user with a role ROLE_USER, send him an email and user can confirm this registration. Nothing more at the moment. The devs will choose theirs path.

After this we can merge a PR, than we need a test, which I can handle.

MedUnes commented 6 years ago

I added the remaining features and tried to commit but there are CI errors, if you could help, thanks.

krzysiekpiasecki commented 6 years ago

but there are CI errors, if you could help, thanks.

Yes. Symfony 3.2.7 has a security issue. Give me a time.

Thanks for developing Gentelella.

MedUnes commented 6 years ago

I fixed this through #composer update, though there are others due to Symfony 3.3 compatibility/best practices

krzysiekpiasecki commented 6 years ago 
                                                                                  At this moment you can use 3.2.*. Not tested but it should work.

Sent from my BlackBerry 10 smartphone on the Orange network. From: MedUnesSent: poniedziałek, 20 listopada 2017 16:32To: krzysiekpiasecki/GentelellaReply To: krzysiekpiasecki/GentelellaCc: Krzysztof Piasecki; MentionSubject: Re: [krzysiekpiasecki/Gentelella] Added feature: User registration (#54)I fixed this through compose update, though there are others due to Symfony 3.3 compatibility

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.

{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/krzysiekpiasecki/Gentelella","title":"krzysiekpiasecki/Gentelella","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/krzysiekpiasecki/Gentelella"}},"updates":{"snippets":[{"icon":"PERSON","message":"@MedUnes in #54: I fixed this through compose update, though there are others due to Symfony 3.3 compatibility "}],"action":{"name":"View Pull Request","url":"https://github.com/krzysiekpiasecki/Gentelella/pull/54#issuecomment-345731112"}}}

MedUnes commented 6 years ago

I'm running through the same CI errors:

Remaining deprecation notices (7) Autowiring-types are deprecated since Symfony 3.3 and will be removed in 4.0. Use aliases instead for "Psr\Log\LoggerInterface": 2x 2x in AppKernel::boot Using the unquoted scalar value "!event" is deprecated since version 3.3 and will be considered as a tagged value in 4.0. You must quote it in "/home/travis/build/krzysiekpiasecki/Gentelella/app/config/config_dev.yml" on line 19: 1x 1x in AppKernel::boot Using the unquoted scalar value "!event" is deprecated since version 3.3 and will be considered as a tagged value in 4.0. You must quote it in "/home/travis/build/krzysiekpiasecki/Gentelella/app/config/config_dev.yml" on line 22: 1x 1x in AppKernel::boot Using the unquoted scalar value "!doctrine" is deprecated since version 3.3 and will be considered as a tagged value in 4.0. You must quote it in "/home/travis/build/krzysiekpiasecki/Gentelella/app/config/config_dev.yml" on line 22: 1x 1x in AppKernel::boot The "framework.trusted_proxies" configuration key has been deprecated in Symfony 3.3. Use the Request::setTrustedProxies() method in your front controller instead: 1x 1x in AppKernel::boot Symfony\Component\HttpKernel\DependencyInjection\Extension::addClassesToCompile() is deprecated since version 3.3, to be removed in 4.0: 1x 1x in AppKernel::boot The command "./vendor/bin/phpunit" exited with 1.

krzysiekpiasecki commented 6 years ago

I fixed this through #composer update, though there are others due to Symfony 3.3 compatibility/best practices

Update your fork using 3.0.2 version. The dependencies are now updated and deprecation are removed.

Make PR with a new future branch - please do not make PR into a master without tests. Thanks @MedUnes

MedUnes commented 6 years ago

I'm a bit lost here..

krzysiekpiasecki commented 6 years ago

Maybe reset fork to the last future changes, pull this repository and merge this repository changes into your fork. Create future branch. Remove this PR. Make new PR.

krzysiekpiasecki commented 6 years ago

Your changes were merged into a future branch. https://github.com/krzysiekpiasecki/Gentelella/tree/MedUnes-master.

I'm closing this PR. You can now push commits into a new future branch. When this feature will be tested it will be merged into master and new version will be released.

Thanks @MedUnes