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

Moved SessionID from query param to the request headers. #53

Merged
merged 3 commits into from
Oct 2, 2016
Merged

Moved SessionID from query param to the request headers. #53

merged 3 commits into from
Oct 2, 2016

Conversation

poratuk
Copy link
Contributor

@poratuk poratuk commented Sep 23, 2016

No description provided.

@munvier
Copy link
Contributor

munvier commented Sep 23, 2016

Copy link
Owner

@jasny jasny left a comment

Choose a reason for hiding this comment

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

Please follow RFC 6750 and follow PSR-2 coding style guide.

@@ -192,7 +190,7 @@ protected function request($method, $command, $data = null)
$ch = curl_init($url);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_CUSTOMREQUEST, $method);
curl_setopt($ch, CURLOPT_HTTPHEADER, ['Accept: application/json']);
curl_setopt($ch, CURLOPT_HTTPHEADER, ['Accept: application/json', 'Authorization:'. $this->getSessionID()]);
Copy link
Owner

Choose a reason for hiding this comment

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

Please use 'Authorization: Bearer ' . $this->getSessionId(). See this answer to know why.

@@ -68,12 +68,12 @@ public function startBrokerSession()
{
if (isset($this->brokerId)) return;

if (!isset($_GET['sso_session'])) {
$sid = $this->getBrokerSessionID();
Copy link
Owner

Choose a reason for hiding this comment

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

Please adhere to PSR2 and use 4 spaces, not tabs.

if (!isset($_GET['sso_session'])) {
$sid = $this->getBrokerSessionID();

if ($sid == FALSE) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please adhere to PSR2 and use false, not FALSE.

* Get session ID from header Authorization or from $_GET/$_POST
*/
protected function getBrokerSessionID(){
$headers = getallheaders();
Copy link
Owner

Choose a reason for hiding this comment

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

Please adhere to PSR2 and use 4 spaces, not tabs.

return $_POST['sso_session'];
}

return FALSE;
Copy link
Owner

Choose a reason for hiding this comment

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

Please adhere to PSR2 and use false, not FALSE.

if (isset($_GET['sso_session'])) {
return $_GET['sso_session'];
}
if (isset($_POST['sso_session'])) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please follow RFC 6750 and accept $_GET['access_token'] or $_POST['access_token']. Also accept $_GET['sso_session'] for backwards compatibility.

$headers = getallheaders();

if (isset($headers['Authorization'])){
if (strpos($headers['Authorization'], 'Bearer') === 0){
Copy link
Owner

Choose a reason for hiding this comment

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

Do not accept Authorization if it doesn't start with Bearer (mind the space). You can use 1 if.
Site note: Please put a space between ){.

if (isset($_GET['sso_session'])) {
return $_GET['sso_session'];
}
if (isset($_POST['sso_session'])) {
Copy link
Owner

Choose a reason for hiding this comment

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

Do not accept $_POST['sso_session'], only $_GET['sso_session']

$sid = $this->getBrokerSessionID();

if ($sid == FALSE) {
$sid = $this->getBrokerSessionID();
Copy link
Owner

Choose a reason for hiding this comment

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

spacing is not alligned

@jasny jasny merged commit cc130f2 into jasny:master Oct 2, 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.

3 participants