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

Support for sign-in & sign-up via OAuth resource owner #163

Closed
wants to merge 10 commits into from

Conversation

headrevision
Copy link
Contributor

What is an e-commerce solution without the possibility to sign-up ad-hoc via a user account from an OAuth sign-in provider (eg. Login with Amazon, wich is particularly suggesting in the case of Sylius' application domain)?

I already began to work on a reference implementation for an OAuth support in Sylius by using the HWIOAuthBundle: https://github.com/headrevision/Sylius/tree/oauth-login

(so far the OAuth sign-in is completely separated from Sylius' native sign-in: http://t.co/TCZ5tZ0s0K)

@jjanvier
Copy link
Contributor

Good idea 👍

@pjedrzejewski
Copy link
Member

@headrevision If you want to open a PR from this branch, I will be more than happy to discuss the problem. Thanks!

@headrevision
Copy link
Contributor Author

@pjedrzejewski I have changed it to a implementation that is less generic but that actually works now with Amazon.

The Behat scenarios in https://github.com/headrevision/Sylius/blob/oauth-login/features/frontend/user_login_via_amazon.feature fail during the form submit step (https://github.com/headrevision/Sylius/blob/d05a26dc679dc2c1b8d60b51e230a0351d4a21eb/features/frontend/user_login_via_amazon.feature#L20) as you get to Amazon's 404 page. In any case Amazon's login form works without JS. So it should work with Goutte. I don't know why it doesn't.

BDD-ish thinking tells us to get rid of the separate OAuth login page next (ie. add the OAuth sign-in provider buttons to the existing Sylius login page). Subsequently they should also be added to the registration page.

<attribute-override name="password">
<field name="password" column="password" nullable="true" />
</attribute-override>
</attribute-overrides>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is not good idea... If you have email of user, why not use FOSUserBundle to generate password and send it to user? =)

@pjedrzejewski ping

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stloyd I don't feel comfortable with this either. But generating a (second) password would make the possibility to sign in via a foreign account expendable. It would just serve the purpose of initially signing in for the implicit sign-up at Sylius. That is clearly not the intention behind all those "login with ..." services.

@headrevision
Copy link
Contributor Author

@pjedrzejewski Finished with integrating OAuth sign-up & sign-in on login page as well as on register page. I also wrote Behat scenarios for that. Can you please give me feedback?

@pjedrzejewski
Copy link
Member

@headrevision Thank you Fabian, I'll check it out today!

@pjedrzejewski
Copy link
Member

First quick thought - I liked your approach with separate model for OAuth accounts, any reason to abandon this idea?

@headrevision
Copy link
Contributor Author

@pjedrzejewski I would like to enable OAuth support in Sylius by default for Amazon, Google and Facebook (including configuration and DB changes) such that you can see the buttons on the login page and the register page. What do you think? (Proper integration in Twitter Bootstrap for that is still missing, it doesn't look very fancy yet)

Just discovered that logout is not working so far in the case the user used OAuth instead of native login before. What else is missing in order to make this WIP PR a ready-to-merge PR?

@headrevision
Copy link
Contributor Author

Now also Google is supported. Behat scenarios run successfully with WebDriver.

@@ -5,6 +5,8 @@
<h1>Registration <small>Create an account in store</small></h1>
</div>

{% render url('hwi_oauth_connect') %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a detail but the function way is preferred: {{ render(url(...)) }}

@marcoalbarelli
Copy link
Contributor

Hi
I'm currently starting to write an ecommerce solution
What's the status of this?
Can I be of any help?

@headrevision
Copy link
Contributor Author

@marcoalbarelli Login via Amazon and Google works and other OAuth resource owners can be added easily. However the PR is not yet merged into Sylius/Sylius.

@headrevision
Copy link
Contributor Author

@marcoalbarelli You could just try this branch. I would like to receive feedback from others if it works without any problems regarding configuration and if adding another resource owner is self-explaining. Just be aware that OAuth 2.0 requires a publicly accessible web server with SSL support.

# - sleep 5
# - app/console cache:warmup --env=test > /dev/null
# - chmod -R 777 app/cache app/logs
- curl https://gist.github.com/santiycr/5139565/raw/sauce_connect_setup.sh | bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the addon at Travis-CI: http://about.travis-ci.org/docs/user/addons/#Sauce-Connect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@marcoalbarelli
Copy link
Contributor

I think I'll sit this one out, sry
I tried using sylius as a base for a project, but even the task of actually enabling payment gateways took me a whole morning and I didn't see the end of it
I'm backing up to my symfony2 custom solution

Thanks for your effort though

@winzou
Copy link
Contributor

winzou commented Sep 6, 2013

What the status of this PR @headrevision @pjedrzejewski?
Can you rebase as master branch has changed quite a lot?

@stloyd
Copy link
Contributor

stloyd commented Sep 26, 2013

Closing in favor of #354.

@stloyd stloyd closed this Sep 26, 2013
pamil pushed a commit to pamil/Sylius that referenced this pull request Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants