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

Redirect to private link after log in #1900

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

LukasHirt
Copy link
Collaborator

Description

If login page is triggered after trying to access private link, url of private link gets saved to store and via vuex-persist is persisted during the redirection to different page due to login. After login is successful the redirect to private link is triggered. During that redirection the private link gets deleted from store to allow normal log in later.

Motivation and Context

When users don't get redirected to private link after login it is not only a bug but also could lead to potential confusion when they'd think they landed into the folder and could start throwing items in there which where supposed to be somewhere else.

How Has This Been Tested?

  • test environment: Manually
  1. Create a private link for folder
  2. Exit Phoenix
  3. Enter private link
  4. Go through authorisation process
  5. Get redirected to the item
  6. Copy different private link
  7. Enter the private link without exiting Phoenix
  8. Get redirected to the item
  9. Exit Phoenix
  10. Go through authorisation process without private link
  11. See normal files list

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@LukasHirt LukasHirt added the Status:Needs-Review Needs review from a maintainer label Sep 5, 2019
@LukasHirt LukasHirt requested a review from PVince81 September 5, 2019 13:03
@LukasHirt LukasHirt self-assigned this Sep 5, 2019
@@ -29,7 +30,8 @@ export const Store = new Vuex.Store({
app,
apps,
user,
config
config,
links
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be it's own module due to vuex-persist. Since we are using modules in options we can't use reducer to persist only specific states. This would mean the only option of adding it to existing module would be user module which doesn't seem right to me to contain info about private link.

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUITrashbin failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/4976/

20190905-131520-311.png
20190905-131557-497.png

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

See comments.

I'm a bit worried about the potential race conditions when multiple browser tabs are opened with different private links, while here we only store a single link. I'm not sure if this is a big concern though, need to evaluate the risk.

Would also be good to know what @DeepDiver1975 thinks here

src/store/links.js Outdated Show resolved Hide resolved
apps/files/src/components/FilesApp.vue Outdated Show resolved Hide resolved
@PVince81
Copy link
Contributor

PVince81 commented Sep 6, 2019

also see #1882

not sure if the persistence layer works in incognito mode ?

@LukasHirt
Copy link
Collaborator Author

not sure if the persistence layer works in incognito mode ?

Just tried this - works fine

@LukasHirt LukasHirt force-pushed the bugfix/private-link-redirection-after-login branch from 128f9bd to e422aa6 Compare September 6, 2019 12:06
@ownclouders
Copy link
Contributor

💥 Acceptance tests webUITrashbin failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/4999/

20190906-121622-217.png
20190906-121622-503.png
20190906-121705-129.png
20190906-121705-578.png
20190906-121739-733.png
20190906-121739-978.png
20190906-121814-710.png
20190906-121814-957.png
20190906-121849-262.png
20190906-121924-381.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/4999/

20190906-121935-337.png
20190906-121935-460.png
20190906-121956-683.png
20190906-121956-851.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/4999/

20190906-122349-792.png
20190906-122349-927.png
20190906-122412-947.png
20190906-122413-032.png
20190906-122449-120.png
20190906-122449-202.png
20190906-122449-285.png
20190906-122449-374.png
20190906-122449-447.png
20190906-122449-543.png
20190906-122449-614.png
20190906-122449-702.png
20190906-122449-793.png
20190906-122449-868.png
20190906-122450-009.png

@LukasHirt
Copy link
Collaborator Author

LukasHirt commented Sep 6, 2019

I'm a bit worried about the potential race conditions when multiple browser tabs are opened with different private links, while here we only store a single link. I'm not sure if this is a big concern though, need to evaluate the risk.

Maybe session storage could help here - https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage Will do some testing with it.

This would mean then introducing separate vuex-persist logic for the links module though.

@PVince81
Copy link
Contributor

PVince81 commented Sep 9, 2019

Are there any other alternatives where we could pass the old link around ?
I guess this would mean that we need to support passing the redirect URL through the whole authentication/authorize process ?

@LukasHirt
Copy link
Collaborator Author

Are there any other alternatives where we could pass the old link around ?
I guess this would mean that we need to support passing the redirect URL through the whole authentication/authorize process ?

I think there aren't. Since we leave Phoenix completely due to the authorisation we need to store it in the storage. Other scenarios would lead to losing the link.

Added Url to name and default value to null
@LukasHirt LukasHirt force-pushed the bugfix/private-link-redirection-after-login branch from e422aa6 to 63e79f7 Compare September 13, 2019 04:25
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 3deea60 into master Sep 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the bugfix/private-link-redirection-after-login branch September 13, 2019 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants