microsoftgraph / msgraph-sdk-php

Microsoft Graph Library for PHP.
Other
566 stars 143 forks source link

Reuse access_token from OAuth login #1407

Open ian-paqt opened 9 months ago

ian-paqt commented 9 months ago

Hi, thanks for getting the v2.0 update out. It looks promising.

We are currently using v1.x in a project. After an OAuth request we store the access_token. That access_token is than later used to fetch data from the api with this library. In v1.x this used to work like:

$client = new Graph();

$domainRequest = $client
            ->setAccessToken($accessToken)
            ->createCollectionRequest('GET', '/domains');

From the documentation and upgrade guide I cannot see how we should handle this in v2.0. Am I missing something? Should we still be able to grab the access_token from somewhere to store for later use?

Ndiritu commented 9 months ago

Hi @ian-paqt. For v2.0 the SDK handles the OAuth request, stores the access token and refreshes the token for you. You'll only need to initialise the Auth provider with the relevant credentials depending on the OAuth flow you're app uses: client-credentials (application permissions), authorization code (delegated permissions) etc and pass any scopes

Here's how to pass your OAuth credentials

Here's how to pass them to the GraphServiceClient

Thanks for the feedback. Updating the upgrade guide.

ian-paqt commented 9 months ago

Alright, that sounds neat!

And could you specify what tentantId is needed in the TokenRequestContext? Is that the ID of the tentant you want to retrieve data from, or the tenant that the application is part of (where clientId and clientSecret are also from)?

Ndiritu commented 9 months ago

@ian-paqt should be the tenant that you want to request data from. However, if your application is multi-tenant you can pass "common"

Checking the Endpoints on your App Registrations page in Azure would also be useful.

Orrison commented 9 months ago

@Ndiritu I am also having this issue, and I am unsure of how the changes to have the SDK "stores the access token and refreshes the token for you" work in this scenario.

I have already had my users go through the OAuth flow and redeem the authorization code for an access_token and refresh_token. I am then storing these so that they can be used to call graph endpoints as the User, such as syncing my applications calendar to their outlook calendar. Without requiring the User to go through the OAuth flow every time they visit the page to retrieve an authorization code and to allow the application to do things with the credentials in the background.

Surely, there must be some way to provide an already generated access_token/refresh_token to the client, right?

Are authorization codes not short-lived? So with the 2.0 SDK, the User would have to go through the OAuth flow for every request that the Client needs to be instantiated?

Ndiritu commented 9 months ago

Thanks for the feedback. We need to document this better.

Using the info in the TokenRequestContext e.g. AuthorizationCodeContext the SDK fetches a token for the requests made on a GraphServiceClient object. However, for a refresh token to also be requested, the offline_access scope needs to be included when instantiating the GraphServiceClient i.e.

$tokenRequestContext = new AuthorizationCodeContext(
    'tenantId',
    'clientId',
    'clientSecret',
    'authCode',
    'redirectUri'
);

// Adds offline_access scope so that a refresh token is returned during the token request
$scopes = ['User.Read', 'Mail.ReadWrite', 'offline_access'];
$graphServiceClient = new GraphServiceClient($tokenRequestContext, $scopes);

The same $tokenRequestContext can be re-used to initialise other GraphServiceClients and it will contain a reference to the previously requested access token &/or refresh token of the particular user.

This way the SDK re-uses the cached access token and refreshes it if it's expired and your user doesn't have to authenticate for every request.

Does this help @Orrison @quentinxp?

Ndiritu commented 9 months ago

I would recommend the approach above however, should you want to handle the tokens yourself:


$authenticationProvider = new CustomAuthenticationProvider($tokenRequestContext, $scopes);
$requestAdapter = new GraphRequestAdapter($authenticationProvider);
$graphServiceClient = GraphServiceClient::createWithRequestAdapter($requestAdapter);
Orrison commented 9 months ago

Thanks for the feedback. We need to document this better.

Using the info in the TokenRequestContext e.g. AuthorizationCodeContext the SDK fetches a token for the requests made on a GraphServiceClient object. However, for a refresh token to also be requested, the offline_access scope needs to be included when instantiating the GraphServiceClient i.e.

$tokenRequestContext = new AuthorizationCodeContext(
    'tenantId',
    'clientId',
    'clientSecret',
    'authCode',
    'redirectUri'
);

