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

Disable app token creation for impersonated people, ref #15539 #15936

Merged
merged 4 commits into from
Sep 15, 2019
Merged

Disable app token creation for impersonated people, ref #15539 #15936

merged 4 commits into from
Sep 15, 2019

Conversation

GretaD
Copy link
Contributor

@GretaD GretaD commented Jun 12, 2019 •
edited
Loading

ref #15539
I created a new method for getter and setter, so impersonate app would not set and get the variable directly, but use this method.

@GretaD GretaD added bug 2. developing Work in progress labels Jun 12, 2019
@GretaD GretaD force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from 32351cb to d41b561 Compare June 12, 2019 13:56
@GretaD GretaD added the 3. to review Waiting for reviews label Jun 12, 2019
@GretaD GretaD changed the title (WIP) Disable app token creation for impersonated people, ref #15539 Disable app token creation for impersonated people, ref #15539 Jun 12, 2019
@GretaD GretaD removed the 2. developing Work in progress label Jun 17, 2019
@nickvergessen
Copy link
Member

This should also be checked on the actual endpoint that is being called.

@GretaD GretaD force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from 25b4d7b to 2ed59f4 Compare July 18, 2019 11:28
@nickvergessen
Copy link
Member

@GretaD time to fixup and rebase

@GretaD GretaD force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from c94da72 to 1f441b9 Compare August 20, 2019 10:45
@nickvergessen nickvergessen added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 20, 2019
nickvergessen
nickvergessen reviewed Aug 20, 2019
View reviewed changes
lib/public/IUserSession.php
* @return string|null
* @since 17.0.0
*/
public function getImpersonatingUserID(): ?string;
Copy link
Member

Choose a reason for hiding this comment

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

PHP Fatal error: Class Mock_IUserSession_84aca509 contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (OCP\IUserSession::getImpersonatingUserID, OCP\IUserSession::setImpersonatingUserID) in phar:///usr/local/bin/phpunit/phpunit-mock-objects/Generator.php(264) : eval()'d code on line

seems like there are some usages in unit tests which are not working as expected

@blizzz
Copy link
Member

blizzz commented Sep 6, 2019

There are still some errors from testing, e.g.:

1) Test\Settings\Controller\AuthSettingsControllerTest::testCreate
TypeError: Argument 7 passed to OC\Settings\Controller\AuthSettingsController::__construct() must implement interface OCP\IUserSession, instance of Mock_IManager_2651a371 given, called in /drone/src/tests/Settings/Controller/AuthSettingsControllerTest.php on line 81
/drone/src/settings/Controller/AuthSettingsController.php:89
/drone/src/tests/Settings/Controller/AuthSettingsControllerTest.php:81

and the webpack build complains about uncomitted changes.

@GretaD do you want to give it a look?

@GretaD GretaD force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch 2 times, most recently from 47ef208 to ee02bd6 Compare September 11, 2019 13:19
@blizzz
Copy link
Member

blizzz commented Sep 14, 2019

Now I think you have to rebuild the JS part (npm run build) and commit the related changes.

@GretaD
Copy link
Contributor Author

GretaD commented Sep 14, 2019

/compile amend /

@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from a9174c3 to 84beca9 Compare September 14, 2019 13:12
@GretaD GretaD force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from 84beca9 to fdd0cfe Compare September 14, 2019 13:20
@GretaD
Copy link
Contributor Author

GretaD commented Sep 14, 2019

/compile amend /settings

@npmbuildbot-nextcloud npmbuildbot-nextcloud bot force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from fdd0cfe to 1542c8c Compare September 14, 2019 13:22
@rullzer
Copy link
Member

rullzer commented Sep 15, 2019

This would then require also additions to the impersonate app 😉

GretaD and others added 4 commits September 15, 2019 12:04
@GretaD @rullzer
Disable app token creation for impersonated people, ref #15539
0a874c5 
Signed-off-by: Greta Doci <[email protected]>
@nickvergessen @rullzer
Fix wrongly mixed mock objects in encryption tests
16d47b5 
Signed-off-by: Joas Schilling <[email protected]>
@GretaD @rullzer
Fixed the test
4ff7551 
Signed-off-by: Greta Doci <[email protected]>
@GretaD @rullzer
Fixed the SecurityTest error
68ef242 
Signed-off-by: Greta Doci <[email protected]>
Signed-off-by: npmbuildbot[bot] <npmbuildbot[bot]@users.noreply.github.com>
@rullzer rullzer added this to the Nextcloud 18 milestone Sep 15, 2019
@rullzer rullzer force-pushed the bugfix/15539/wronguser-apptoken-impersonation branch from 1542c8c to 68ef242 Compare September 15, 2019 10:04
@rullzer
Copy link
Member

rullzer commented Sep 15, 2019

I updated the phpdoc and rebased. Will emrge after

@rullzer rullzer merged commit 8137616 into master Sep 15, 2019
@rullzer rullzer deleted the bugfix/15539/wronguser-apptoken-impersonation branch September 15, 2019 17:34
rullzer added a commit that referenced this pull request Sep 25, 2019
@rullzer
Fix apptoken creation
59e545b 
Seems there was a small bug in #15936
This inverts the check and add the property

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer rullzer mentioned this pull request Sep 25, 2019
Merged
rullzer added a commit that referenced this pull request Sep 25, 2019
@rullzer
Fix apptoken creation
17a6bb9 
Seems there was a small bug in #15936
This inverts the check and add the property

Signed-off-by: Roeland Jago Douma <[email protected]>
@skjnldsv skjnldsv mentioned this pull request Dec 7, 2019
Closed
@kesselb kesselb mentioned this pull request Dec 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@rullzer rullzer Awaiting requested review from rullzer

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Milestone
Nextcloud 18.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@GretaD @nickvergessen @blizzz @rullzer @ChristophWurst