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.

Sign up for GitHub

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

Jump to bottom

[OP#49621] fix app password form becomes inactive #456

Merged
merged 5 commits into from
Aug 16, 2023
Merged

[OP#49621] fix app password form becomes inactive #456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
49e185b
Check and remove the app password with name OpenProject
SwikritiT Aug 10, 2023
5137f8f
add phpunit test
SwikritiT Aug 14, 2023
dea267c
add api test to cover the bug
SwikritiT Aug 14, 2023
34bb4e5
add unit test for multiple token
SwikritiT Aug 15, 2023
7c44315
add unit test for delete token
SwikritiT Aug 15, 2023
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
16 changes: 13 additions & 3 deletions lib/Service/OpenProjectAPIService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,8 +1129,12 @@ public function generateAppPasswordTokenForUser(): string {
*/
public function deleteAppPassword(): void {
if ($this->hasAppPassword()) {
$tokenId = $this->tokenProvider->getTokenByUser(Application::OPEN_PROJECT_ENTITIES_NAME)[0]->getId();
$this->tokenProvider->invalidateTokenById(Application::OPEN_PROJECT_ENTITIES_NAME, $tokenId);
$tokens = $this->tokenProvider->getTokenByUser(Application::OPEN_PROJECT_ENTITIES_NAME);
foreach ($tokens as $token) {
if ($token->getName() === Application::OPEN_PROJECT_ENTITIES_NAME) {
$this->tokenProvider->invalidateTokenById(Application::OPEN_PROJECT_ENTITIES_NAME, $token->getId());
}
}
}
}

Expand All @@ -1140,7 +1144,13 @@ public function deleteAppPassword(): void {
* @return bool
*/
public function hasAppPassword(): bool {
return sizeof($this->tokenProvider->getTokenByUser(Application::OPEN_PROJECT_ENTITIES_NAME)) === 1;
$tokens = $this->tokenProvider->getTokenByUser(Application::OPEN_PROJECT_ENTITIES_NAME);
foreach ($tokens as $token) {
if ($token->getName() === Application::OPEN_PROJECT_ENTITIES_NAME) {
return true;
}
}
return false;
}

/**
Expand Down
10 changes: 6 additions & 4 deletions tests/acceptance/features/api/setup.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,10 +538,9 @@ Feature: setup the integration through an API
And groupfolder "OpenProject" should be managed by the user "OpenProject"
# the next step is only for the tests, because that user has a random password
Given the administrator has changed the password of "OpenProject" to the default testing password
Then user "OpenProject" should have a folder called "OpenProject"

And user "OpenProject" should have a folder called "OpenProject"
# folders inside the OpenProject folder can only be deleted/renamed by the OpenProject user
Given user "Carol" has been created
And user "Carol" has been created
And user "Carol" has been added to the group "OpenProject"
And user "OpenProject" has created folder "/OpenProject/project-abc"
Then user "Carol" should have a folder called "OpenProject/project-abc"
Expand Down Expand Up @@ -635,9 +634,12 @@ Feature: setup the integration through an API
When user "OpenProject" sends a "PROPFIND" request to "/remote.php/webdav" using current app password
Then the HTTP status code should be "207"

# this is to provide test coverage for issues like this
# https://community.openproject.org/projects/nextcloud-integration/work_packages/49621
When a new browser session for "Openproject" starts
# but other values can be updated by sending a PATCH request
# also we can replace old app password by sending PATCH request to get new user app password
When the administrator sends a PATCH request to the "setup" endpoint with this data:
And the administrator sends a PATCH request to the "setup" endpoint with this data:
"""
{
"values" : {
Expand Down
83 changes: 83 additions & 0 deletions tests/acceptance/features/bootstrap/FeatureContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use GuzzleHttp\Client;
use GuzzleHttp\Exception\RequestException;
use GuzzleHttp\Psr7\Request;
use GuzzleHttp\Cookie\CookieJar;
use PHPUnit\Framework\Assert;
use Psr\Http\Message\ResponseInterface;

Expand Down Expand Up @@ -39,6 +40,9 @@ class FeatureContext implements Context {

private ?ResponseInterface $response = null;

private CookieJar $cookieJar;
private string $requestToken;

public function getAdminUsername(): string {
return $this->adminUsername;
}
Expand All @@ -55,6 +59,20 @@ public function getBaseUrl(): string {
return $this->baseUrl;
}

/**
* @return string
*/
public function getRequestToken():string {
return $this->requestToken;
}

/**
* @return CookieJar
*/
public function getCookieJar():CookieJar {
return $this->cookieJar;
}

/**
* @return ResponseInterface|null
*/
Expand Down Expand Up @@ -83,6 +101,7 @@ public function __construct(
$this->adminUsername = $adminUsername;
$this->adminPassword = $adminPassword;
$this->regularUserPassword = $regularUserPassword;
$this->cookieJar = new CookieJar();
}

/**
Expand Down Expand Up @@ -909,6 +928,70 @@ public function theContentOfFileAtForUserShouldBe(
Assert::assertSame($content, $this->response->getBody()->getContents());
}

/**
* @When a new browser session for :user starts
*
* @param string $user
*
* @return void
*/
public function aNewBrowserSessionForUserStarts(string $user):void {
$loginUrl = $this->getBaseUrl() . '/index.php/login';
$options['cookies'] = $this->getCookieJar();
// Request a new session and extract CSRF token
$this->setResponse(
$this->sendHttpRequest(
$loginUrl,
null,
null,
'GET',
null,
null,
$options
)
);
$this->theHttpStatusCodeShouldBe(200);
$this->extractRequestTokenFromResponse($this->getResponse());

// Login and extract new token
$body = [
'user' => $user,
'password' => $this->getRegularUserPassword(),
'requesttoken' => $this->getRequestToken()
];
$options['cookies'] = $this->getCookieJar();
$this->setResponse(
$this->sendHttpRequest(
$loginUrl,
null,
null,
'POST',
null,
$body,
$options
)
);
$this->theHttpStatusCodeShouldBe(200);
$this->extractRequestTokenFromResponse($this->getResponse());
}

/**
* @param ResponseInterface $response
*
* @return void
*/
public function extractRequestTokenFromResponse(ResponseInterface $response):void {
$this->requestToken = \substr(
\preg_replace(
'/(.*)data-requesttoken="(.*)">(.*)/sm',
'\2',
$response->getBody()->getContents()
),
0,
89
);
}

/**
* @param string|null $username
* @param string|null $password
Expand Down
153 changes: 151 additions & 2 deletions tests/lib/Service/OpenProjectAPIServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,7 @@ private function getOpenProjectAPIService(
* @param ISubAdmin|null $subAdminManagerMock
* @param ISecureRandom|null $iSecureRandomMock
* @param IConfig|null $configMock
* @param IProvider|null $tokenProviderMock
* @return OpenProjectAPIService|MockObject
*/
private function getServiceMock(
Expand All @@ -411,7 +412,8 @@ private function getServiceMock(
$appManagerMock = null,
$subAdminManagerMock = null,
$iSecureRandomMock = null,
$configMock = null
$configMock = null,
$tokenProviderMock = null
): OpenProjectAPIService {
$onlyMethods[] = 'getBaseUrl';
if ($rootMock === null) {
Expand All @@ -438,6 +440,9 @@ private function getServiceMock(
if ($configMock === null) {
$configMock = $this->createMock(IConfig::class);
}
if ($tokenProviderMock === null) {
$tokenProviderMock = $this->createMock(IProvider::class);
}
$mock = $this->getMockBuilder(OpenProjectAPIService::class)
->setConstructorArgs(
[
Expand All @@ -454,7 +459,7 @@ private function getServiceMock(
$groupManagerMock,
$appManagerMock,
$this->createMock(IDBConnection::class),
$this->createMock(IProvider::class),
$tokenProviderMock,
$iSecureRandomMock,
$this->createMock(IEventDispatcher::class),
$subAdminManagerMock,
Expand Down Expand Up @@ -1739,6 +1744,150 @@ public function testIsSystemReadyForGroupFolderSetUpUserOrGroupExistsException(
$service->isSystemReadyForProjectFolderSetUp();
}

public function testProjectFolderHasAppPassword(): void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets add also a test where multiple tokens are returned but only one is an OpenProject token.
and what happens if multiple tokens have the name OpenProject?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there are multiple OpenProject tokens then hasAppPassword will return true but while deleting it will delete all the app passwords before creating a new one. Is that okay or do we want to check if there's only one token named OpenProject and if there are multiple then send an error message?

$tokenProviderMock = $this->getMockBuilder(IProvider::class)->disableOriginalConstructor()
->getMock();
$tokenMock = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock
->method('getName')
->willReturn('OpenProject');
$tokenProviderMock
->method('getTokenByUser')
->with(Application::OPEN_PROJECT_ENTITIES_NAME)
->willReturn([$tokenMock]);
$service = $this->getServiceMock([],
null,
null,
null,
null,
null,
null,
null,
null,
$tokenProviderMock);
$this->assertTrue($service->hasAppPassword());
}

public function testProjectFolderHasMultipleAppPassword(): void {
$tokenProviderMock = $this->getMockBuilder(IProvider::class)->disableOriginalConstructor()
->getMock();
$tokenMock1 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock1
->method('getName')
->willReturn('session');
$tokenMock2 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock2
->method('getName')
->willReturn('test');
$tokenMock3 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock3
->method('getName')
->willReturn('new-token');
$tokenMock4 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock4
->method('getName')
->willReturn('OpenProject');
$tokenProviderMock
->method('getTokenByUser')
->with(Application::OPEN_PROJECT_ENTITIES_NAME)
->willReturn([$tokenMock1,$tokenMock2,$tokenMock3,$tokenMock4]);
$service = $this->getServiceMock([],
null,
null,
null,
null,
null,
null,
null,
null,
$tokenProviderMock);
$this->assertTrue($service->hasAppPassword());
}

public function testProjectFolderHasAppPasswordNegativeCondition(): void {
$tokenProviderMock = $this->getMockBuilder(IProvider::class)->disableOriginalConstructor()
->getMock();
$tokenMock = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock
->method('getName')
->willReturn('session');
$tokenProviderMock
->method('getTokenByUser')
->with(Application::OPEN_PROJECT_ENTITIES_NAME)
->willReturn([$tokenMock]);
$service = $this->getServiceMock([],
null,
null,
null,
null,
null,
null,
null,
null,
$tokenProviderMock);
$this->assertFalse($service->hasAppPassword());
}

public function testProjectFolderDeleteAppPassword(): void {
$tokenProviderMock = $this->getMockBuilder(IProvider::class)->disableOriginalConstructor()
->getMock();
$tokenMock1 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock1
->method('getName')
->willReturn('session');
$tokenMock1
->method('getId')
->willReturn(1);
$tokenMock2 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock2
->method('getName')
->willReturn('test');
$tokenMock2
->method('getId')
->willReturn(2);
$tokenMock3 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock3
->method('getName')
->willReturn('new-token');
$tokenMock3
->method('getId')
->willReturn(3);
$tokenMock4 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock4
->method('getName')
->willReturn('OpenProject');
$tokenMock4
->method('getId')
->willReturn(4);
$tokenMock5 = $this->getMockBuilder(IToken::class)->getMock();
$tokenMock5
->method('getName')
->willReturn('OpenProject');
$tokenMock5
->method('getId')
->willReturn(5);
$tokenProviderMock
->method('getTokenByUser')
->with(Application::OPEN_PROJECT_ENTITIES_NAME)
->willReturn([$tokenMock1,$tokenMock2,$tokenMock3,$tokenMock4,$tokenMock5]);
$service = $this->getServiceMock(['hasAppPassword'],
null,
null,
null,
null,
null,
null,
null,
null,
$tokenProviderMock)
;
$service->method('hasAppPassword')->willReturn(true);
$tokenProviderMock->expects($this->exactly(2))
->method('invalidateTokenById')
->withConsecutive([Application::OPEN_PROJECT_ENTITIES_NAME, 4], [Application::OPEN_PROJECT_ENTITIES_NAME, 5]);
$service->deleteAppPassword();
}

public function testLinkWorkPackageToFilePact(): void {
$consumerRequest = new ConsumerRequest();
$consumerRequest
Expand Down
Loading