// Adds offline_access scope so that a refresh token is returned during the token request
$scopes = ['User.Read', 'Mail.ReadWrite', 'offline_access'];
$graphServiceClient = new GraphServiceClient($tokenRequestContext, $scopes);

The same $tokenRequestContext can be re-used to initialise other GraphServiceClients and it will contain a reference to the previously requested access token &/or refresh token of the particular user.

This way the SDK re-uses the cached access token and refreshes it if it's expired and your user doesn't have to authenticate for every request.

Does this help @Orrison @quentinxp?

It is unclear to me how this object could be re-used between requests, let alone sessions. Is it expected that this entire object be persisted in a database or cache?

It just seems to me that, though this new functionality of handling the tokens for you is great, it lacks the ability to re-use tokens already obtained out of the box. Which is a very common, if not one of the most used, scenario of interaction with Graph is it not?

Orrison commented 9 months ago

I would recommend the approach above however, should you want to handle the tokens yourself:

  • Extend the GraphPhpLeagueAccessTokenProvider and override getAuthorizationTokenAsync() to return an access token.
  • Extend the GraphPhpLeagueAuthenticationProvider and override getAccessTokenProvider() to return your custom AccessTokenProvider from the step above.
  • You can then pass this custom authentication provider when initialising the GraphServiceClient as follows:
$authenticationProvider = new CustomAuthenticationProvider($tokenRequestContext, $scopes);
$requestAdapter = new GraphRequestAdapter($authenticationProvider);
$graphServiceClient = GraphServiceClient::createWithRequestAdapter($requestAdapter);

Interesting, this does get us closer to what would be expected to be possible. How would I provide it the refresh_token as well?

ian-paqt commented 9 months ago

It is unclear to me how this object could be re-used between requests, let alone sessions. Is it expected that this entire object be persisted in a database or cache?

Yes, this is my exact question!

This way the SDK re-uses the cached access token and refreshes it if it's expired and your user doesn't have to authenticate for every request.

In our application the user authenticates one time. I could use that to instantiate a AuthorizationCodeContext. But the next thing I want to do is access the MS graph in a server side job or queued message. In that context, how should I build a new $tokenRequestContext?

Ndiritu commented 9 months ago

@Orrison @ian-paqt yes, the expectation would be that you cache the $tokenRequestContext preferrably in memory for re-use. Would this be ideal?

Ndiritu commented 9 months ago

I would recommend the approach above however, should you want to handle the tokens yourself:

  • Extend the GraphPhpLeagueAccessTokenProvider and override getAuthorizationTokenAsync() to return an access token.
  • Extend the GraphPhpLeagueAuthenticationProvider and override getAccessTokenProvider() to return your custom AccessTokenProvider from the step above.
  • You can then pass this custom authentication provider when initialising the GraphServiceClient as follows:
$authenticationProvider = new CustomAuthenticationProvider($tokenRequestContext, $scopes);
$requestAdapter = new GraphRequestAdapter($authenticationProvider);
$graphServiceClient = GraphServiceClient::createWithRequestAdapter($requestAdapter);

Interesting, this does get us closer to what would be expected to be possible. How would I provide it the refresh_token as well?

With this approach, all your token requests using the GraphServiceClient would go through your custom AccessTokenProvider. Therefore, getAuthorizationTokenAsync() will need to always return a valid access token. This means it should be able to request new access tokens & refresh tokens, perform refresh requests if necessary and create a cache for re-use across sessions/requests.

This custom AccessTokenProvider would already have access to a PHPLeague OAuth 2.0 provider that you can use to request access tokens and refresh them.

You can then choose to cache these tokens and re-use them within your custom class.

You can check our current implementation for reference

Orrison commented 9 months ago

@Orrison @ian-paqt yes, the expectation would be that you cache the $tokenRequestContext preferrably in memory for re-use. Would this be ideal?

Not really. It would be preferable not to have to cache an entire object and persist these between sessions/requests. Especially since this would also mean I would need to cache this object, per User and create a paradigm for making sure I am using the correct cached object per User... It would be best to simply be able to generate a client easily using an existing and valid access_token and/or refresh_token

With this approach, all your token requests using the GraphServiceClient would go through your custom AccessTokenProvider. Therefore, getAuthorizationTokenAsync() will need to always return a valid access token. This means it should be able to request new access tokens & refresh tokens, perform refresh requests if necessary and create a cache for re-use across sessions/requests.

This custom AccessTokenProvider would already have access to a PHPLeague OAuth 2.0 provider that you can use to request access tokens and refresh them.

You can then choose to cache these tokens and re-use them within your custom class.

You can check our current implementation for reference

Hmm, I will have to explore this further soon. Still unsure the best way to provide the existing refresh_token here, but I have yet to dig into it too much.

It would be great if this functionality was provided. I think this scenario is just too common, if not the single most common scenario, for the package not to support it natively, IMO.

Ndiritu commented 9 months ago

@Orrison @ian-paqt yes, the expectation would be that you cache the $tokenRequestContext preferrably in memory for re-use. Would this be ideal?

Not really. It would be preferable not to have to cache an entire object and persist these between sessions/requests. Especially since this would also mean I would need to cache this object, per User and create a paradigm for making sure I am using the correct cached object per User... It would be best to simply be able to generate a client easily using an existing and valid access_token and/or refresh_token

With this approach, all your token requests using the GraphServiceClient would go through your custom AccessTokenProvider. Therefore, getAuthorizationTokenAsync() will need to always return a valid access token. This means it should be able to request new access tokens & refresh tokens, perform refresh requests if necessary and create a cache for re-use across sessions/requests. This custom AccessTokenProvider would already have access to a PHPLeague OAuth 2.0 provider that you can use to request access tokens and refresh them. You can then choose to cache these tokens and re-use them within your custom class. You can check our current implementation for reference

Hmm, I will have to explore this further soon. Still unsure the best way to provide the existing refresh_token here, but I have yet to dig into it too much.

It would be great if this functionality was provided. I think this scenario is just too common, if not the single most common scenario, for the package not to support it natively, IMO.

For caching, the TokenRequestContext object e.g. AuthorizationCodeContext contains getCacheKey() that could help with this. It generates a unique key per user, per tenant {tenantId}-{clientId}-{userId} and for client credentials/application permissions/apps without a logged in user, the key is unique per tenant {tenantId}-{clientId}

I agree, it's worth a discussion. I'll get back with feedback from the team.

AlexanderAdelAU commented 9 months ago

Just adding this for info only. I currently download files from user shared folders, and my approach is to get the user to approve the azure application first, and once that is done I request an an offline access token during sign-in a

$authorize_url .= "&scope=$scope offline_access"; //note here we are asking for a refresh token

and then grab the refresh token on redirect, i.e.

 ` $authCode = $_GET['code'];
   $token_url = "https://login.microsoftonline.com/common/oauth2/v2.0/token";
   $scope = "https://graph.microsoft.com/.default";

   $postdata = http_build_query(
    array(
        'grant_type' => 'authorization_code',
        'code' => $authCode,
        'redirect_uri' => $redirect_uri,
        'client_id' => $client_id,
        'client_secret' => $client_secret,
         'scope' => "https://graph.microsoft.com/.default offline_access"
    )
);

$opts = array(
    'http' =>
        array(
            'method' => 'POST',
            'header' => 'Content-Type: application/x-www-form-urlencoded',
            'content' => $postdata
        )
);
$context = stream_context_create($opts);
$response = file_get_contents($token_url, false, $context);
$token = json_decode($response);
// Store the token in a session variable for later use
$access_token = $token->access_token;
$refresh_token = $token->refresh_token;
}`

You now just only need to save the $refresh_token to a database and use it each time you need to download a file for a particular user.

Orrison commented 9 months ago

Just adding this for info only. I currently download files from user shared folders, and my approach is to get the user to approve the azure application first, and once that is done I request an an offline access token during sign-in a

$authorize_url .= "&scope=$scope offline_access"; //note here we are asking for a refresh token

and then grab the refresh token on redirect, i.e.

 ` $authCode = $_GET['code'];
   $token_url = "https://login.microsoftonline.com/common/oauth2/v2.0/token";
   $scope = "https://graph.microsoft.com/.default";

   $postdata = http_build_query(
    array(
        'grant_type' => 'authorization_code',
        'code' => $authCode,
        'redirect_uri' => $redirect_uri,
        'client_id' => $client_id,
        'client_secret' => $client_secret,
         'scope' => "https://graph.microsoft.com/.default offline_access"
    )
);

$opts = array(
    'http' =>
        array(
            'method' => 'POST',
            'header' => 'Content-Type: application/x-www-form-urlencoded',
            'content' => $postdata
        )
);
$context = stream_context_create($opts);
$response = file_get_contents($token_url, false, $context);
$token = json_decode($response);
// Store the token in a session variable for later use
$access_token = $token->access_token;
$refresh_token = $token->refresh_token;
}`

You now just only need to save the $refresh_token to a database and use it each time you need to download a file for a particular user.

So are you using the new Client? And if so, how are you instantiating it with the stored refresh_token?

TheSnip3r commented 8 months ago

any update about this one? I'm also having many access tokens for my users already existed, and I'm not able to reuse them in v2 like I did before in v1.

Squad-G commented 8 months ago

For caching, the TokenRequestContext object e.g. AuthorizationCodeContext contains getCacheKey() that could help with this. It generates a unique key per user, per tenant {tenantId}-{clientId}-{userId} and for client credentials/application permissions/apps without a logged in user, the key is unique per tenant {tenantId}-{clientId}

I agree, it's worth a discussion. I'll get back with feedback from the team.

I never comment but, this is a very poor implementation.

How do we reuse the object ? How do we get the token ? I can get the key OK but how do I set the key? It requires AccessToken object which is not given.

The documentation is very bad. It's a terrible package.

inserve-paul commented 8 months ago

I'm having the same situation, the v2.0 version of this SDK looks promising. However since we're re-using the accessTokens and can't use it (or retrieve it) we can't upgrade to this new version.

It would be great if you could add a method to retrieve/refresh the AccessToken and also pass it when creating the GraphServiceClient

frank-inserve commented 7 months ago

@Orrison @ian-paqt yes, the expectation would be that you cache the $tokenRequestContext preferrably in memory for re-use. Would this be ideal?

Not really. It would be preferable not to have to cache an entire object and persist these between sessions/requests. Especially since this would also mean I would need to cache this object, per User and create a paradigm for making sure I am using the correct cached object per User... It would be best to simply be able to generate a client easily using an existing and valid access_token and/or refresh_token

With this approach, all your token requests using the GraphServiceClient would go through your custom AccessTokenProvider. Therefore, getAuthorizationTokenAsync() will need to always return a valid access token. This means it should be able to request new access tokens & refresh tokens, perform refresh requests if necessary and create a cache for re-use across sessions/requests. This custom AccessTokenProvider would already have access to a PHPLeague OAuth 2.0 provider that you can use to request access tokens and refresh them. You can then choose to cache these tokens and re-use them within your custom class. You can check our current implementation for reference

Hmm, I will have to explore this further soon. Still unsure the best way to provide the existing refresh_token here, but I have yet to dig into it too much. It would be great if this functionality was provided. I think this scenario is just too common, if not the single most common scenario, for the package not to support it natively, IMO.

For caching, the TokenRequestContext object e.g. AuthorizationCodeContext contains getCacheKey() that could help with this. It generates a unique key per user, per tenant {tenantId}-{clientId}-{userId} and for client credentials/application permissions/apps without a logged in user, the key is unique per tenant {tenantId}-{clientId}

I agree, it's worth a discussion. I'll get back with feedback from the team.

@Ndiritu, is there any update from the team?

jamesBotwright commented 6 months ago

@Ndiritu Do the team have any comments to make? We are trying to make use of SDK 2 but without a method of re-using the tokens we cannot understand how we can keep users logged in across sessions when they use different parts of our web application. We successfully authenticate and retrieve an authCode but we do not store the authCode in our dB as it has a very short expiry, with version 1 of the SDK we stored the access token and the refresh tokens, when the user accessed the web application again in a different session if the token had expired we passed the refresh token to get a new one. We are not sure if we are approaching the flow in the correct way for SDK 2 to keep users logged in across sessions it appears the same issue the above users are experiencing?

Ndiritu commented 6 months ago

@jamesBotwright thanks for reaching out and apologies for the delay on this. Hoping to have a solution reviewed and released by early next week.

uncaught commented 6 months ago

This is a blocker for us, too. We already store and renew our user's access tokens, and that's a system build for multiple OAuth services, not just the msgraph. This already uses League\OAuth2. And we make sure to renew the token before calling any SDK.

