FacebookAuth::verifyCallback() - Code Metrics - Inspection of "Fix Facebook Auth" - TechWilk/rota - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#24)

by Christopher

created 2017-09-30 22:19 UTC

FacebookAuth::verifyCallback() B

↳ Parent: FacebookAuth

Complexity

Conditions	7
Paths	7

Size

Total Lines	60
Code Lines	33

Duplication

Lines	24
Ratio	40 %

Importance

Changes

Metric	Value
cc	7
eloc	33
c	0
b	0
f	0
nc	7
nop	1
dl	24
loc	60
rs	7.4661

How to fix Long Method

<?php

namespace TechWilk\Rota\AuthProvider\Callback;

use Facebook\Exceptions\FacebookResponseException;
use Facebook\Exceptions\FacebookSDKException;
use Facebook\Facebook;
use TechWilk\Rota\AuthProvider\CallbackInterface;
use TechWilk\Rota\EmailAddress;

class FacebookAuth implements CallbackInterface
{
    protected $facebook;
    protected $enabled;

    protected $appId;
    protected $permissions;

    protected $router;

    protected $user;

    public function __construct(Facebook $facebook, $appId, $baseUrl, $router, $enabled = true)
    {
        $this->facebook = $facebook;
        $this->enabled = (bool) $enabled;

        $this->appId = $appId;
        $this->router = $router;
        $this->baseUrl = $baseUrl;
class MyClass { }

$x = new MyClass();
$x->foo = true;

        $this->permissions = ['email'];
    }

    public function isEnabled()
    {
        return $this->enabled;
    }

    public function getCallbackUrl()
    {
        $helper = $this->facebook->getRedirectLoginHelper();

        $path = $this->router->pathFor('login-callback', ['provider' => $this->getAuthProviderSlug()]);

        $url = $this->baseUrl.$path;

        return $helper->getLoginUrl($url, $this->permissions);
    }

    public function verifyCallback($args)
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
    {
        $helper = $this->facebook->getRedirectLoginHelper();

        try {
            $accessToken = $helper->getAccessToken();
        } catch (FacebookResponseException $e) {
            // When Graph returns an error
            echo 'Graph returned an error: '.$e->getMessage();

            return false;
        } catch (FacebookSDKException $e) {
            // When validation fails or other local issues
            echo 'Facebook SDK returned an error: '.$e->getMessage();

            return false;
        }

        if (!isset($accessToken)) {

            if ($helper->getError()) {
''   == false // true
''   == null  // true
'ab' == false // false
'ab' == null  // false

// It is often better to use strict comparison
'' === false // false
'' === null  // false
                header('HTTP/1.0 401 Unauthorized');
                echo 'Error: '.$helper->getError()."\n";
                echo 'Error Code: '.$helper->getErrorCode()."\n";
                echo 'Error Reason: '.$helper->getErrorReason()."\n";
                echo 'Error Description: '.$helper->getErrorDescription()."\n";
            } else {
                header('HTTP/1.0 400 Bad Request');
                echo 'Bad request';
            }

            return false;
        }

        // The OAuth 2.0 client handler helps us manage access tokens
        $oAuth2Client = $this->facebook->getOAuth2Client();

        // Get the access token metadata from /debug_token
        $tokenMetadata = $oAuth2Client->debugToken($accessToken);

        // Validation (these will throw FacebookSDKException's when they fail)
        $tokenMetadata->validateAppId($this->appId); // Replace {app-id} with your app id
        // If you know the user ID this access token belongs to, you can validate it here
        //$tokenMetadata->validateUserId('123');

        $tokenMetadata->validateExpiration();

        if (!$accessToken->isLongLived()) {

            // Exchanges a short-lived access token for a long-lived one
            try {
                $accessToken = $oAuth2Client->getLongLivedAccessToken($accessToken);
            } catch (FacebookSDKException $e) {
                echo '<p>Error getting long-lived access token: '.$helper->getMessage()."</p>\n\n";


                return false;
            }
        }

        $_SESSION['fb_access_token'] = (string) $accessToken;

        return true;
    }

    public function getAuthProviderSlug()
    {
        return 'facebook';
    }

    public function getUserId()
// Bad
class Router
{
    public function generate($path)
    {
        return $_SERVER['HOST'].$path;
    }
}

// Better
class Router
{
    private $host;

    public function __construct($host)
    {
        $this->host = $host;
    }

    public function generate($path)
    {
        return $this->host.$path;
    }
}

class Controller
{
    public function myAction(Request $request)
    {
        // Instead of
        $page = isset($_GET['page']) ? intval($_GET['page']) : 1;

        // Better (assuming you use the Symfony2 request)
        $page = $request->query->get('page', 1);
    }
}
    {
        $accessToken = $_SESSION['fb_access_token'];

        try {
            // Returns a `Facebook\FacebookResponse` object
            $response = $this->facebook->get('/me?fields=id,name,email', $accessToken);
        } catch (Facebook\Exceptions\FacebookResponseException $e) {

            echo 'Graph returned an error: '.$e->getMessage();
            exit;

        } catch (Facebook\Exceptions\FacebookSDKException $e) {

            echo 'Facebook SDK returned an error: '.$e->getMessage();
            exit;

        }

        $user = $response->getGraphUser();

        $this->user = $user;

        return $user->getId();
    }

