Add support for Open Id connect

+1

We're using oauth2_server for now because of this. I saw some reference to openid connect in this issue queue but couldn't understand how to implement it.

See also ...
https://www.drupal.org/project/simple_oauth/issues/2944981#comment-13049736

Implementing it with the base fields and an alter hook/event sounds like a good idea.

Some bits from my project that I can share:

services:

services:
  ....openid_connect.identity_provider:
    class: \Drupal\...\OpenIdConnect\UserIdentityProvider
    arguments: ['@entity_type.manager']
  ....openid_connect.authenticated_claim_set:
    class: \OpenIDConnectServer\Entities\ClaimSetEntity
    arguments: [...custom claims....]
    private: true
  ....openid_connect.claim_extractor:
    class: \OpenIDConnectServer\ClaimExtractor
    arguments: [['@....openid_connect.authenticated_claim_set']]
  ....openid_connect.scope_repository:
    public: false
    class: \Drupal\...\OpenIdConnect\OpenIdConnectScopeRepository
    decorates: simple_oauth.repositories.scope
    arguments: ['@....openid_connect.scope_repository.inner']

service provider that alters the response type service

$definition = $container->getDefinition('simple_oauth.server.response_type');
    $definition->setClass(\OpenIDConnectServer\IdTokenResponse::class);
    $definition->setArguments([
      new Reference('...openid_connect.identity_provider'),
      new Reference('...openid_connect.claim_extractor'),
    ]);

the identity provider:

/**
 * A user identity provider for the OpenID Connect integration.
 */
class UserIdentityProvider implements IdentityProviderInterface {

  /**
   * The entity type manager.
   *
   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
   */
  protected $entityTypeManager;

  /**
   * UserIdentityProvider constructor.
   *
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   *   The entity type manager.
   */
  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
    $this->entityTypeManager = $entity_type_manager;
  }

  /**
   * {@inheritdoc}
   */
  public function getUserEntityByIdentifier($identifier) {

    /** @var \Drupal\user\UserInterface $user */
    $user = $this->entityTypeManager->getStorage('user')->load($identifier);

    $user_entity = new UserEntityWithClaims();
    $user_entity->setIdentifier($identifier);

    $claims = [
      'name' => $user->getDisplayName(),
      'preferred_username' => $user->getAccountName(),
      'email' => $user->getEmail(),
      'email_verified' => TRUE,
      'locale' => $user->getPreferredLangcode(),
      'updated_at' => $user->getChangedTime(),
    ];

    $user_entity->setClaims($claims);
    return $user_entity;
  }

}

The UserEntityWithClaims class:

use Drupal\simple_oauth\Entities\UserEntity;

/**
 * A user entity with claims.
 */
class UserEntityWithClaims extends UserEntity {

  /**
   * The claims.
   *
   * @var array
   */
  protected $claims;

  /**
   * Returns the claims.
   *
   * @return array
   *   List of claims.
   */
  public function getClaims() {
    return $this->claims;
  }

  /**
   * Sets the claims.
   *
   * @param array $claims
   *   List of claims.
   */
  public function setClaims(array $claims) {
    $this->claims = $claims;
  }

}

The OpenIdConnectScopeRepository, this part is a bit weird, openid connect defines a bunch of fixed scopes and I didn't want to define them all as roles, so I basically hardcoded them.

/**
 * OpenID Connect scope repository decorator.
 */
class OpenIdConnectScopeRepository implements ScopeRepositoryInterface {

  use StringTranslationTrait;

  /**
   * The inner scope repository.
   *
   * @var \League\OAuth2\Server\Repositories\ScopeRepositoryInterface
   */
  protected $innerScopeRepository;

  /**
   * OpenIdConnectScopeRepository constructor.
   *
   * @param \League\OAuth2\Server\Repositories\ScopeRepositoryInterface $inner_scope_repository
   *   The inner scope repository.
   */
  public function __construct(ScopeRepositoryInterface $inner_scope_repository) {
    $this->innerScopeRepository = $inner_scope_repository;
  }

  /**
   * {@inheritdoc}
   */
  public function getScopeEntityByIdentifier($identifier) {
    // First check if this scope exists as a role.
    $role_scope = $this->innerScopeRepository->getScopeEntityByIdentifier($identifier);
    if ($role_scope) {
      return $role_scope;
    }

    // Fall back to a fixed list of OpenID scopes.
    $openid_scopes = $this->getOpenIdScopes();
    if (isset($openid_scopes[$identifier])) {
      return new OpenIdConnectScopeEntity($identifier, $openid_scopes[$identifier]);
    }

    return NULL;
  }

