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

Context route auth redirect #6314

Merged
merged 3 commits into from
Jan 26, 2022
Merged

Context route auth redirect #6314

merged 3 commits into from
Jan 26, 2022

Conversation

kulmann
Copy link
Contributor

@kulmann kulmann commented Jan 25, 2022

Description

This PR includes the contextRoute of the next route when checking if authentication is required.

In the (hopefully very near) future this can be expanded to public links with password protection. But for now this simple PR solves the case of bookmarks to editors having a file open from a private context.

Related Issue

How Has This Been Tested?

  • manually... I don't see much room for tests in the current state of the implementation.

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:

@@ -4,3 +4,9 @@
<p v-translate class="oc-invisible">Loading app</p>
</div>
</template>

<script>
export default {
Copy link
Contributor Author

@kulmann kulmann Jan 25, 2022

Choose a reason for hiding this comment

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

Just added the script part to give the component a name and let my IDE give the imports pretty colors. Meaning: not really related to the changes in this PR, semantically.

@@ -1,6 +1,7 @@
import get from 'lodash-es/get.js'
import Vue from 'vue'
import Router from 'vue-router'
// eslint-disable-next-line no-unused-vars
import Router, { Route } from 'vue-router'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Route is not used in code but in a docblock. Apparently eslint doesn't consider docblocks.

@@ -15,13 +15,13 @@ const appInfo = {
const routes = [
{
name: 'apps',
path: '/:file_id/:app?',
path: '/:contextRouteName/:file_id/:app?',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the contextRouteName so that we can check the auth requirements of the context route (and redirect to IDP login page if coming from a private context).

@kulmann kulmann force-pushed the context-route-auth-redirect branch from 135c6c0 to a52a2c2 Compare January 25, 2022 22:34
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

5.6% 5.6% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@fschade fschade left a comment

Choose a reason for hiding this comment

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

👍

@kulmann kulmann merged commit a50d3c8 into master Jan 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the context-route-auth-redirect branch January 26, 2022 12:21
@@ -241,6 +241,7 @@ export default {
const routeData = this.$router.resolve({
name: 'external-apps',
params: {
contextRouteName: this.$route.name,
Copy link
Member

Choose a reason for hiding this comment

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

I know I'm late but this would have been a chance to introduce a common helper for params to external and the other apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you like to do that in a separate PR? 😇 and/or create an issue? 😊

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

Successfully merging this pull request may close these issues.

New requests to external apps are unauthenticated Open a document in a external app via bookmark fails
4 participants