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 multiple accounts and service account authentication #18

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

aschempp
Copy link
Collaborator

@aschempp aschempp commented Aug 4, 2016

This PR adds two features:

  1. You can have multiple accounts / multiple google client IDs.
    Use e.g. happyr.google.api.default_analytics and happyr.google.api.special_youtube with different credentials.
  2. New support for "service account" credentials. Our application uses Google Analytics information in the backend, so we need server-to-server communication. See https://developers.google.com/identity/protocols/OAuth2ServiceAccount for information from Google.

Everything should be fully BC with existing version (automatically converting the bundle configuration). I have been using the service authentication with client v1 for over a year now, it works well. I just added client v2 now and will test that implementation soon.

Let me know what you think :-)

@aschempp
Copy link
Collaborator Author

As promised, v2 tested and works as expected 😎

@aschempp
Copy link
Collaborator Author

aschempp commented Nov 8, 2016

Any update on this PR ?

@srslynow
Copy link

Nice work, thanks @aschempp

@aschempp
Copy link
Collaborator Author

will this get merged somewhen?

@aschempp
Copy link
Collaborator Author

aschempp commented Feb 4, 2019

@Nyholm @cakper is there anything I can do to get this merged?

@cakper
Copy link

cakper commented Feb 4, 2019

@aschempp I'm not involved in this project anymore, @Nyholm would you be able to help?

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

It's quite a large change and no tests.

Im happy you are using this bundle. What do you say if we merge this and tag 3.0?

Could you rebase this and then write a entry in the changelog?

{
/**
* {@inheritDoc}
*/
public function load(array $configs, ContainerBuilder $container)
public function loadInternal(array $config, ContainerBuilder $container)
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ConfigurableExtension is just a shortcut with helper methods.

@aschempp
Copy link
Collaborator Author

aschempp commented Feb 7, 2019

Im happy you are using this bundle. What do you say if we merge this and tag 3.0?

The changes are supposed to be backwards compatible, so there's no need for v3 from my side. As you can see, it has been a while (Aug 2016) when I implemented this. I'm still actively using that fork, but I'd love to see this in the core.

What would you say if I create separate branches and pull requests for the separate features? I can also look at the unit tests in that case. Or would you say its too much work to review every detail and just get a 3.0 out?

@ddonnini
Copy link
Contributor

ddonnini commented Feb 8, 2019

Thanks, @aschempp, I missed your PR! I think there are too many changes as you pointed out, maybe open different PRs would be better for @Nyholm!

* Add symfony 4 compatibility

* Remove useless compatibility for symfony/yaml

(cherry picked from commit 9238275)
@aschempp
Copy link
Collaborator Author

Sorry for the delay. I will create separate PRs as soon as I can find some time. I'm still actively using that feature, so I won't forget about it.

@Nyholm
Copy link
Member

Nyholm commented Nov 23, 2019

#SymfonyHackday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants