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

Updated the authentication configuration to support credentials #44

Merged
merged 3 commits into from
Feb 28, 2016

Conversation

Nyholm
Copy link
Member

@Nyholm Nyholm commented Feb 25, 2016

This PR fixes #42

// config.yml
httplug:
    plugins:
        authentication:
            my_basic:
                type: 'basic'
                username: 'foo'
                password: 'bar'
            my_wsse:
                type: 'wsse'
                username: 'foo'
                password: 'bar'
            my_brearer:
                type: 'bearer'
                token: 'foo'

    clients:
        acme:
            factory: 'httplug.factory.guzzle6'
            plugins: ['httplug.plugin.authentication.my_wsse']

This PR will break BC because change of the configuration in httplug.plugins.authentication.

@Nyholm Nyholm force-pushed the authentication branch 2 times, most recently from cca547c to f5815b6 Compare February 25, 2016 13:57
->end()
->scalarNode('username')->end()
->scalarNode('password')->end()
->scalarNode('token')->end()
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could go all fancy with custom validation to have the right required parameters based on the type. but we don't need that now.

i wonder if we should add type service that accepts a scalarNode authentication_service to keep supporting custom authentication services?

Copy link
Member Author

Choose a reason for hiding this comment

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

i wonder if we should add type service that accepts a scalarNode authentication_service to keep supporting custom authentication services?

Sure!

Copy link
Member

Choose a reason for hiding this comment

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

Not sure. People can still set up authentication plugins, can't they?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we add type service then you could use the authentication plugin just as before. It just require you to do a minor config change.

Before:

// config.yml
httplug:
    plugins:
        authentication:
             authentication: 'my_authentication_service'
    clients:
        my_client:
            factory: 'httplug.factory.guzzle6'
            plugins: ['httplug.plugin.authentication']

After:

// config.yml
httplug:
    plugins:
        authentication:
            acme:
                type: 'service'
                service: 'my_authentication_service'
    clients:
        my_client:
            factory: 'httplug.factory.guzzle6'
            plugins: ['httplug.plugin.authentication.acme']

@dbu
Copy link
Collaborator

dbu commented Feb 25, 2016

perfect, i think this configuration looks as simple and straightforward as it can be!

@Nyholm
Copy link
Member Author

Nyholm commented Feb 25, 2016

Thank you for the feedback. I've addressed your comments.

}
break;
case 'bearer':
if (empty($config['bearer'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/bearer/token/

@Nyholm
Copy link
Member Author

Nyholm commented Feb 28, 2016

I've merged PR #48 and I think this PR is ready for merge into master.

@sagikazarmark
Copy link
Member

A squash would be a good idea first.

@Nyholm
Copy link
Member Author

Nyholm commented Feb 28, 2016

Of course, Sorry @sagikazarmark.
They are now squashed.

dbu added a commit that referenced this pull request Feb 28, 2016
Updated the authentication configuration to support credentials
@dbu dbu merged commit f5c7b0d into master Feb 28, 2016
@dbu dbu deleted the authentication branch February 28, 2016 15:27
@dbu
Copy link
Collaborator

dbu commented Feb 28, 2016

great job, thanks a lot!

@Nyholm
Copy link
Member Author

Nyholm commented Feb 28, 2016

Thank you for the input!

@Nyholm Nyholm mentioned this pull request Feb 28, 2016
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.

Add configuration for autentication
3 participants