And even after reading through here, I still don't know where this SDK is storing or caching anything. There is no storage interface I have to configure. Do you store this on the hard drive or what? That won't work for spawned container instances.

Please don't be opinionated on how the access token is obtained or stored. If you want to offer a feature for this, fine, but please make it optional (preferably even in a separate package).

Ndiritu commented 6 months ago

Please upgrade to version 2.3.0 and check the docs on how to pass your access tokens to the library and re-use access tokens across multiple requests from a user without having them log in each time.

Your feedback is welcome and appreciated.

bilalthepunjabi commented 5 months ago
<?php

use Microsoft\Kiota\Authentication\Cache\AccessTokenCache;
use Microsoft\Kiota\Authentication\Oauth\CAEConfigurationTrait;
use Microsoft\Kiota\Authentication\Oauth\TokenRequestContext;
use Microsoft\Kiota\Authentication\Oauth\BaseSecretContext;
use League\OAuth2\Client\Token\AccessToken;

class MicrosoftAccessTokenCache implements AccessTokenCache {

    public array $accessTokens = [];

    /**
     * Returns the access token with the given identity from the cache
     *
     * @param string $identity
     * @return AccessToken|null
     */
    public function getAccessToken(string $identity): ?AccessToken
    {
        return $this->accessTokens[$identity] ?? null;
    }

    /**
     * Adds an access token with the given identity to the cache
     *
     * @param string $identity
     * @param AccessToken $accessToken
     * @return void
     */
    public function persistAccessToken(string $identity, AccessToken $accessToken): void
    {
        $this->accessTokens[$identity] = $accessToken;
    }
}

class MicrosoftOnBehalfOfContext extends BaseSecretContext implements TokenRequestContext
{
    use CAEConfigurationTrait;

    /**
     * @var string|null
     */
    private ?string $cacheKey = null;

    /**
     * @var string
     */
    private string $assertion;

    /**
     * @var array<string, string>
     */
    private array $additionalParams;

    /**
     * @param string $tenantId
     * @param string $clientId
     * @param string $clientSecret
     * @param string $assertion initial access token sent to the current API
     * @param array<string, string> $additionalParams extra params to be added to token request
     */
    public function __construct(string $tenantId, string $clientId, string $clientSecret, string $assertion, array $additionalParams = [])
    {
        if (!$assertion) {
            throw new \InvalidArgumentException("Assertion cannot be empty");
        }
        $this->assertion = $assertion;
        $this->additionalParams = $additionalParams;
        parent::__construct($tenantId, $clientId, $clientSecret);
    }

    /**
     * Set the identity of the user/application. This is used as the unique cache key
     * For delegated permissions the key is {tenantId}-{clientId}-{userId}
     * For application permissions, they key is {tenantId}-{clientId}
     * @param AccessToken|null $accessToken
     * @return void
     */
    public function setCacheKey(?AccessToken $accessToken = null): void
    {
        if ($accessToken && $accessToken->getToken()) {
            $this->cacheKey = 'me';
        }
    }

    /**
     * Return the identity of the user/application. This is used as the unique cache key
     * For delegated permissions the key is {tenantId}-{clientId}-{userId}
     * For application permissions, they key is {tenantId}-{clientId}
     * @return string|null
     */
    public function getCacheKey(): ?string
    {
        return $this->cacheKey;
    }

    /**
     * @inheritDoc
     */
    public function getParams(): array
    {
        return array_merge($this->additionalParams, parent::getParams(), [
            'assertion' => $this->assertion,
            'grant_type' => $this->getGrantType(),
            'requested_token_use' => 'on_behalf_of'
        ]);
    }

    /**
     * @inheritDoc
     */
    public function getGrantType(): string
    {
        return 'urn:ietf:params:Oauth:grant-type:jwt-bearer';
    }
}

$trc = new MicrosoftOnBehalfOfContext(
    'common',
    env('MICROSOFT_CLIENT_ID'),
    env('MICROSOFT_CLIENT_SECRET'),
    $stored_access_token
);

$access_token = new AccessToken(
    [
        'access_token' => $stored_access_token,
        'refresh_token' => $stored_refresh_token,
        'expires' => $expires_in
    ]
);

$trc->setCacheKey($access_token);    
$cache = new MicrosoftAccessTokenCache();
$cache->persistAccessToken('me',$access_token);