  /**
   * {@inheritdoc}
   */
  public function finalizeScopes(array $scopes, $grantType, ClientEntityInterface $clientEntity, $userIdentifier = NULL) {
    $finalized_scopes = $this->innerScopeRepository->finalizeScopes($scopes, $grantType, $clientEntity, $userIdentifier);

    // Make sure that the openid scopes are in the user list.
    $openid_scopes = $this->getOpenIdScopes();
    foreach ($scopes as $scope) {
      if (isset($openid_scopes[$scope->getIdentifier()])) {
        $finalized_scopes = $this->addRoleToScopes($finalized_scopes, new OpenIdConnectScopeEntity($scope->getIdentifier(), $openid_scopes[$scope->getIdentifier()]));
      }
    }
    return $finalized_scopes;
  }

  /**
   * Returns fixed OpenID Connect scopes.
   *
   * @return array
   *   A list of scope names keyed by their identifier.
   */
  protected function getOpenIdScopes() {
    $openid_scopes = [
      'openid' => $this->t('User information'),
      'profile' => $this->t('Profile information'),
      'email' => $this->t('E-Mail'),
      'phone' => $this->t('Phone'),
      'address' => $this->t('Address'),
    ];
    return $openid_scopes;
  }

  /**
   * Add an additional scope if it's not present.
   *
   * @param \League\OAuth2\Server\Entities\ScopeEntityInterface[] $scopes
   *   The list of scopes.
   * @param \League\OAuth2\Server\Entities\ScopeEntityInterface $new_scope
   *   The additional scope.
   *
   * @return \League\OAuth2\Server\Entities\ScopeEntityInterface[]
   *   The modified list of scopes.
   */
  protected function addRoleToScopes(array $scopes, ScopeEntityInterface $new_scope) {
    // Only add the role if it's not already in the list.
    $found = array_filter($scopes, function (ScopeEntityInterface $scope) use ($new_scope) {
      return $scope->getIdentifier() == $new_scope->getIdentifier();
    });
    if (empty($found)) {
      // If it's not there, then add it.
      array_push($scopes, $new_scope);
    }

    return $scopes;
  }

}

/**
 * The OpenID Connect scope entity.
 */
class OpenIdConnectScopeEntity extends ScopeEntity implements ScopeEntityNameInterface {

  /**
   * OpenIdConnectScopeEntity constructor.
   *
   * @param string $identifier
   *   The scope identifier.
   * @param string $name
   *   The scope name.
   */
  public function __construct($identifier, $name) {
    $this->setIdentifier($identifier);
    $this->name = $name;
  }

}

That should basically take care of returning those claims in the oauth response, I also implemented a route at /userinfo that returns those claims as well.

This is a fantastic idea. Anyone willing to push this forward during DrupalCon?

> Some bits from my project that I can share

Thanks for sharing that @berdir! What a great starting point!

> openid connect defines a bunch of fixed scopes
> I didn't want to define them all as roles, so I basically hardcoded them

I've only recently starting delving into simple_oauth, but I was wondering about this the guidance relative to roles as well.

I think some flavor of an approach like this makes sense for this use case.

This is an initial implementation that puts into a patch what @Berdir shared above and adds some stuff.

Thoughts / questions:

1. I created a normalizer and a container parameter for the different claims. This way a dev can decorate the normalizer to map additional claims once the data model is known.
2. Since scopes are not tied to roles, should we add manual permission checks in the normalizer? This is a potential access bypass security issue.
3. I haven't gone in depth into the OpenID Connect specification, so maybe there are some fundamental flaws. I plant to do it some day, when I have the time.

@solomonrothman and others. Have you had the time to try this patch?

@e0ipso - Still haven't carved out time to test it yet. Unfortunately, I'm guessing it may be around 3-4 weeks before I'm poised to circle back to it. Sorry! Hopefully others will be able to jump in before I circle back to it. If not, I'm still interested, but it could be a bit before I can focus on it.

I'm currently testing this out and so far pretty happy with it.
One gotcha is that userinfo route is /userinfo and not /oauth/userinfo similar to the other oauth routes - took me a while to figure that out!

@thtas yeah, the correct fix for that discoverability is to implement the route https://[base-server-url]/.well-known/openid-configuration that contains where each endpoint resides. See more info at https://connect2id.com/products/server/docs/api/discovery.

If you are interested in it, please create an issue for that implementation.

the correct fix for that discoverability is to implement the route .well-known/openid-configuration

I think that's covered in this ticket ...

