Implement discussion idea #37. #190
Conversation
I didn't review the code to see if it is up to date with Facebook's oauth flow. datamweb#37
WalkthroughThis pull request adds placeholder configuration settings and a new OAuth class. In the configuration file, it introduces commented-out options for Yahoo and Facebook OAuth without affecting functionality. Additionally, a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant A as FacebookOAuth
participant FB as Facebook API
U->>A: Request login link (makeGoLink)
A->>U: Provide authorization URL
U->>FB: Navigate to authorization URL
FB->>U: Redirect with auth code
U->>A: Submit auth code
A->>FB: POST request for access token (fetchAccessTokenWithAuthCode)
FB-->>A: Return access token
A->>FB: Request user info (fetchUserInfoWithToken)
FB-->>A: Return user info
A->>A: Process user data (setColumnsName)
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Libraries/FacebookOAuth.php (2)
21-25: Add type declarations for static properties.Defining each static property’s type (e.g.,
private static string $API_CODE_URL = ...;) will improve clarity and satisfy PHP 8.1 strict typing requirements.- private static $API_CODE_URL = 'https://www.facebook.com/v16.0/dialog/oauth'; - private static $API_TOKEN_URL = 'https://graph.facebook.com/v16.0/oauth/access_token'; - private static $API_USER_INFO_URL = 'https://graph.facebook.com/me?fields'; - private static $APPLICATION_NAME = 'SheildOAuth'; + private static string $API_CODE_URL = 'https://www.facebook.com/v16.0/dialog/oauth'; + private static string $API_TOKEN_URL = 'https://graph.facebook.com/v16.0/oauth/access_token'; + private static string $API_USER_INFO_URL = 'https://graph.facebook.com/me?fields'; + private static string $APPLICATION_NAME = 'SheildOAuth';🧰 Tools
🪛 GitHub Check: PHP 8.1 Static Analysis
[failure] 24-24:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$APPLICATION_NAME has no type specified.
[failure] 23-23:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$API_USER_INFO_URL has no type specified.
[failure] 22-22:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$API_TOKEN_URL has no type specified.
[failure] 21-21:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$API_CODE_URL has no type specified.
44-48: Minor naming error in$callbake_url.The property
$callbake_urlintroduces confusion. Consider renaming to$callback_urlfor clarity.- protected string $callbake_url; + protected string $callback_url;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Config/ShieldOAuthConfig.php(1 hunks)src/Libraries/FacebookOAuth.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Libraries/FacebookOAuth.php (2)
src/Libraries/Basic/AbstractOAuth.php (3)
AbstractOAuth(16-49)setToken(28-31)getToken(33-36)src/Config/Services.php (1)
Services(19-37)
🪛 GitHub Check: PHP 8.1 Static Analysis
src/Libraries/FacebookOAuth.php
[failure] 67-67:
Call to method getMessage() on an unknown class Datamweb\ShieldOAuth\Libraries\Exception.
[failure] 66-66:
Caught class Datamweb\ShieldOAuth\Libraries\Exception not found.
[failure] 53-53:
Access to an undefined property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$client.
[failure] 49-49:
Method Datamweb\ShieldOAuth\Libraries\FacebookOAuth::fetchAccessTokenWithAuthCode() has parameter $allGet with no value type specified in iterable type array.
[failure] 37-37:
Access to an undefined property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$config.
[failure] 35-35:
Access to an undefined property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$client.
[failure] 24-24:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$APPLICATION_NAME has no type specified.
[failure] 23-23:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$API_USER_INFO_URL has no type specified.
[failure] 22-22:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$API_TOKEN_URL has no type specified.
[failure] 21-21:
Property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$API_CODE_URL has no type specified.
🪛 GitHub Actions: Rector
src/Libraries/FacebookOAuth.php
[warning] 12-12: Skipped rule "Rector\Php73\Rector\FuncCall\JsonThrowOnErrorRector" is never registered. You can remove it from "->withSkip()".
🪛 GitHub Actions: PHPCSFixer
src/Libraries/FacebookOAuth.php
[error] 16-16: PHP CS Fixer: Multiple rule violations including class_attributes_separation, single_quote, concat_space, no_trailing_whitespace, statement_indentation, blank_line_before_statement, binary_operator_spaces.
🔇 Additional comments (2)
src/Config/ShieldOAuthConfig.php (2)
53-55: No functional impact, but consider removing commented-out placeholders if not needed.These lines simply add placeholders for Yahoo’s OAuth configuration. If you do not plan to enable or use this configuration anytime soon, consider removing them to reduce clutter. Otherwise, they look fine.
56-59: Added Facebook placeholders match the new OAuth library.These commented-out lines for Facebook config align with the new
FacebookOAuthclass. They are currently harmless. Whenever you plan to activate Facebook OAuth, remember to remove the comments or override them in an environment-specific config.
| protected function setColumnsName(string $nameOfProcess, $userInfo): array | ||
| { | ||
| if ($nameOfProcess === 'syncingUserInfo') { | ||
| $usersColumnsName = [ | ||
| $this->config->usersColumnsName['first_name'] => $userInfo->first_name, | ||
| $this->config->usersColumnsName['last_name'] => $userInfo->last_name, | ||
| $this->config->usersColumnsName['avatar'] => $userInfo->picture->data->url, | ||
| ]; | ||
| } | ||
|
|
||
| if ($nameOfProcess === 'newUser') { | ||
| $usersColumnsName = [ | ||
| // users tbl // OAuth | ||
| 'username' => $userInfo->first_name, | ||
| 'email' => $userInfo->email, | ||
| 'password' => random_string('crypto', 32), | ||
| 'active' => '1', | ||
| $this->config->usersColumnsName['first_name'] => $userInfo->first_name, | ||
| $this->config->usersColumnsName['last_name'] => $userInfo->last_name, | ||
| $this->config->usersColumnsName['avatar'] => $userInfo->picture->data->url, | ||
| ]; | ||
| } | ||
|
|
||
| return $usersColumnsName; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle missing or incomplete picture data.
Accessing $userInfo->picture->data->url will fail if picture is absent or its structure changes. Consider adding a safety check or fallback logic.
| public function __construct(string $token = '') | ||
| { | ||
| $this->token = $token; | ||
| $this->client = \Config\Services::curlrequest(); | ||
|
|
||
| $this->config = config('ShieldOAuthConfig'); | ||
| $this->callbake_url = base_url('oauth/' . $this->config->call_back_route); | ||
| $this->client_id = $this->config->oauthConfigs['facebook']['client_id']; | ||
| $this->client_secret = $this->config->oauthConfigs['facebook']['client_secret']; | ||
| $this->fb_scope = "id,first_name,last_name,middle_name,name,name_format,picture,short_name,email"; | ||
| } |
There was a problem hiding this comment.
Declare the missing $client and $config properties.
The code references $this->client and $this->config, but these properties are not declared, causing static analysis failures and likely runtime errors. Add them with proper types.
class FacebookOAuth extends AbstractOAuth
{
+ protected \CodeIgniter\HTTP\CURLRequest $client;
+ protected \Datamweb\ShieldOAuth\Config\ShieldOAuthConfig $config;
...
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function __construct(string $token = '') | |
| { | |
| $this->token = $token; | |
| $this->client = \Config\Services::curlrequest(); | |
| $this->config = config('ShieldOAuthConfig'); | |
| $this->callbake_url = base_url('oauth/' . $this->config->call_back_route); | |
| $this->client_id = $this->config->oauthConfigs['facebook']['client_id']; | |
| $this->client_secret = $this->config->oauthConfigs['facebook']['client_secret']; | |
| $this->fb_scope = "id,first_name,last_name,middle_name,name,name_format,picture,short_name,email"; | |
| } | |
| <?php | |
| namespace Datamweb\ShieldOAuth\Libraries; | |
| class FacebookOAuth extends AbstractOAuth | |
| { | |
| protected \CodeIgniter\HTTP\CURLRequest $client; | |
| protected \Datamweb\ShieldOAuth\Config\ShieldOAuthConfig $config; | |
| public function __construct(string $token = '') | |
| { | |
| $this->token = $token; | |
| $this->client = \Config\Services::curlrequest(); | |
| $this->config = config('ShieldOAuthConfig'); | |
| $this->callbake_url = base_url('oauth/' . $this->config->call_back_route); | |
| $this->client_id = $this->config->oauthConfigs['facebook']['client_id']; | |
| $this->client_secret = $this->config->oauthConfigs['facebook']['client_secret']; | |
| $this->fb_scope = "id,first_name,last_name,middle_name,name,name_format,picture,short_name,email"; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: PHP 8.1 Static Analysis
[failure] 37-37:
Access to an undefined property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$config.
[failure] 35-35:
Access to an undefined property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$client.
| protected function fetchUserInfoWithToken(): object | ||
| { | ||
| // send request to API URL | ||
| try { | ||
| $response = $this->client->request('POST', self::$API_USER_INFO_URL.'='.$this->fb_scope, [ | ||
| 'headers' => [ | ||
| 'Accept' => 'application/json', | ||
| 'User-Agent' => self::$APPLICATION_NAME . '/1.0', | ||
| 'Authorization' => 'Bearer ' . $this->getToken(), | ||
| ], | ||
| 'http_errors' => false, | ||
| ]); | ||
| } catch (Exception $e) { | ||
| exit($e->getMessage()); | ||
| } | ||
| return json_decode($response->getBody()); | ||
| } |
There was a problem hiding this comment.
Fix the query string concatenation and HTTP method.
- Currently,
'?fields' . '=' . $this->fb_scopeyields an extra=(resulting in?fields==...). It should be'?fields=' . $this->fb_scope. - Typically, fetching user info is done with a GET request instead of POST. Verify that Facebook’s API accepts POST here.
- $response = $this->client->request('POST', self::$API_USER_INFO_URL.'='.$this->fb_scope, [
+ $response = $this->client->request('GET', self::$API_USER_INFO_URL . '=' . $this->fb_scope, [Committable suggestion skipped: line range outside the PR's diff.
| protected function fetchAccessTokenWithAuthCode(array $allGet): void | ||
| { | ||
| try { | ||
| // send request to API URL | ||
| $response = $this->client->request('POST', self::$API_TOKEN_URL, [ | ||
| 'form_params' => [ | ||
| 'client_id' => $this->client_id, | ||
| 'client_secret' => $this->client_secret, | ||
| 'code' => $allGet['code'], | ||
| 'redirect_uri' => $this->callbake_url, | ||
| 'grant_type' => 'authorization_code', | ||
| ], | ||
| 'headers' => [ | ||
| 'User-Agent' => self::$APPLICATION_NAME . '/1.0', | ||
| 'Accept' => 'application/json', | ||
| ], | ||
| ]); | ||
| } catch (Exception $e) { | ||
| exit($e->getMessage()); | ||
| } | ||
| $token = json_decode($response->getBody())->access_token; | ||
| $this->setToken($token); | ||
| } |
There was a problem hiding this comment.
Import the correct Exception class and avoid terminating the app on errors.
- Ensure the
Exceptionclass is imported viause Exception;or fully qualified ascatch (\Exception $e). - Relying on
exit($e->getMessage())may abruptly stop the entire application. Consider throwing a typed exception or logging the error instead.
+ use Exception;
protected function fetchAccessTokenWithAuthCode(array $allGet): void
{
try {
...
} catch (\Exception $e) {
- exit($e->getMessage());
+ // Log or rethrow the exception
+ throw $e;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function fetchAccessTokenWithAuthCode(array $allGet): void | |
| { | |
| try { | |
| // send request to API URL | |
| $response = $this->client->request('POST', self::$API_TOKEN_URL, [ | |
| 'form_params' => [ | |
| 'client_id' => $this->client_id, | |
| 'client_secret' => $this->client_secret, | |
| 'code' => $allGet['code'], | |
| 'redirect_uri' => $this->callbake_url, | |
| 'grant_type' => 'authorization_code', | |
| ], | |
| 'headers' => [ | |
| 'User-Agent' => self::$APPLICATION_NAME . '/1.0', | |
| 'Accept' => 'application/json', | |
| ], | |
| ]); | |
| } catch (Exception $e) { | |
| exit($e->getMessage()); | |
| } | |
| $token = json_decode($response->getBody())->access_token; | |
| $this->setToken($token); | |
| } | |
| <?php | |
| use Exception; | |
| class FacebookOAuth { | |
| // Other class members... | |
| protected function fetchAccessTokenWithAuthCode(array $allGet): void | |
| { | |
| try { | |
| // send request to API URL | |
| $response = $this->client->request('POST', self::$API_TOKEN_URL, [ | |
| 'form_params' => [ | |
| 'client_id' => $this->client_id, | |
| 'client_secret' => $this->client_secret, | |
| 'code' => $allGet['code'], | |
| 'redirect_uri' => $this->callbake_url, | |
| 'grant_type' => 'authorization_code', | |
| ], | |
| 'headers' => [ | |
| 'User-Agent' => self::$APPLICATION_NAME . '/1.0', | |
| 'Accept' => 'application/json', | |
| ], | |
| ]); | |
| } catch (\Exception $e) { | |
| // Log or rethrow the exception | |
| throw $e; | |
| } | |
| $token = json_decode($response->getBody())->access_token; | |
| $this->setToken($token); | |
| } | |
| // Other class members... | |
| } |
🧰 Tools
🪛 GitHub Check: PHP 8.1 Static Analysis
[failure] 67-67:
Call to method getMessage() on an unknown class Datamweb\ShieldOAuth\Libraries\Exception.
[failure] 66-66:
Caught class Datamweb\ShieldOAuth\Libraries\Exception not found.
[failure] 53-53:
Access to an undefined property Datamweb\ShieldOAuth\Libraries\FacebookOAuth::$client.
[failure] 49-49:
Method Datamweb\ShieldOAuth\Libraries\FacebookOAuth::fetchAccessTokenWithAuthCode() has parameter $allGet with no value type specified in iterable type array.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/index.md (1)
12-13: Refine Tone and Style in Feature Description
The updated description now includes Facebook OAuth as a default supported service, which is great for clarity. However, the phrase “this is very easy. Just create a class in routeapp\Libraries\ShieldOAuth!” can be rephrased to adopt a more neutral tone. For example, consider:-...GitLab and ..., this is very easy. Just create a class in route `app\Libraries\ShieldOAuth`! +...GitLab and ... clearly. Simply create a class in the `app\Libraries\ShieldOAuth` directory.This change will enhance readability and maintain consistency with the overall documentation style.
🧰 Tools
🪛 LanguageTool
[style] ~12-~12: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...tter, LinkedIn, GitLab and ..., this is very easy. Just create a class in route `app\Libr...(EN_WEAK_ADJECTIVE)
[style] ~12-~12: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 1531 characters long)
Context: ...ass in routeapp\Libraries\ShieldOAuth! more info see [How to add other servic...(EN_EXCESSIVE_EXCLAMATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/get_keys.md(1 hunks)docs/index.md(1 hunks)docs/install.md(1 hunks)docs/quickstart.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/quickstart.md
🧰 Additional context used
🪛 LanguageTool
docs/index.md
[style] ~12-~12: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...tter, LinkedIn, GitLab and ..., this is very easy. Just create a class in route `app\Libr...
(EN_WEAK_ADJECTIVE)
[style] ~12-~12: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 1531 characters long)
Context: ...ass in route app\Libraries\ShieldOAuth! more info see [How to add other servic...
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (2)
docs/install.md (1)
121-125: Add Facebook OAuth Endpoint to Installation URLs
The new Facebook OAuth URL (http://localhost:8080/oauth/facebook) is correctly inserted into the list of available OAuth endpoints. This update aligns with the recent integration changes and improves clarity for users setting up multiple providers.docs/get_keys.md (1)
35-40: Introduce Facebook Keys Documentation Section
The addition of the "Get Facebook Keys" section with the appropriate link to Facebook’s developer documentation is a clear enhancement. It makes the documentation more comprehensive by covering all default OAuth providers. Please verify that the formatting (heading level and spacing) is consistent with the other sections.
I didn't review the code to see if it is up to date with Facebook's oauth flow.
#37
Summary by CodeRabbit
New Features
Documentation Updates