The website encountered an unexpected error. Please try again later.
TypeError: Argument 2 passed to snsw_openid_connect_openid_connect_pre_authorize() must be an instance of Drupal\user\UserInterface, boolean given in mymodule_openid_connect_openid_connect_pre_authorize()
CommentFileSizeAuthor
#5 openid_connect.hook-pre-authorize-account.2940867-5.patch955 bytessun
#3 2940867-3.patch1.46 KBjibran
#2 review-only-2940867-2.patch903 bytesjibran
#2 2940867-2.patch1.86 KBjibran

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new 2940867-2.patch1.86 KB
new review-only-2940867-2.patch903 bytes

Here is the patch.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new 2940867-3.patch1.46 KB

This one is better. I'm opinion replacing the account in openid_connect_connect_current_user is fine but doing so in openid_connect_complete_authorization doesn't make sense.

mario steinitz’s picture

mario steinitz
Location Hong Kong Central
commented

It makes perfectly sense for certain use cases (e.g. denying access based on claims). But you are right, the signature of that hook is false. As $account can be an instance of \Drupal\user\UserInterface or FALSE.

So the better (and less invasive) approach to your issue without breaking functionality for other plugins would be removing the UserInterface type hint from the $account parameter and checking for FALSE/instance of UserInterface within your hook implementation.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new openid_connect.hook-pre-authorize-account.2940867-5.patch955 bytes

removing the UserInterface type hint from the $account parameter and checking for FALSE/instance of UserInterface within your hook implementation

+1, but that is also an API change (if removing a type-hint from a hook signature can be counted as API change).

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Needs work

Let's say I have a case where I want to assign a role to the new user who has just been connected. Right now with #3 I can do this.


/**
 * Implements hook_openid_connect_pre_authorize().
 */
function my_openid_connect_openid_connect_pre_authorize(array $tokens, UserInterface $account, array $userinfo, $plugin_id, $sub) {
  $roles = $account->getRoles(TRUE);
  if (empty($roles)) {
    $account->addRole('some_role');
    $account->save();
  }
}

There is no way to do that with #5.

mario steinitz’s picture

mario steinitz
Location Hong Kong Central
commented

@jibran: That is true. But the purpose of this existing hook is not manipulating the user properties. It rather is intended for checking authorization constraints or letting other modules identify and provide an account for authorization.

As it is very likely that another module is using this hook already for the above mentioned purpose, we shouldn't move it to another position, but fix the issue with the type hint only, as it was done in #5.

Then we can suggest another hook for adding user properties just before authorization/when mapping claims to user properties, which btw. is inspired by a use-case similar to yours. We wanted to add/change user roles according to attributes provided by the IdP.

mario steinitz’s picture

mario steinitz
Location Hong Kong Central
commented
Status: Needs work » Needs review

  • Mario Steinitz committed 59ea638 on 8.x-1.x authored by sun 
    Issue #2940867 by sun: hook_openid_connect_pre_authorize can result in...
mario steinitz’s picture

mario steinitz
Location Hong Kong Central
commented
Status: Needs review » Fixed

I committed the hook signature change as of patch in #5.

@jibran: Please try using the newly introduced hook_openid_connect_userinfo_save() for your customizations and let me know, whether you need any further assistance.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

Unfortunately, you can't save role using hook_openid_connect_userinfo_save() becuase openid_connect_save_userinfo() doesn't handle that case. I created #2962023: Allow adding roles in \Drupal\openid_connect\OpenIDConnect::createUser using userinfo instead.

Status: Fixed » Closed (fixed)

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