https://www.drupal.org/project/simple_oauth/issues/3033472

... it shouldn't be too bad, but just a matter of carving out time to work on it (which I haven't been able to do yet).

I think that's covered in this ticket ...

Perfect! Thanks @matt_paz!

Finally getting back to this and tested the patch from #13 with the 4.x-dev branch and everything seems to be working well for me.

I haven't tried to do anything special with claims yet, but overall this is looking good.

I do think we might want to adjust the path for the simple_openid_connect.userinfo route to be /oauth/userinfo -- not b/c we won't eventually be able to discover it via well-known/openid-configuration, but to keep common conventions (as @thtas mentioned) and b/c we're less likely to have to deal with collisions where one might predict a person might have a pre-existing route/alias setup for /userinfo. Obviously that route could be altered to compensate for that if it occurred, but it might save others some headaches down the road.

Looks like #13 no longer applies to the latest dev

Below is the merge conflict resolved in the composer.json file:

<<<<<<< HEAD
"drupal/consumers": "^1.2",
"php": ">=7.0"
=======
"steverhoades/oauth2-openid-connect-server": "^1.1",
"drupal/consumers": "^1.2"
>>>>>>> Applying patch from issue 2999521 comment c8c49be54b1d51

As all code changes seemed to be independent from the main module I have taken the code from the provided patch and created a new module Simple OAuth OIDC (Open ID Connect) for it, which I also feel to be a better approach (I personally think that sub-modules should be avoided).

Your'e welcome to contribute over there!

@e0ipso: I already added you as maintainer because I believe it makes sense for you to have access, let me know if this is not OK for any reason. :)

@esolitos I see that the issue status might have thrown you off, however the code has already been merged as a submodule in Simple OAuth. I believe that we should not duplicated code in different repos.

If there are any improvements in your fork the Simple OAuth team will be glad to review and incorporate to this module.

@esolitos I think it's quite a bad idea to fork/duplicate efforts, and even it may be a licensing/copyright issue specially if the original authors didn't approve that, and you added only your name under the fork composer.json. I suggest creating an issue on the webmasters's queue for deleting that fork.

@o0ipso: That's great to hear, however could you point out where is the code? Because it's definitely not in the repo (unless I'm missing something)
https://git.drupalcode.org/project/simple_oauth/-/tree/8.x-4.x/

I believe that we should not duplicated code in different repos.

I personally think that modules-within-modules is a bad drupalism, that said your'e still the project maintainer here so I'll stand by your final decision and if we can get a release containing the simple_openid_connect submodule in the codebase of simple_oauth I'm OK to deprecate the fork I created.
Not that we might encounter some weirdness in the composer package naming now that drupal/simple_openid_connect is taken, so if I am to deprecate the project I created we might need to open an issue on the webmaster queue to get the package name to be re-assigned (if they even do this sort of things).

Regarding changes: I have just some fixes here and there as you can see from the commit history..

@esolitos I think that @penyaskito is coming from a Drupal background when approaching forking. We have seen many duplicated efforts. See:

1. https://www.drupal.org/node/23789
2. https://www.drupal.org/contribute/development#collaboration

I cannot see the submodule in the current development branch. Maybe I was close to merge it but didn't go ahead (?). I am sorry for the confusion I caused you and @penyaskito by my earlier comment. I'll look into the git history to see what is going on. In any case I believe that the original plan to put it in the Simple OAuth module still makes sense.

I cannot see the submodule in the current development branch

Ya, I don't think this has been committed yet.
I'm looking forward to seeing it become officially supported tho! :)

Glad to hear that may be close

Setting the correct status then again.

@e0ipso, @penyaskito: I surely agree with the "collaborate, don't compete", in fact I think that the confusion might have been because I used a wrong term i didn't properly "fork" the module, I merely took the patch content and made it into a standalone module. :)
Without absolutely any intent of competing on anything as it provides features which the simple_oauth module doesn't currently cover.

In any case I believe that the original plan to put it in the Simple OAuth module still makes sense.

As you prefer, jsut let us know when the code is in the repo and at that point I'll add a big red deprecation warning in the simple_openid_connect page. :)

I have been able to test the module based on the patch and it works fine. I am working on integrating Drupal with Moodle through oauth and Openid and it works.

The only problem I see is that the claims are very restricted and when they ask me for a mandatory field in Moodle such as the last name, it does not happen. I could only pass these fields

$claims = [
      'name' => $user->getDisplayName(),
      'preferred_username' => $user->getAccountName(),
      'email' => $user->getEmail(),
      'email_verified' => TRUE,
      'locale' => $user->getPreferredLangcode(),
      'updated_at' => $user->getChangedTime(),
    ];

It would be great to have all the claims like the standar https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims

It would be great to have all the standard claims. I am not a developer. it's just a suggestion. ;-)

Drupal core doesn't have a firstname/lastname concept by default, so that's not possible. Looks like the code currently uses a normalizer to convert the claims, which seems very strange to me, but it should allow you provide your own instead by having a service with a higher weight. That said, it would make much more sense to me to just have a dedicated service for this that could be decorated directly or maybe an event that you can listen to.

Good morning and thanks for the comment @Berdir

The truth is that I would not know how to develop a new service is impossible for me. I know some code but not at this level.

Would it be possible to put the new fields as calls to the database?

It could be done within the claims of this part of the code. The truth is I don't know if this is possible.

Thank you...

The problem is that making something configurable/flexible enough to allow to dynamically use configurable fields, through tokens or something tends to be quite a lot more complicated then just implementing a service/alter so that probably won't happen unless someone needs it for a specific use case for themself or gets paid to implement that.

That said, implementing a settings form that allows to do something like claim_name|[user:some_field:value] and then looping over that might not be too complicated. Maybe someone is interested to do that or if you need it for a client project, you might have the budget to pay someone who can a few hours?

Was just looking back at commits that @esolitos made to that variant. I definitely think those could be good fodder for inclusion here (the sub claim addition, in particular makes a ton of sense).

Another thing we should start looking towards is D9 compatibility (which for this particular patch may be limited to info.yml only).

@esolitos - Wondering if you might be interested in getting an updated patch here to help move this along?

See also: Mateu looking for co-maintainers back in May:
https://drupal.slack.com/archives/C5A70F7D1/p1590050850366900

Relative to lowering the bar for extending a full set of claims, I do wonder if a simple hook ala this ...
https://git.drupalcode.org/project/oauth2_server/blob/8.x-1.x/src/OAuth2...
... inside of getClaimsFromAccount might be a lower threshold bridge for accomplishing this (and perhaps increase adoption as a result), but perhaps we don't want to go there.

> the code currently uses a normalizer to convert the claims, which seems very strange to me

@berdir, what were you envisioning as an alternative?

@matt_paz Right now I'm working on something completely different and for a few weeks I won't have time to look into this. That said it should be just dumping the (sub)module in the module?

I was attempting to use the patch from #26, but ran into some issues. They seem to be caused by some updates in the league/oauth2-server library, which changed some function signatures among other things. Additionally, I was attempting to use Drupal as an openid provider for GitLab, and there was a path missing for the JWKS. I re-rolled the patch with my fixes and the new JWKS endpoint support.

I do not expect this to pass, but it should help get the ball rolling. This change was introduced upstream in this commit.

I do not expect this to pass

I am quite happy to eat my own words.

1. I think we should not do OpenID Connect in a separate submodule. Instead rework it to be in the module and have a config to turn it off (on by default). I will let the community create the issue to turn OpenID Connect off.
2. I considered Berdir's feedback here to maybe avoid the normalizer. I like the idea of normalizer as the proper interface to turn objects into JSON, I think it's the right abstraction. I admit that ergonomics could be better.
3. In the configuration forms, we should link to a documentation page (for now an empty one). This will contain info on how to add additional claims to the response.

I believe that it's fair to create a new major version to introduce this patch. Then we'll switch to the new semantic release naming convention.

The documentation page mentioned above: https://www.drupal.org/docs/contributed-modules/oauth2-openid-connect

+++ b/simple_oauth.routing.yml
@@ -80,3 +80,35 @@ oauth2_token.user_debug:
+
+simple_oauth.userinfo:
+  path: '/userinfo'
+  defaults:
+    _controller: Drupal\simple_oauth\Controller\UserInfo::handle
+    _title: 'OpenID Connect User Info'
+  methods: [GET]
+  requirements:
+    _role: 'authenticated'
+  options:
+    _auth: ['oauth2']

Not a fan of having it the main module. Even if the feature isn't enabled, this route will always be there and will conflict with people using their own integration or the available contrib module for these routes. Even with the major version switch, that will result in quite some work for me in our project to redo our implementation on top of this.

I think that is fair Berdir. My reason to move forward with a single module is that I want the Drupal community to discover and implement standard patterns together. Many people assume that OAuth2 already does what OpenID Connect provides. From here we can implement .well-known and some other follow ups that are in the issue queue.

However, I would hate to put your sites in a rough place because of this. Specially since most of the code here is yours (or your team's) to start with.

Would it be OK if we added this under /simple-oauth/openid-connect/userinfo or /openid-connect/userinfo instead?

I changed the routes to /oauth/userinfo and /oauth/jwks and implemented the opt-out functionality for the OpenID Connect integration.

Hello,

We have integrated the simple_openid_connect from @esolitos, which is based mostly on the patch. We had to fix a few things which could also help your integration.

Regarding the /userinfo endpoint, according to the specs, it should accept GET and POST requests. Also, we found out that this endpoint is being cached. With Dynamic Page Cache enabled, it returned the content of previous users that accessed the endpoint. Maybe we need to ensure this work with DPC and the contexts are correct or even better, not caching the endpoint at all as this endpoint is basically not accessed often and very unique per user.

Thanks for the pointer! I found the place where it says that:

The UserInfo Endpoint MUST support the use of the HTTP GET and HTTP POST methods defined in RFC 2616 [RFC2616].

https://openid.net/specs/openid-connect-core-1_0.html#UserInfo

With Dynamic Page Cache enabled, it returned the content of previous users that accessed the endpoint.

🤔 AFICT the cache context for the user is added properly to the userinfo endpoint. I am probably missing something.

I think this is the good one.

+++ b/src/Controller/UserInfo.php
@@ -0,0 +1,103 @@
+    );
+    return $response
+      ->addCacheableDependency($this->config)
+      ->addCacheableDependency($token)
+      ->addCacheableDependency($this->user);
+  }

> 🤔 AFICT the cache context for the user is added properly to the userinfo endpoint.

Not really.

What this is doing is adding a dependency on that object. that doesn't necessarily mean the _current_ user, it just means that specific user. In order words, a cache tag on user:X. But the user object doesn't know where it came from.

The cache context for current user you need to add yourself explicitly.

Should have a test IMHO that tests it with two users with the same set of permissions. Log in with one, access the page, then login with the other and access it. Should fail right now.

That said, you could also just opt out of caching entirely. Because this is a controller for authenticated users, the only thing that could cache it is Dynamic page cache and that by default blacklists the user cache context so it's not caching it.

So you could simply change to a non-cachable json response and remove all the cache dependency stuff. that's what I did in my project.

I haven't dug deeply into the updated patch yet, but any thoughts on adding something like this to the patch?
https://git.drupalcode.org/project/simple_openid_connect/commit/9bd7c74

Or are you thinking this shouldn't be part of a standard default and should be configured separately?
Perhaps b/c some might prefer to have the uuid be the value for sub, etc?

See also:

https://openid.net/specs/openid-connect-core-1_0.html#IDToken
https://www.drupal.org/project/openid_connect/issues/2999862

Thanks for breathing new life into this issue!

That said, you could also just opt out of caching entirely

So you could simply change to a non-cachable json response and remove all the cache dependency stuff.

That seems reasonable/pragmatic to me.

🤞

And now without caching.

Thanks to all involved, but specially to Berdir and matt_paz.

I haven't tested it yet, but from a quick look at the code, it seems to be looking good.

I don't think this has been committed yet, so I think this should be set back to Needs review for the time being. But please change if I'm mistaken.

Also setting this as a reminder that at some point, it'd be great to get this stub created by Mateo populated with some content aimed at helping people understand how to customize the claims.

https://www.drupal.org/docs/contributed-modules/oauth2-openid-connect/cu...

Ooops. I stand corrected -- just noticed this. :)
https://git.drupalcode.org/project/simple_oauth/commit/46d5151

A note for all that may find their way here, though, that while this issue is tagged as 8.x-4.x-dev, the commit was not made on 8.x-4.x-dev. It was made on 8.x-5.x-dev and appears in the 8.x-5.0.0 release. This confused me.

Ah yes! At the time of pushing the release did not exist in d.o so I could not select it in the dropdown. That probably also caused the missing commit comment in the issue.

Thanks for clarifying that! Also, in 5.x there is a failing test that will be fixed soon.

Sorry one quick comment (I may open this as a separate issue) but can we reverse the naming / value for the open id connect enable flag?

It's really confusing reading the checkbox label with the question mark that says "Disable open id connect?" and a config variable called "disable_openid_connect". Do I check the box or uncheck or...

https://git.drupalcode.org/project/simple_oauth/commit/46d5151#af0bcacb8...

I feel as though the variable should just be called enable_openid_connect and that way the state of it being on is the same as the true/false value of the checkbox.

Just my two cents. But great work everyone getting this in.

@minorOffense would you mind opening a new issue for this? Thank you.