Skip to content

Conversation

@rullzer
Copy link
Member

@rullzer rullzer commented Jun 5, 2018

If the user is authenticated to cloudA. And then visits a password
protected public link where we authenticate. We should rotate the token
to the new session id. Else they have to reauthenticate

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

@rullzer rullzer added enhancement 2. developing Work in progress labels Jun 5, 2018
@rullzer rullzer added this to the Nextcloud 14 milestone Jun 5, 2018
@rullzer rullzer mentioned this pull request Jun 5, 2018
7 tasks
If the user is authenticated to cloudA. And then visits a password
protected public link where we authenticate. We should rotate the token
to the new session id. Else they have to reauthenticate

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the bugfix/noid/do_not_logout_on_public_share_auth branch from c81f991 to 8da4fe3 Compare June 5, 2018 19:48
@codecov
Copy link

codecov bot commented Jun 5, 2018

Codecov Report

Merging #9756 into master will decrease coverage by 21.76%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #9756       +/-   ##
=============================================
- Coverage     51.72%   29.96%   -21.77%     
+ Complexity    25753    24452     -1301     
=============================================
  Files          1638     1478      -160     
  Lines         96052    79956    -16096     
  Branches       1385        0     -1385     
=============================================
- Hits          49682    23955    -25727     
- Misses        46370    56001     +9631
Impacted Files Coverage Δ Complexity Δ
apps/files_sharing/lib/AppInfo/Application.php 0% <0%> (-48.92%) 15 <0> (ø)
...s/files_sharing/lib/Controller/ShareController.php 0% <0%> (-47.56%) 65 <0> (+1)
...ib/private/Files/Config/UserMountCacheListener.php 0% <0%> (-100%) 2% <0%> (ø)
apps/comments/lib/AppInfo/Application.php 0% <0%> (-100%) 1% <0%> (ø)
...yBuilder/FunctionBuilder/SqliteFunctionBuilder.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/Comments/ManagerFactory.php 0% <0%> (-100%) 2% <0%> (ø)
lib/public/Comments/CommentsEvent.php 0% <0%> (-100%) 3% <0%> (ø)
apps/files_sharing/lib/External/MountProvider.php 0% <0%> (-100%) 4% <0%> (ø)
apps/user_ldap/lib/Mapping/GroupMapping.php 0% <0%> (-100%) 1% <0%> (ø)
lib/private/TagManager.php 0% <0%> (-100%) 4% <0%> (ø)
... and 844 more

@rullzer rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 6, 2018
$this->session->regenerateId();
$newId = $this->session->getId();
try {
$this->tokenProvider->renewSessionToken($oldId, $newId);
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to work with the provider directly here? IMO it should be handled via some kind of manager service. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the quick fix. Once #9518 is in this all abstracted away from the users.

Copy link
Member Author

Choose a reason for hiding this comment

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

But fair enough. Some manager in the public I terface to tackle this makes sense...

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe an extra manager is overkill here. Can't we just add a parameter (or new function) to the ISession?

@rullzer
Copy link
Member Author

rullzer commented Jun 11, 2018

#9823 is probably better.

@rullzer rullzer closed this Jun 11, 2018
@rullzer rullzer deleted the bugfix/noid/do_not_logout_on_public_share_auth branch June 11, 2018 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants