Skip to content

Conversation

@schiessle
Copy link
Member

make exchange of shared secret more robust and improve error reporting.

@codecov
Copy link

codecov bot commented Apr 20, 2018

Codecov Report

Merging #9256 into master will decrease coverage by 20.43%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master   #9256       +/-   ##
============================================
- Coverage     51.93%   31.5%   -20.44%     
- Complexity    25389   25402       +13     
============================================
  Files          1608    1608               
  Lines         95436   95464       +28     
  Branches       1394    1394               
============================================
- Hits          49564   30074    -19490     
- Misses        45872   65390    +19518
Impacted Files Coverage Δ Complexity Δ
...federation/lib/Controller/OCSAuthAPIController.php 69.09% <ø> (-0.56%) 10 <0> (ø)
...s/federation/lib/BackgroundJob/GetSharedSecret.php 0% <0%> (-69.24%) 35 <0> (+14)
...deration/lib/BackgroundJob/RequestSharedSecret.php 71.25% <100%> (+0.16%) 18 <0> (-1) ⬇️
apps/user_ldap/lib/LDAPProviderFactory.php 0% <0%> (-100%) 2% <0%> (ø)
lib/public/Files/ForbiddenException.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/Files/Mount/LocalHomeMountProvider.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/SystemTag/ManagerFactory.php 0% <0%> (-100%) 3% <0%> (ø)
apps/files_trashbin/lib/Command/Expire.php 0% <0%> (-100%) 3% <0%> (ø)
lib/private/Files/Mount/CacheMountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
...rivate/Authentication/Token/DefaultTokenMapper.php 0% <0%> (-100%) 11% <0%> (ø)
... and 371 more

@MorrisJobke
Copy link
Member

There is another one that resets the token:

$this->dbHandler->addToken($target, '');


$this->trustedServers->addSharedSecret($url, $sharedSecret);
// reset token after the exchange of the shared secret was successful
$this->dbHandler->addToken($url, '');
Copy link
Member

Choose a reason for hiding this comment

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

can you explain why?

Copy link
Member

Choose a reason for hiding this comment

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

Because this is not needed to be reset. The code knows what is the bigger and what is the smaller token. It often confused people why one server deletes it. And also there is code wise no good reason why it should be read. On the other hand this could give insights into debugging sessions so we keep it just as it is.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Makes a lot of sense 👍

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me then

@MorrisJobke MorrisJobke force-pushed the fed-sharing-improvements branch from dd3f0cb to 178244a Compare April 25, 2018 09:59
@MorrisJobke
Copy link
Member

I also fixed the tests 🙈

@MorrisJobke MorrisJobke force-pushed the fed-sharing-improvements branch from 178244a to 5110983 Compare April 25, 2018 10:03
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 25, 2018
@MorrisJobke MorrisJobke force-pushed the fed-sharing-improvements branch from 5110983 to 5dfddce Compare April 25, 2018 11:50
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@MorrisJobke MorrisJobke force-pushed the fed-sharing-improvements branch from 5dfddce to 0e0cfa0 Compare April 25, 2018 11:54
@MorrisJobke
Copy link
Member

I just noticed that the $dbHandler dependency could be dropped, so I removed it from the code completely ;)

@MorrisJobke MorrisJobke merged commit 9c64a20 into master Apr 25, 2018
@MorrisJobke MorrisJobke deleted the fed-sharing-improvements branch April 25, 2018 13:49
@MorrisJobke
Copy link
Member

@schiessle What about the backport? 🏓 😉

@MorrisJobke
Copy link
Member

Backport in #9657

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish feature: federation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants