daithi-coombes / api-connection-manager

Manages connections and requests to 3rd party providers or services
0 stars 1 forks source link

custom services with no login forms #16

Closed daithi-coombes closed 11 years ago

daithi-coombes commented 11 years ago

@mrdavidlaing @ryanholder have come up with an interesting conundrum. Grabbing the acces token with some custom services will require a username/password to be sent, thus after a user clicks 'login with ci', for example, they will have to be taken to a form to grab the ci login details. The issue is that the form resides on the blog installation and not on the ci domain. This could be seen as, and I suppose is, a security concern by openly allowing the ease of phising username/passwords by dodgy sites with wordpress installations.

I don't think this should be a feature should the project take of, but then again I don't see a way around it.

mrdavidlaing commented 11 years ago

I agree that the collection of credentials shouldn't be part of the API Connection Manager.

In the short term, lets implement a separate PHP page that collects your CI credentials and returns the CI session key as an access_token.

In the long term, I need to push to have the CIAPI support OAuth2.

daithi-coombes commented 11 years ago

cool, i'll stick with the custom ci api for now Closing comment.

mrdavidlaing commented 11 years ago

The City Index OAuth2 / Login server should work something like this:

2013-01-25 16 57 35

daithi-coombes commented 11 years ago

perfect. I could have a look at the php implementation for the oauth2 server on their site and see how solid their code is.

Using the oauth2 service these should be the only methods to be overriden for the ci module...

class CityIndex_API extends API_Con_Mngr_Module{

        /**
         * Make sure you call this from your child class.
         * 
         * @global API_Connection_Manager $API_Connection_Manager 
         */
        function __construct();

        /**
         * This method checks a response from the service for an error and must
         * be declared by your class.
         * 
         * If you find an error in the response return the error string else
         * return false for no error.
         * 
         * @param array $response The response in the same format as returned by
         * the WP_HTTP class.
         * @return mixed Returns false if no error or string if error found.
         */
        abstract public function check_error( array $response );

        /**
         * Make a request to verify a token. If no token then return false, if
         * service provides no call to test token then make request for profile
         * etc as a test.
         * @return boolean 
         */
        abstract public function verify_token();

        /**
         * Makes a request to get an accounts uid. Must return the uid of the
         * remote service or WP_Error if none.
         * @return string 
         */
        abstract public function get_uid();

}
$module = new CityIndex_API();

Calls from plugins to use the cityindex module would then be:

$module = $API_Connection_Manager->get_service('cityindex/index. php');
$result = $module->request("/endpoint", "POST", array('foo'=>'bar'));
daithi-coombes commented 11 years ago

agreed and implemented.

Will use php's exception handler to try catch errors for the wordpress dashboard error reporting: http://php.net/manual/en/function.set-error-handler.php

its also important to note from the docs.. "Also note that it is your responsibility to die() if necessary. If the error-handler function returns, script execution will continue with the next statement after the one that caused an error."

So fatal errors from one module not up to standard don't need to cause WSOD on some installations and instead the module will be deactivated, the error message displayed in dashboard and the blogs code flow will go on as expected.

@mrdavidlaing Am just thinking that maybe this feature, which would be a wordpress necessity, should be left for the open source community? Maybe a task thats left undone to get some interest in this project?

mrdavidlaing commented 11 years ago

If you don't feel like implementing it, why would someone else who has no vested interest in the plugin?

In my experience, contributions are most likely to be plugins that customise you core code for a specific scenario. So I'd exect contributions of API-con modules for additional Apis, but just complaints or requests for additional core features.

I'm always afraid to submit core changes because I'm unsure if they will be accepted. Writing an additional plugin to a well defined plugin spec is a much easier inital contribution to make.

To conclude, if the feature is worth having, schedule it to be implemented by yourself in a later release. If it isn't worth you doing, then it probably isn't worth doing at all.

On Monday, January 28, 2013, david-coombes wrote:

agreed and implemented.

Will use php's exception handler to try catch errors for the wordpress dashboard error reporting: http://php.net/manual/en/function.set-error-handler.php

its also important to note from the docs.. "Also note that it is your responsibility to die() if necessary. If the error-handler function returns, script execution will continue with the next statement after the one that caused an error."

So fatal errors from one module not up to standard don't need to cause WSOD on some installations and instead the module will be deactivated, the error message displayed in dashboard and the blogs code flow will go on as expected.

@mrdavidlaing https://github.com/mrdavidlaing Am just thinking that maybe this feature, which would be a wordpress necessity, should be left for the open source community? Maybe a task thats left undone to get some interest in this project?

— Reply to this email directly or view it on GitHubhttps://github.com/david-coombes/api-connection-manager/issues/16#issuecomment-12800974.

David Laing Open source @ City Index - github.com/cityindex http://davidlaing.com Twitter: @davidlaing

daithi-coombes commented 11 years ago

got ya, it makes sense. Will implement it as per above (catching errors with phps set_error_handler())

On Mon, Jan 28, 2013 at 9:01 PM, David Laing notifications@github.comwrote:

If you don't feel like implementing it, why would someone else who has no vested interest in the plugin?

In my experience, contributions are most likely to be plugins that customise you core code for a specific scenario. So I'd exect contributions of API-con modules for additional Apis, but just complaints or requests for additional core features.

I'm always afraid to submit core changes because I'm unsure if they will be accepted. Writing an additional plugin to a well defined plugin spec is a much easier inital contribution to make.

To conclude, if the feature is worth having, schedule it to be implemented by yourself in a later release. If it isn't worth you doing, then it probably isn't worth doing at all.

On Monday, January 28, 2013, david-coombes wrote:

agreed and implemented.

Will use php's exception handler to try catch errors for the wordpress dashboard error reporting: http://php.net/manual/en/function.set-error-handler.php

its also important to note from the docs.. "Also note that it is your responsibility to die() if necessary. If the error-handler function returns, script execution will continue with the next statement after the one that caused an error."

So fatal errors from one module not up to standard don't need to cause WSOD on some installations and instead the module will be deactivated, the error message displayed in dashboard and the blogs code flow will go on as expected.

@mrdavidlaing https://github.com/mrdavidlaing Am just thinking that maybe this feature, which would be a wordpress necessity, should be left for the open source community? Maybe a task thats left undone to get some interest in this project?

— Reply to this email directly or view it on GitHub< https://github.com/david-coombes/api-connection-manager/issues/16#issuecomment-12800974>.

David Laing Open source @ City Index - github.com/cityindex http://davidlaing.com Twitter: @davidlaing

— Reply to this email directly or view it on GitHubhttps://github.com/david-coombes/api-connection-manager/issues/16#issuecomment-12804462.

"Any society that would give up a little liberty to gain a little security, will deserve neither and lose both." - Benjamin Franklin

daithi-coombes commented 11 years ago

@ryanholder @mrdavidlaing Requesting to close this comment.

The temporary custom login form has been implemented for now: 9548e130bcc8045c022fb133572ab88b22031cfc

Requesting to close this as there may be situations where blogs with their own api's would like to have their own custom forms. In cases like this they won't be putting their modules to the www so leaving the code for the custom login form will cater for these situations.

Also... My mention above about the php set error handler I think was meant for another issue: error report module child classes So I've moved that mention to issue num 10