[D7] Naver Login

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2407833

Project 2: https://www.drupal.org/node/2405385

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

annyeonghaseyo! [[[Just seen that you are from Korea. It's in my bucket list for this year and I absolutely love your country!]]]

Login integration module needs to be written very carefully, so I think this application will need a few more users to review.
Before trying the functionality, here are some best practices checks:

naver_login.admin.inc
- Some string literals use URLs. Consider changing them to use proper t() function modifiers. For example, t('Visit <a href="@url">there</a> to obtain Naver Login Service key', array('http://example.com'))

- naver_login_callback_uri: This variable will be saved back to the database

naver_login.install
- The hook_enable implementation is not necessary. Drupal takes care to install the schema when installing a module for the first time, and removing it when uninstalling.

- Module creates some variables, which need to be removed in a hook_uninstall() implementation (Not a hook_disable() because in Drupal 7, modules can be disabled without removing its data).

naver_login.module

- Rather than adding a user category and altering its page callback, consider introducing a new tab (user/%user/naver_login or a new sub tab user/%user/edit/%naver_login; latter one inherits permissions for user/%user/edit) in your hook_menu implementation.

- hook_user_cancel implementation is not necessary because you already have a hook_user_delete(), which is called every time a user is deleted regardless of the cancellation method.

- When deleting the user accounts, unset the data you set in $user->data. This will not raise any database errors, but they will remain in the database and memory.

naver_login.pages.inc

- Is there are specific reason why you didn't use drupal_goto() in naver_login_auth_request? There is a header() call, which drupal_goto doing.
- I see that you have moved some submit and validate handlers to the .module file. Well, you don have to! When you have the $form_state by reference, call form_load_include to the pages.inc file (even though you call this function from the same page), and Drupal will take care to include those files before executing any submit or validate handlers.
- naver_login_user_settings_form(): Consider using user_access() function to check permissions instead of checking for the administrator role. In "minimal" Drupal installations, there is no "administrator" role. It's an unlocked role so site users have probably changed it.
- It raises a lot of problems if you call drupal_goto() in a form validate or submit handler, because that prevents the rest of the page from being built. If it's a validate handler, you can trigger a validation error. For submit handlers, call return;, so the rest of the code block will not be called, but regular form events will run.
- naver_login_user_identities(): You can use drupal_goto() in the menu router item itself and map arguments accordingly, so there is no need for this function to exist.

Overall: A very well written module. I'm setting the status to "needs work" because module leaves some variables and user data in the database after uninstalling and when deleting user accounts. I haven't tested the module myself, but I will reply with a hands-on review later.

Nice work in the module! I haven't tested the installed module yet, but looking at the code repo, they looks very good IMO.

- I would replace db_select() with db_query() because db_query is faster. db_select would be a better choice if your query is complex, but I didn't see any in your module. This is not a blocker at all though.

I'm reviewing it now.

Timeout when invoking pareview.sh for http://git.drupal.org/sandbox/LaelRukius/2405635.git at http://pareview.sh/pareview/httpgitdrupalorgsandboxLaelRukius2405635git

Do you have any third-party files committed? 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access.

I'm a robot and this is an automated message from Project Applications Scraper.

Hello,

I just did the automated testing, and i found some small errors in the naver_login.pages.inc file:

FILE: /var/www/drupal-7-pareview/pareview_temp/naver_login.pages.inc
-------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
-------------------------------------------------------------------------
88 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
131 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
168 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found 4
-------------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------

i didn't have the login functionality but i enabled the module, and i think it was written in a good way, but i think it's more recommended as a standards wise to move all your custom function to the naver_login.pages.inc file and keep the naver_login.module file to drupal hooks.

Thank you and good luck.

Hello,

You have missed the hook_help. You should use it hook_help. So that it can be helpful for the new user to know the functionality of this module.

Thanks

Hi,

Using $_GET without sanitizing it first creates the possibility for XSS. Sanitize this first before using it.

'code' => $_GET['code']

I also did some checks on the complexity level of the code. Some files contain almost unreadable code due to the fact that the number of lines per function is too large. In order to have the community in helping you to support this module, try to lower the complexity level of the code.

Some examples:

naver_login.pages.inc

Function naver_login_auth_request contains 35 lines of code.
Function naver_login_auth_retrieve contains 185 lines of code.
Function naver_login_auth_register contains 46 lines of code.
...

Just to give you some insights on how to lower the complexity of your code, I tried this on the function naver_login_auth_retrieve:

/**
 * Callback - Retrieve Naver login Auth context.
 */
function naver_login_auth_retrieve() {
  $naver_login_client_id = NULL;
  $naver_login_client_secret = NULL;

  if (user_is_logged_in() && !isset($_SESSION['naver_login_loggedin_user_want_connect'])) {
    // Block logged user.
    drupal_access_denied();
    return;
  }

  if ((bool) variable_get('naver_login_client_id', FALSE) && (bool) variable_get('naver_login_client_secret', FALSE)) {
    $naver_login_client_id = variable_get('naver_login_client_id', NULL);
    $naver_login_client_secret = variable_get('naver_login_client_secret', NULL);
  }
  else {
    drupal_set_message(t('Cannot initialize Naver Login. Do you configure correctly?'), 'error');
    drupal_access_denied();
    return;
  }

  if (!isset($_GET['code']) || !isset($_GET['state']) || !isset($_SESSION['naver_login']['naver_login_state_token'])) {
    // Block invalid request.
    drupal_access_denied();
    return;
  }

  if ($_GET['state'] != $_SESSION['naver_login']['naver_login_state_token']) {
    // If invalid -> unset token and goto main page.
    if (isset($_SESSION['naver_login']) && isset($_SESSION['naver_login']['naver_login_state_token'])) {
      unset($_SESSION['naver_login']['naver_login_state_token']);
    }
    drupal_goto();
  }

  /*
    Step 1. Retrieve ACCESS TOKEN using $_GET['code'] value.
   */
  $naver_login_access_user_access_token = _naver_login_auth_retrieve_access_token(
    $naver_login_client_id,
    $naver_login_client_secret
  );

  if (!$naver_login_access_user_access_token) {
    // Error.
    drupal_set_message(t('Error occurred while retrieving data. Please try again.'), 'error');
    drupal_goto();
  }

  /*
    Step 2. Retrieve ACCESS TOKEN using $_GET['code'] value.
   */
  $user_profile_xml = _naver_login_auth_user_profile_xml(
    $naver_login_access_user_access_token
  );

  if (!$user_profile_xml) {
    // Error.
    drupal_set_message(t('Error occurred while retrieving data. Please try again.'), 'error');
    drupal_goto();
  }

  /*
    Step 3. Processing user profile.
   */
  $user_profile_array = json_decode(json_encode($user_profile_xml), TRUE);

  $naver_userinfo_enc_id = $user_profile_array['response']['enc_id'];
  $naver_userinfo_email = $user_profile_array['response']['email'];

  $drupal_uid = naver_login_get_drupal_uid_by_naver_enc_id($naver_userinfo_enc_id);

  if ($drupal_uid) {
    $exist_user = user_load($drupal_uid);

    if (!$exist_user) {
      naver_login_revoke_connection($drupal_uid);
      $drupal_uid = NULL;
      $exist_user = NULL;
    }
  }

  if (user_is_logged_in() && isset($_SESSION['naver_login_loggedin_user_want_connect'])) {
    global $user;

    if ($user->mail == $naver_userinfo_email) {
      naver_login_make_connection($user->uid, $naver_userinfo_enc_id, $user_profile_array);
      drupal_set_message(t('Account has been Successfully connected with Naver.'));
    }
    else {
      drupal_set_message(t('Your account email is differ from Naver account. connection failed.'), 'error');
    }

    unset($_SESSION['naver_login_loggedin_user_want_connect']);
    drupal_goto('user/' . $user->uid . '/edit/naver_login');
  }

  // If already connected,
  if ($drupal_uid) {
    // User already registered Naver account to site, log them in.
    $form_state['uid'] = $drupal_uid;
    user_login_submit(array(), $form_state);

    // Need to redirect destination.
    drupal_goto('user');
  }
  elseif ($account_id = naver_login_email_already_exist($naver_userinfo_email)) {
    // The user was not found in the users table, but the.
    // Email from Naver might already have an account.
    $account_info = user_load($account_id);

    // Redirect user to the login page with a message..
    drupal_set_message(t('This email address is already registered to an account. <br />You can connect in user settings form.<br />Account Username : @accountID', array('@accountID' => $account_info->name)));
    $options = array(
      'query' => array(
        'naver_login' => 'true',
      ),
    );
    drupal_goto('user/login', $options);
  }
  else {

    $_SESSION[$_GET['state']]['user_profile'] = serialize($user_profile_array);

    drupal_goto('naver_login/auth/register',
      array(
        'query' => array(
          'state' => $_GET['state'],
        ),
      )
    );

  }
}

/**
 * Get the access token.
 *
 * @param string $naver_login_client_id
 *   The client id for the naver login.
 * @param string $naver_login_client_secret
 *   The client secret for the naver login.
 *
 * @return mixed
 *  Returns FALSE on failure or the access token on success.
 */
function _naver_login_auth_retrieve_access_token($naver_login_client_id, $naver_login_client_secret) {
  $curl_results = _naver_login_make_curl_results(
    'https://nid.naver.com/oauth2.0/token',
    array(
      'grant_type' => 'authorization_code',
      'client_id' => $naver_login_client_id,
      'client_secret' => $naver_login_client_secret,
      'code' => $_GET['code'],
      'state' => $_SESSION['naver_login']['naver_login_state_token'],
    )
  );

  $curl_results_obj = json_decode($curl_results);

  if ($curl_results_obj == NULL || isset($curl_results_obj->error)) {
    // Error.
    return FALSE;
  }
  else {
    // Success.
    return $curl_results_obj->access_token;
  }
}

/**
 * Get the access token.
 *
 * @param string $naver_login_access_user_access_token
 *   The user access token for the naver login.
 *
 * @return mixed
 *  Returns FALSE on failure or the user profile xml object on success.
 */
function _naver_login_auth_user_profile_xml($naver_login_access_user_access_token) {
  $curl_results = _naver_login_make_curl_results(
    'https://apis.naver.com/nidlogin/nid/getUserProfile.xml',
    NULL,
    "Authorization: Bearer " . $naver_login_access_user_access_token
  );

  $user_profile_xml = simplexml_load_string($curl_results, 'SimpleXMLElement', LIBXML_NOCDATA);

  if ($user_profile_xml == NULL
    || !isset($user_profile_xml->result)
    || !isset($user_profile_xml->result->resultcode)
    || $user_profile_xml->result->resultcode != '00') {
      return FALSE;
  }

  return $user_profile_xml;
}

/**
 * Make and get the curl result.
 *
 * @param string $url
 *   The url to call through curl.
 * @param array $http_params
 *   HTTP Params if required for the curl call.
 * @param string $authorization
 *   The authorization string if required.
 *
 * @return mixed
 *   Returns TRUE or the result on success and FALSE on failure.
 */
function _naver_login_make_curl_results($url, $http_params = NULL, $authorization = NULL) {
  $curl_connection = curl_init();
  curl_setopt_array(
    $curl_connection,
    _naver_login_prepare_curl_opt($url, $http_params, $authorization)
  );
  $curl_results = curl_exec($curl_connection);
  curl_close($curl_connection);

  return $curl_results;
}

/**
 * Prepare the curl options for the curl exec.
 *
 * @param string $url
 *   The url to call through curl.
 * @param array $http_params
 *   HTTP Params if required for the curl call.
 * @param string $authorization
 *   The authorization string if required.
 *
 * @return array
 *   Returns an array of prepared curl options.
 */
function _naver_login_prepare_curl_opt($url, $http_params = NULL, $authorization = NULL) {
  $curl_opt = array(
    CURLOPT_URL => $url,
    CURLOPT_POST => TRUE,
    CURLOPT_RETURNTRANSFER => TRUE,
    CURLOPT_CONNECTTIMEOUT => 10,
    CURLOPT_TIMEOUT => 10,
    CURLOPT_HTTPHEADER => array(
      "Host: nid.naver.com",
      "Pragma: no-cache",
      "Accept: */*",
    ),
  );

  if (!empty($http_params)) {
    $curl_opt[URLOPT_POSTFIELDS] = http_build_query($http_params),
  }

  if (!empty($authorization)) {
    $curl_opt[CURLOPT_HTTPHEADER][] = $authorization;
  }

  return $curl_opt;
}

I am changing status basing on the last comments.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.