$service = GraphServiceClient::createWithAuthenticationProvider(
    GraphPhpLeagueAuthenticationProvider::createWithAccessTokenProvider(
        GraphPhpLeagueAccessTokenProvider::createWithCache(
            $cache,
            $trc,
            $stored_scopes,
        )
    )
);

$user = $service->me()->get()->wait();

--working... I have to implement a custom AccessTokenCache and TokenRequestContext interfaces to go around your caching token mechanism. Your TokenRequestContext implementations use DelegatedPermissionTrait that has this implementation to set CacheKey :

public function setCacheKey(?AccessToken $accessToken = null): void
    {
        if ($accessToken && $accessToken->getToken()) {
            $tokenParts = explode('.', $accessToken->getToken());
            if (count($tokenParts) == 3) {
                $payload = json_decode(base64_decode($tokenParts[1]), true);
                if (is_array($payload) && array_key_exists('sub', $payload)) {
                    $subject = $payload['sub'];
                    $this->cacheKey = ($subject) ? "{$this->getTenantId()}-{$this->getClientId()}-{$subject}" : null;
                }
            }
        }
    }

But the access_token I have stored after oauth2.0 flow doesn't contain any "." which results in null cacheKey on TokenRequestContext throwing error Unable to initialize cache key for context using access token even I use your provided solution of using InMemoryAccessTokenCache.

uncaught commented 5 months ago

Our implementation:

msgraph v1 ```php use Microsoft\Graph\Graph; class GraphFactory { public function create(string $accessToken): Graph { $graph = new Graph(); $graph->setAccessToken($accessToken); return $graph; } } ```
msgraph v2 ```php use App\Kernel; use League\OAuth2\Client\Token\AccessToken; use Microsoft\Graph\Core\Authentication\GraphPhpLeagueAccessTokenProvider; use Microsoft\Graph\Core\Authentication\GraphPhpLeagueAuthenticationProvider; use Microsoft\Graph\GraphServiceClient; use Microsoft\Kiota\Authentication\Cache\AccessTokenCache; use Microsoft\Kiota\Authentication\Oauth\AuthorizationCodeContext; class GraphFactory { public function create(string $accessToken): GraphServiceClient { $token = new AccessToken([ 'access_token' => $accessToken, 'expires' => time() + 10, //Our AccessTokenProvider makes sure the token is valid for at least 60 seconds ]); $tokenRequestContext = new class extends AuthorizationCodeContext { public function __construct() { //We don't want Microsoft\Graph to request access tokens itself, but all these values may not be empty: parent::__construct('x', 'x', 'x', 'x', 'x'); } public function getCacheKey(): ?string { return 'ignored'; //this ends up as $identity in AccessTokenCache::getAccessToken(), which we don't use } }; $cache = new class($token) implements AccessTokenCache { public function __construct(private readonly AccessToken $token) { } public function getAccessToken(string $identity): ?AccessToken { return $this->token; } public function persistAccessToken(string $identity, AccessToken $accessToken): void { Kernel::$defaultLogger->warning('Microsoft\Graph trying to persist access token!'); } }; return GraphServiceClient::createWithAuthenticationProvider( GraphPhpLeagueAuthenticationProvider::createWithAccessTokenProvider( GraphPhpLeagueAccessTokenProvider::createWithCache($cache, $tokenRequestContext) ) ); } } ```

It's not ideal, but it seems to work.

It would be nice if this work flow could be made a bit easier in the future. This all feels like a hack:

Ndiritu commented 5 months ago

Thank you for your feedback and the workaround @bilalthepunjabi.

Expecting the access token to always be a JWT was a mistake and parsing it isn't recommended. I think generating a UUID to distinguish each user's cached token would be ideal.

Would you @uncaught or @bilalthepunjabi be willing to submit a PR for this?

It would involve changing the logic here to set the cacheKey to {tenantId}-{clientId}-{uuid}. The UUID can be generated using this package which is already a dependency.

roshantwaanabasu commented 3 months ago

@Ndiritu Any update on this for accessing GRAPH SDK using only accessToken?

daverdalas commented 3 months ago

Perhaps this solution will prove useful to those who come here: https://github.com/microsoftgraph/msgraph-sdk-php/issues/1472#issuecomment-2151714684