Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gitlab v15 token refresh #596

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/packages/knpu_oauth2_client.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ knpu_oauth2_client:
type: gitlab
client_id: '%env(OAUTH_GITLAB_CLIENT_ID)%'
client_secret: '%env(OAUTH_GITLAB_CLIENT_SECRET)%'
redirect_route: register_gitlab_check
redirect_route: package_gitlab_check
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still possible to register a user with oauth flow? (route: register_gitlab_check (\Buddy\Repman\Controller\OAuth\GitLabController::registerCheck) will create/login users \Buddy\Repman\Controller\OAuth\OAuthController::createAndAuthenticateUser

edit:
this breaks the initial redirect to create the user

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the redirect_url the same in the initial request for the oauth token which is stored and for the token refresh, i changed the config here.

I changed the route for registration to the register_gitlab_check in d22367b

redirect_params: {}
use_state: true
domain: '%env(APP_GITLAB_API_URL)%'
Expand Down
2 changes: 1 addition & 1 deletion src/Controller/OAuth/GitLabController.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public function register(): Response
{
$this->ensureOAuthRegistrationIsEnabled();

return $this->oauth->getClient('gitlab')->redirect(['read_user'], []);
return $this->oauth->getClient('gitlab')->redirect(['read_user'], ['redirect_uri' => $this->generateUrl('register_gitlab_check', [], UrlGeneratorInterface::ABSOLUTE_URL)]);
}

/**
Expand Down
3 changes: 3 additions & 0 deletions src/Entity/User/OAuthToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ public function accessToken(UserOAuthTokenRefresher $tokenRefresher): string
try {
$newToken = $tokenRefresher->refresh($this->type, $this->refreshToken);
$this->accessToken = $newToken->token();
if ($newToken->getRefreshToken() !== null) {
$this->refreshToken = $newToken->getRefreshToken();
}
$this->expiresAt = $newToken->expiresAt();
} catch (\Throwable $exception) {
throw new \RuntimeException('An error occurred while refreshing the access token: '.$exception->getMessage());
Expand Down
1 change: 1 addition & 0 deletions src/Service/User/UserOAuthTokenRefresher.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ public function refresh(string $type, string $refreshToken): AccessToken

return new AccessToken(
$accessToken->getToken(),
$accessToken->getRefreshToken(),
$accessToken->getExpires() !== null ? (new \DateTimeImmutable())->setTimestamp($accessToken->getExpires()) : null
);
}
Expand Down
10 changes: 9 additions & 1 deletion src/Service/User/UserOAuthTokenRefresher/AccessToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@ class AccessToken
{
private string $token;

private ?string $refreshToken;

private ?\DateTimeImmutable $expiresAt;

public function __construct(string $token, ?\DateTimeImmutable $expiresAt = null)
public function __construct(string $token, ?string $refreshToken = null, ?\DateTimeImmutable $expiresAt = null)
{
$this->token = $token;
$this->refreshToken = $refreshToken;
$this->expiresAt = $expiresAt;
}

Expand All @@ -21,6 +24,11 @@ public function token(): string
return $this->token;
}

public function getRefreshToken(): ?string
{
return $this->refreshToken;
}

public function expiresAt(): ?\DateTimeImmutable
{
return $this->expiresAt;
Expand Down
21 changes: 21 additions & 0 deletions tests/Unit/Entity/User/OAuthTokenTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,27 @@ public function testExpiredAccessToken(string $modifyTime): void
self::assertEquals('new-token', $token->accessToken($this->refresher));
}

public function testAccessTokenDoesUpdateRefreshToken(): void
{
$nowMinusOneDay = (new \DateTimeImmutable())->modify('-1 day');
$token = OAuthTokenMother::withExpireTime($nowMinusOneDay);
$this->refresher->method('refresh')->withConsecutive(
['github', 'refresh'],
['github', 'new-refresh-token1'],
['github', 'new-refresh-token1']
)->willReturnOnConsecutiveCalls(
// On second call, "new-refresh-token1" should be used to refresh the token
new AccessToken('new-token1', 'new-refresh-token1', $nowMinusOneDay),
// Do not update the refresh token if its not provided by the oauth refresh endpoint
new AccessToken('new-token2', null, $nowMinusOneDay),
new AccessToken('new-token3')
);

self::assertEquals('new-token1', $token->accessToken($this->refresher));
self::assertEquals('new-token2', $token->accessToken($this->refresher));
self::assertEquals('new-token3', $token->accessToken($this->refresher));
}

public function testAccessTokenWithFutureExpirationDate(): void
{
$token = OAuthTokenMother::withExpireTime((new \DateTimeImmutable())->modify('61 sec'));
Expand Down
10 changes: 6 additions & 4 deletions tests/Unit/Service/User/UserOAuthTokenRefresherTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,15 @@ public function testRefreshToken(): void
$oauth->method('getClient')->willReturn($client);

$provider->method('getAccessToken')->willReturnOnConsecutiveCalls(
new LeagueAccessToken(['access_token' => 'new-token']),
new LeagueAccessToken(['access_token' => 'new-token', 'expires_in' => 3600])
new LeagueAccessToken(['access_token' => 'new-token-1', 'refresh_token' => 'new-refresh-token-1']),
new LeagueAccessToken(['access_token' => 'new-token-2']),
new LeagueAccessToken(['access_token' => 'new-token-3', 'refresh_token' => 'new-refresh-token-3', 'expires_in' => 3600])
);

$refresher = new UserOAuthTokenRefresher($oauth);

self::assertEquals(new AccessToken('new-token'), $refresher->refresh('github', 'refresh-token'));
self::assertEquals(new AccessToken('new-token', (new \DateTimeImmutable())->setTimestamp(time() + 3600)), $refresher->refresh('github', 'refresh-token'));
self::assertEquals(new AccessToken('new-token-1', 'new-refresh-token-1'), $refresher->refresh('github', 'refresh-token'));
self::assertEquals(new AccessToken('new-token-2'), $refresher->refresh('github', 'refresh-token'));
self::assertEquals(new AccessToken('new-token-3', 'new-refresh-token-3', (new \DateTimeImmutable())->setTimestamp(time() + 3600)), $refresher->refresh('github', 'refresh-token'));
}
}