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

fix: context fallback to user instead of undefined #7437

Merged
merged 4 commits into from
Aug 18, 2022

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Aug 11, 2022

Description

We've fixed a bug where routes without explicit auth requirement (i.e. user context) and without any context route in the URL were recognized as neither user-context nor public-link-context. In such situations we now expect that the session requires a user and redirect to the login page (instead of staying stuck on a most likely broken page).

Motivation / context

We want to provide an endpoint in ocis that let's e.g. the desktop client query a URL for opening a document (via app provider / external app) in web. The desktop client has the file id and the app provider is capable of determining a default app if no app name is provided. But neither the desktop client nor the ocis backend know what route the file might have come from if opened via files app. Opening a file in web from the desktop client is always supposed to open in an authenticated session / user session (user needs to log in if not already done before).

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:

@dschmidt
Copy link
Member

dschmidt commented Aug 12, 2022

Interesting unit test failure in FileDetails.spec.js:

Screenshot_20220812_091101

That happens because

    showShares() {
      return this.hasAnyShares && isUserContext(this.$router, this.$route)
    },

now returns true in this test. I think it's fine to just update the snapshots...

... but opening files like this without contextRoute breaks the AppTopBar! We need to adjust the TopBar to not show/disable the close button (and explain that) or make it work somehow for example by defaulting the close to navigating to /.
I would ask you to take care of that in this PR so we don't forget about it, as it shouldn't be a huge change anyway.

@kulmann kulmann self-assigned this Aug 12, 2022
@kulmann kulmann force-pushed the default-to-user-context branch from 8a4ddff to 17da585 Compare August 15, 2022 19:18
@kulmann
Copy link
Member Author

kulmann commented Aug 15, 2022

isUserContext

I've changed the implementation to not use the isUserContext helper anymore (that's meant for internal auth handling only) but instead use the usePublicLinkContext composable.

Regarding the close action I've added a default of redirecting to / if the current file context has no context route name set. Good finding!

@kulmann
Copy link
Member Author

kulmann commented Aug 15, 2022

Some issues remain with shares loading in the file details panel. At least it looks weird to me. Will have a look tomorrow morning.

@kulmann kulmann force-pushed the default-to-user-context branch from df5fa23 to 0d079ed Compare August 16, 2022 14:21
@dschmidt
Copy link
Member

isUserContext

I've changed the implementation to not use the isUserContext helper anymore (that's meant for internal auth handling only) but instead use the usePublicLinkContext composable.

👍🏻

Regarding the close action I've added a default of redirecting to / if the current file context has no context route name set. Good finding!

Hehe, yeah, I care about apps 🤓 Good fix!

if (this.file.type === 'file' && isUserContext(this.$router, this.$route)) {
calls.push(this.loadVersions({ client: this.$client, fileId: this.file.id }))
try {
await Promise.allSettled([
Copy link
Member

Choose a reason for hiding this comment

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

Why allSettled? Can the catch block be entered at all then?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm true, probably not...

Copy link
Member Author

Choose a reason for hiding this comment

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

Running into a major scope creep... reverting that change and following up in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

@kulmann kulmann force-pushed the default-to-user-context branch from 0d079ed to 2f9507f Compare August 17, 2022 11:10
If we don't return within the detected context other contexts would get
resolve attempts as well.
@sonarcloud
Copy link

sonarcloud bot commented Aug 18, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

9.1% 9.1% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit e23dfa1 into master Aug 18, 2022
@delete-merged-branch delete-merged-branch bot deleted the default-to-user-context branch August 18, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants