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

Allowed API insta-login via HTTP Basic Auth #3443

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

colinmollenhour
Copy link
Member

@colinmollenhour colinmollenhour commented Aug 15, 2023

Using the XMLRPC and JSONRPC API is a hassle because you have to login to get a session id and then use that session id in subsequent requests and then deal with expiration, etc. Some users also create new sessions in every new process so you end up with lots of login calls. This allows one to authenticate to the API without calling the login method but rather passing the username and password using HTTP Basic Auth. This allows a method to be called with a single API request making it much easier to consume the API with low-code tools. Pass null as the session id.

Example request (PhpStorm format):

### Product list
POST http://openmage-7f000001.nip.io/api/jsonrpc
Content-Type: application/json
Authorization: Basic test VTm5pOQ

{
  "jsonrpc": 2.0,
  "id": 5501,
  "method": "call",
  "params": [
    null,
    "catalog_product.list",
    []
  ]
}

@github-actions github-actions bot added the Component: Api PageRelates to Mage_Api label Aug 15, 2023
@colinmollenhour colinmollenhour changed the title Allow insta-login via HTTP Basic Auth Allow API insta-login via HTTP Basic Auth Aug 15, 2023
@kiatng
Copy link
Collaborator

kiatng commented Aug 16, 2023

I tested and it works:

    public function jsonrpcAction()
    {
        $url = Mage::getStoreConfig('muze/stars/url');
        $apiUser = Mage::getStoreConfig('muze/stars/user');
        $apiKey = Mage::getStoreConfig('muze/stars/key'); 
        $client = new Krixon_JsonRpc_Client($url);
        $client->getHttpClient()->setAuth($apiUser, $apiKey);
        $result = $client->call('call', [null, 'magento.info', []]); // session_id is set to null, no need to call login API!
        var_dump($result);
    }

Question: the server needs to login for every call and create a new session ID. Is there a way around this? May be a way for the client to get the session ID to avoid constant login?

Is this refactoring better:

    /**
     * Allow insta-login via HTTP Basic Auth
     *
     * @param string $sessionId
     * @return $this
     */
    protected function _instaLogin(&$sessionId)
    {
        if ($sessionId === NULL && !empty($_SERVER['PHP_AUTH_USER']) && !empty($_SERVER['PHP_AUTH_PW'])) {
            $sessionId = $this->login($_SERVER['PHP_AUTH_USER'], $_SERVER['PHP_AUTH_PW']);
        }
        return $this;
    }

    public function call($sessionId, $apiPath, $args = [])
    {
        $this->_instaLogin($sessionId)
            ->_startSession($sessionId);
//...

@colinmollenhour
Copy link
Member Author

Question: the server needs to login for every call and create a new session ID. Is there a way around this?

I think a further improvement would certainly be to somehow prevent the session from even being recorded in the api_session table since it will never be re-used, but I was going to leave that for a separate task. E.g. bypassing recordSession method for these logins so it only exists in memory.

Is this refactoring better:

Probably, but it's a bit subjective when the duplicated code is so short. 🙂

kiatng
kiatng previously approved these changes Aug 16, 2023
ADDISON74
ADDISON74 previously approved these changes Aug 16, 2023
@fballiano
Copy link
Contributor

:-D I didn't notice phpcs was complaining about the NULL thing :-D

@fballiano fballiano dismissed stale reviews from ADDISON74 and kiatng via c2a5942 August 16, 2023 09:04
@fballiano
Copy link
Contributor

I fixed the phpcs thing, I'd merge it since it was already approved but I think we should really have a bit of documentation about this new interesting feature.

Thing is, we can't continue stuffing the README with everything IMHO...

@colinmollenhour
Copy link
Member Author

Oops, I copied this from another project that uses upper-case NULL/TRUE/FALSE - also a habbit.. 😄 Thanks for fixing.

The proper place for documentation would be here, although there is no mention of JSON API there either.. https://devdocs-openmage.org/guides/m1x/

@fballiano
Copy link
Contributor

absolutely, there's should be something about the jsonrpc thing too, but the devdocs... I don't know... it really seems ancient, I was checkin it and it seems to me that almost everything is nowadays irrelevant, it's great to have a mirror of the old docs, but I wouldn't know where to start to expand it :-\

@fballiano
Copy link
Contributor

I'll merge this because it was already approved and the subsequent commits were trivial.

@fballiano fballiano changed the title Allow API insta-login via HTTP Basic Auth Allowed API insta-login via HTTP Basic Auth Aug 17, 2023
@fballiano fballiano merged commit 8e5cc7f into OpenMage:main Aug 17, 2023
15 checks passed
@kiatng
Copy link
Collaborator

kiatng commented Aug 18, 2023

@colinmollenhour Will you be able to make another PR on bypassing the recordSession(). If the clients doesn't endSession(), the table can grow unnecessary.

@colinmollenhour
Copy link
Member Author

@colinmollenhour Will you be able to make another PR on bypassing the recordSession(). If the clients doesn't endSession(), the table can grow unnecessary.

I can't do it right now.. I added an issue report for it though: #3449

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

Successfully merging this pull request may close these issues.

None yet

4 participants