Problem/Motivation

Social Auth should provide configuration options to allow site builders manage the behavior of the site in some circumstances. We should be able to provide settings options similar to the ones of Simple FB Connect

This issue should be approached when this issue is solved.

Proposed resolution

Provide the functionality necessary to make the options on the form work.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gvso created an issue. See original summary.

denutkarsh’s picture

Assigned: Unassigned » denutkarsh

I'd like to provide the patch for this issue.

denutkarsh’s picture

Issue summary: View changes
denutkarsh’s picture

Status: Active » Needs work
FileSize
1.43 KB

IMHO we could easily implement these features in authenticateUser() method #2848144: Create authenticate method. Since the patch hasn't been committed for that issue till now, so i think currently this patch is of no use. Indents still nodes to be fixed on needs line and i haven't implemented "Disable Login Role" feature. Anyways, i am think of something like this.

denutkarsh’s picture

Assigned: denutkarsh » Unassigned
Status: Needs work » Needs review

Here is the patch. Tested it with "Social Auth Twitter", it works correctly.

denutkarsh’s picture

FileSize
1.58 KB
2.16 KB

Here is the patch. Tested it with "Social Auth Twitter", it works correctly.

denutkarsh’s picture

FileSize
1.58 KB
391 bytes

Oops, forgot to correct route path. Here is the new patch.
Edit: Patch name is not right because of the confusing between the comment number.

denutkarsh’s picture

FileSize
1.83 KB
926 bytes

Forgot to implement "Post login path". Here is the new patch, which all contains the fix for a bug for "Disabled Roles".

gvso’s picture

Status: Needs review » Needs work

Thanks!

  1. +++ b/src/SocialAuthUserManager.php
    @@ -98,11 +98,25 @@ class SocialAuthUserManager {
    +      // Check if Admin can authenticate.
    +      if($config->get('disable_admin_login')){
    +        if($drupal_user->id() == 1) {
    +          return $this->redirect('user.login');
    +        }
    +      }
    

    I'd prefer this logic to be place in a different method. Thus, implementers which do not use the authenticateUser method can check this setting. Also, use drupal_set_message to let the user knows why he couldn't log in.

  2. +++ b/src/SocialAuthUserManager.php
    @@ -98,11 +98,25 @@ class SocialAuthUserManager {
    +      // Check for Disabled roles.
    +      $disabled_roles = $config->get('disabled_roles');
    +      foreach($disabled_roles as $role){
    +        if(!empty($role) && $drupal_user->hasRole($role)){
    +          return $this->redirect('user.login');
    +        }
    +      }
    

    Same as above.

  3. +++ b/src/SocialAuthUserManager.php
    @@ -124,7 +138,14 @@ class SocialAuthUserManager {
    +        if($config->get('redirect_user_form')) {
    +          return $this->redirect('entity.user.edit_form', [
    +            'user' => $drupal_user->id(),
    +          ]);
    +        }
    +        else{
    +          return $this->redirect('user.page');
    +        }
    

    Same here. Plus, I think if the new created user is not redirected to the edit form, it should be redirected to post_login_path

denutkarsh’s picture

Status: Needs work » Needs review
FileSize
3.96 KB
4.79 KB

Here is the new patch.

denutkarsh’s picture

Issue tags: +beta blocker
FileSize
7.14 KB
4.35 KB

Here is the new patch, which redirect to route if it exists or else it just redirects to path.

gvso’s picture

Status: Needs review » Needs work

This looks better, but I have a few suggestions.

  1. +++ b/src/SocialAuthUserManager.php
    @@ -100,9 +105,17 @@ class SocialAuthUserManager {
    +      if ($redirect = $this->isAdminDisabled($drupal_user)) {
    ...
    +      if ($redirect = $this->isUserDisabled($drupal_user)) {
    
    @@ -124,7 +137,12 @@ class SocialAuthUserManager {
    +        if ($redirect = $this->redirectToUserForm($drupal_user)) {
    

    I think it is better if this methods return TRUE/FALSE values. First, it would allow implementers which don't use the authenticateUser method to provide their own messages/functionality. Second, you avoid the assignment operation inside the if clause

  2. +++ b/src/SocialAuthUserManager.php
    @@ -333,6 +351,92 @@ class SocialAuthUserManager {
    +      drupal_set_message($this->t('Authentication for Admin is disabled for Security Reasons.'), 'error');
    

    '..for Admin (user 1).'

  3. +++ b/src/SocialAuthUserManager.php
    @@ -333,6 +351,92 @@ class SocialAuthUserManager {
    +        drupal_set_message($this->t('Authentication for @role is disabled for Security Reasons.', array('@role' => $role)), 'error');
    

    '.. is disabled.'

gvso’s picture

Status: Needs work » Needs review
FileSize
11.55 KB
10.29 KB

Here's a new patch for this issue.

denutkarsh’s picture

@gvso The new patch, looks good to me. But should we follow the first suggestion in #12 since we have already removed the assignment operation inside the if clause ?

gvso’s picture

What do you mean by following the first suggestion?

denutkarsh’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, didn't noticed that you are following the first suggestion i.e you are returning TRUE/FALSE values.

  • denutkarsh committed bc6c06c on 8.x-1.x authored by gvso
    Issue #2821853 by denutkarsh, gvso: Provide Social Auth settings:...
denutkarsh’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! I've committed it on 8.x-1.x

denutkarsh’s picture

Issue tags: -beta blocker

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.