Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add hwi/oauth-bundle #395

Merged
merged 9 commits into from Jun 27, 2018

Conversation

Projects
None yet
3 participants
@andreybolonin
Copy link
Contributor

andreybolonin commented Jun 12, 2018

Q A
License MIT
@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request does not pass validation.

# redirect the user to /my/destination after facebook authenticates them. If this is not
# set then the user will be redirected to the original resource that they requested, or
# the base address if no resource was requested. This is similar to the behaviour of
# [target_path_parameter for form login](http://symfony.com/doc/2.0/cookbook/security/form_login.html).

This comment has been minimized.

@symfony-flex-server

symfony-flex-server bot Jun 12, 2018

Contributor

Use https://symfony.com when referencing the Symfony website

scope: "email"
options:
display: popup
csrf: true

This comment has been minimized.

@symfony-flex-server

symfony-flex-server bot Jun 12, 2018

Contributor

Should end with a newline


hwi_oauth_login:
resource: "@HWIOAuthBundle/Resources/config/routing/login.xml"
prefix: /login

This comment has been minimized.

@symfony-flex-server

symfony-flex-server bot Jun 12, 2018

Contributor

Should end with a newline

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request does not pass validation.

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request does not pass validation.

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request passes validation.

@Nyholm
Copy link
Member

Nyholm left a comment

It looks good. Do not mind Travis failing. It just complains that you need to install a http client.


# an optional setting to configure a query string parameter which can be used to redirect
# the user after authentication, e.g. /connect/facebook?_destination=/my/destination will
# redirect the user to /my/destination after facebook authenticates them. If this is not

This comment has been minimized.

@Nyholm

Nyholm Jun 16, 2018

Member

Do we really need all these comments and configuration also as a comment? I would like to see a link to the documentation instead.

Also, remove all "inactive" configuration

<bg=blue;fg=white> </>

* <fg=blue>Configure</> your application:
1. Enable sessions by uncommenting the entire session section in <comment>config/packages/framework.yaml</>

This comment has been minimized.

@Nyholm

Nyholm Jun 16, 2018

Member

This instruction is not really reliable. Maybe write exactly what to do here or refer to documentation.

Enable sessions by adding framework.session.enabled: true in config/packages/framework.yaml

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request does not pass validation.

@@ -0,0 +1 @@
framework.session.enabled: true

This comment has been minimized.

@symfony-flex-server

symfony-flex-server bot Jun 19, 2018

Contributor

Should end with a newline

This comment has been minimized.

@Nyholm

Nyholm Jun 22, 2018

Member

We should not modify other bundles configuration.

Please remove this.
If it is really needed, write something in the post-install.

client_id: <client_id>
client_secret: <client_secret>
options:
csrf: true

This comment has been minimized.

@symfony-flex-server

symfony-flex-server bot Jun 19, 2018

Contributor

Should end with a newline

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request passes validation.

@andreybolonin

This comment has been minimized.

Copy link
Contributor Author

andreybolonin commented Jun 19, 2018

@Nyholm done

@Nyholm
Copy link
Member

Nyholm left a comment

Excellent. Thank you.

Just a minor thing.

@@ -0,0 +1 @@
framework.session.enabled: true

This comment has been minimized.

@Nyholm

Nyholm Jun 22, 2018

Member

We should not modify other bundles configuration.

Please remove this.
If it is really needed, write something in the post-install.

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request passes validation.

@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Jun 22, 2018

Good. I like this.

Could we provide a better example? When I try to install this I get the following error.
screen shot 2018-06-22 at 18 06 34

Maybe we add "facebook" as default, What do you think?

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request passes validation.

@andreybolonin

This comment has been minimized.

Copy link
Contributor Author

andreybolonin commented Jun 23, 2018

@Nyholm yep

@@ -0,0 +1,14 @@
hwi_oauth:
# list of names of the firewalls in which this bundle is active, this setting MUST be set
firewall_names: [secured_area]

This comment has been minimized.

@Nyholm

Nyholm Jun 24, 2018

Member

I like this comment. But may I suggest to change this value to "main"? The default security config looks like:

security:
    # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
    providers:
        in_memory: { memory: ~ }
    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        main:
            anonymous: true

            # activate different ways to authenticate

            # http_basic: true
            # https://symfony.com/doc/current/security.html#a-configuring-how-your-users-will-authenticate

            # form_login: true
            # 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
    access_control:
        # - { path: ^/admin, roles: ROLE_ADMIN }
        # - { path: ^/profile, roles: ROLE_USER }

There is no firewall named "secured_area". What do you think?

@Nyholm

Nyholm approved these changes Jun 25, 2018

Copy link
Member

Nyholm left a comment

Thank you

@andreybolonin

This comment has been minimized.

Copy link
Contributor Author

andreybolonin commented Jun 25, 2018

@Nyholm thank you for review )
what about to merge?

@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Jun 26, 2018

@symfony-flex-server review please

@Nyholm Nyholm closed this Jun 26, 2018

@Nyholm Nyholm reopened this Jun 26, 2018

@Nyholm

This comment has been minimized.

Copy link
Member

Nyholm commented Jun 26, 2018

The review bot seams to be on vacation. I'll be back when it is fixed.

@andreybolonin

This comment has been minimized.

Copy link
Contributor Author

andreybolonin commented Jun 27, 2018

@symfony-flex-server review please

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jun 27, 2018

@symfony-flex-server review please (hey, that's me, be nice :))

@symfony-flex-server
Copy link
Contributor

symfony-flex-server bot left a comment

Pull request passes validation.

@symfony-flex-server symfony-flex-server bot merged commit 967fd36 into symfony:master Jun 27, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/symfony/pr Done
Details

symfony-flex-server bot added a commit that referenced this pull request Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.