    public function getMeta()
    {
        if (empty($this->user)) {
            $this->getUserId();
        }

        return [
            'name'  => $this->user->getName(),
            'email' => new EmailAddress($this->user->getEmail()),
        ];
    }
}


1		<?php
2
3		namespace TechWilk\Rota\AuthProvider\Callback;
4
5		use Facebook\Exceptions\FacebookResponseException;
6		use Facebook\Exceptions\FacebookSDKException;
7		use Facebook\Facebook;
8		use TechWilk\Rota\AuthProvider\CallbackInterface;
9		use TechWilk\Rota\EmailAddress;
10
11		class FacebookAuth implements CallbackInterface
12		{
13		protected $facebook;
14		protected $enabled;
15
16		protected $appId;
17		protected $permissions;
18
19		protected $router;
20
21		protected $user;
22
23		public function __construct(Facebook $facebook, $appId, $baseUrl, $router, $enabled = true)
24		{
25		$this->facebook = $facebook;
26		$this->enabled = (bool) $enabled;
27
28		$this->appId = $appId;
29		$this->router = $router;
30		$this->baseUrl = $baseUrl;
		0 ignored issues – show Bug introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The property `baseUrl` does not exist. Did you maybe forget to declare it? In PHP it is possible to write to properties without declaring them. For example, the following is perfectly valid PHP code: class MyClass { } $x = new MyClass(); $x->foo = true; Generally, it is a good practice to explictly declare properties to avoid accidental typos and provide IDE auto-completion: class MyClass { public $foo; } $x = new MyClass(); $x->foo = true; Loading history...
31
32		$this->permissions = ['email'];
33		}
34
35		public function isEnabled()
36		{
37		return $this->enabled;
38		}
39
40		public function getCallbackUrl()
41		{
42		$helper = $this->facebook->getRedirectLoginHelper();
43
44		$path = $this->router->pathFor('login-callback', ['provider' => $this->getAuthProviderSlug()]);
45
46		$url = $this->baseUrl.$path;
47
48		return $helper->getLoginUrl($url, $this->permissions);
49		}
50
51		public function verifyCallback($args)
		0 ignored issues – show Coding Style introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report `verifyCallback` uses the super-global variable `$_SESSION` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
52		{
53		$helper = $this->facebook->getRedirectLoginHelper();
54
55		try {
56		$accessToken = $helper->getAccessToken();
57		} catch (FacebookResponseException $e) {
58		// When Graph returns an error
59		echo 'Graph returned an error: '.$e->getMessage();
60
61		return false;
62		} catch (FacebookSDKException $e) {
63		// When validation fails or other local issues
64		echo 'Facebook SDK returned an error: '.$e->getMessage();
65
66		return false;
67		}
68
69	View Code Duplication	if (!isset($accessToken)) {
		0 ignored issues – show Duplication introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report This code seems to be duplicated across your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
70		if ($helper->getError()) {
		0 ignored issues – show Bug Best Practice introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The expression `$helper->getError()` of type `string\|null` is loosely compared to `true`; this is ambiguous if the string can be empty. You might want to explicitly use `!== null` instead. In PHP, under loose comparison (like `==`, or `!=`, or `switch` conditions), values of different types might be equal. For `string` values, the empty string `''` is a special case, in particular the following results might be unexpected: '' == false // true '' == null // true 'ab' == false // false 'ab' == null // false // It is often better to use strict comparison '' === false // false '' === null // false Loading history...
71		header('HTTP/1.0 401 Unauthorized');
72		echo 'Error: '.$helper->getError()."\n";
73		echo 'Error Code: '.$helper->getErrorCode()."\n";
74		echo 'Error Reason: '.$helper->getErrorReason()."\n";
75		echo 'Error Description: '.$helper->getErrorDescription()."\n";
76		} else {
77		header('HTTP/1.0 400 Bad Request');
78		echo 'Bad request';
79		}
80
81		return false;
82		}
83
84		// The OAuth 2.0 client handler helps us manage access tokens
85		$oAuth2Client = $this->facebook->getOAuth2Client();
86
87		// Get the access token metadata from /debug_token
88		$tokenMetadata = $oAuth2Client->debugToken($accessToken);
89
90		// Validation (these will throw FacebookSDKException's when they fail)
91		$tokenMetadata->validateAppId($this->appId); // Replace {app-id} with your app id
92		// If you know the user ID this access token belongs to, you can validate it here
93		//$tokenMetadata->validateUserId('123');
		0 ignored issues – show Unused Code Comprehensibility introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report `86%` of this comment could be valid code. Did you maybe forget this after debugging? Sometimes obsolete code just ends up commented out instead of removed. In this case it is better to remove the code once you have checked you do not need it. The code might also have been commented out for debugging purposes. In this case it is vital that someone uncomments it again or your project may behave in very unexpected ways in production. This check looks for comments that seem to be mostly valid code and reports them. Loading history...
94		$tokenMetadata->validateExpiration();
95
96	View Code Duplication	if (!$accessToken->isLongLived()) {
		0 ignored issues – show Duplication introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report This code seems to be duplicated across your project. Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation. You can also find more detailed suggestions in the “Code” section of your repository. Loading history...
97		// Exchanges a short-lived access token for a long-lived one
98		try {
99		$accessToken = $oAuth2Client->getLongLivedAccessToken($accessToken);
100		} catch (FacebookSDKException $e) {
101		echo '<p>Error getting long-lived access token: '.$helper->getMessage()."</p>\n\n";
		0 ignored issues – show Bug introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The method `getMessage()` does not seem to exist on `object<Facebook\Helpers\...ookRedirectLoginHelper>`. This check looks for calls to methods that do not seem to exist on a given type. It looks for the method on the type itself as well as in inherited classes or implemented interfaces. This is most likely a typographical error or the method has been renamed. Loading history...
102
103		return false;
104		}
105		}
106
107		$_SESSION['fb_access_token'] = (string) $accessToken;
108
109		return true;
110		}
111
112		public function getAuthProviderSlug()
113		{
114		return 'facebook';
115		}
116
117		public function getUserId()
		0 ignored issues – show Coding Style introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report `getUserId` uses the super-global variable `$_SESSION` which is generally not recommended. Instead of super-globals, we recommend to explicitly inject the dependencies of your class. This makes your code less dependent on global state and it becomes generally more testable: // Bad class Router { public function generate($path) { return $_SERVER['HOST'].$path; } } // Better class Router { private $host; public function __construct($host) { $this->host = $host; } public function generate($path) { return $this->host.$path; } } class Controller { public function myAction(Request $request) { // Instead of $page = isset($_GET['page']) ? intval($_GET['page']) : 1; // Better (assuming you use the Symfony2 request) $page = $request->query->get('page', 1); } } Loading history...
118		{
119		$accessToken = $_SESSION['fb_access_token'];
120
121		try {
122		// Returns a `Facebook\FacebookResponse` object
123		$response = $this->facebook->get('/me?fields=id,name,email', $accessToken);
124		} catch (Facebook\Exceptions\FacebookResponseException $e) {
		0 ignored issues – show Bug introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The class `Facebook\Facebook\Except...cebookResponseException` does not exist. Did you forget a USE statement, or did you not list all dependencies? Scrutinizer analyzes your `composer.json`/`composer.lock` file if available to determine the classes, and functions that are defined by your dependencies. It seems like the listed class was neither found in your dependencies, nor was it found in the analyzed files in your repository. If you are using some other form of dependency management, you might want to disable this analysis. Loading history...
125		echo 'Graph returned an error: '.$e->getMessage();
126		exit;
		0 ignored issues – show Coding Style Compatibility introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The method `getUserId()` contains an exit expression. An exit expression should only be used in rare cases. For example, if you write a short command line script. In most cases however, using an `exit` expression makes the code untestable and often causes incompatibilities with other libraries. Thus, unless you are absolutely sure it is required here, we recommend to refactor your code to avoid its usage. Loading history...
127		} catch (Facebook\Exceptions\FacebookSDKException $e) {
		0 ignored issues – show Bug introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The class `Facebook\Facebook\Exceptions\FacebookSDKException` does not exist. Did you forget a USE statement, or did you not list all dependencies? Scrutinizer analyzes your `composer.json`/`composer.lock` file if available to determine the classes, and functions that are defined by your dependencies. It seems like the listed class was neither found in your dependencies, nor was it found in the analyzed files in your repository. If you are using some other form of dependency management, you might want to disable this analysis. Loading history...
128		echo 'Facebook SDK returned an error: '.$e->getMessage();
129		exit;
		0 ignored issues – show Coding Style Compatibility introduced 2017-09-30 22:27 UTC by Report Bug Copy Issue Report The method `getUserId()` contains an exit expression. An exit expression should only be used in rare cases. For example, if you write a short command line script. In most cases however, using an `exit` expression makes the code untestable and often causes incompatibilities with other libraries. Thus, unless you are absolutely sure it is required here, we recommend to refactor your code to avoid its usage. Loading history...
130		}
131
132		$user = $response->getGraphUser();
133
134		$this->user = $user;
135
136		return $user->getId();
137		}
138
139		public function getMeta()
140		{
141		if (empty($this->user)) {
142		$this->getUserId();
143		}
144
145		return [
146		'name' => $this->user->getName(),
147		'email' => new EmailAddress($this->user->getEmail()),
148		];
149		}
150		}
151

TechWilk / rota

Pull Request — master (#24)

FacebookAuth::verifyCallback() B

Complexity

Size

Duplication

Importance

How to fix Long Method

Long Method

Duplication Side-by-Side

Filter issues like