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

feat: client side renderers options #2219

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

AGrzes
Copy link

@AGrzes AGrzes commented Jul 22, 2020

Addresses #2167

Implementation Plan

  1. Reproduce code fetching renders configuration from client/components/admin/admin-rendering.vue in client/components/editor.vue.
  2. Pass renderers configuration as a property to editor component
  3. In client/components/editor/editor-markdown.vue Wrap md configuration in a function that accepts renderers configuration
  4. Create md as computed property of markdown editor component that will call the previously defined function to initialize markdown engine based on renderers config property
  5. Use md computed property instead of md module variable.
  6. Update md initialization to use renderers configuration

@@ -63,6 +63,8 @@ import { AtomSpinner } from 'epic-spinners'
import { Base64 } from 'js-base64'
import { StatusIndicator } from 'vue-status-indicator'

import renderersQuery from 'gql/admin/rendering/rendering-query-renderers.gql'
Copy link
Author

Choose a reason for hiding this comment

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

I think it is not right to call admin query here but duplicating the same query in editor section also do not feel right. Any suggestions how to proceed?

@AGrzes AGrzes marked this pull request as ready for review July 23, 2020 08:52
@AGrzes
Copy link
Author

AGrzes commented Jul 24, 2020

From my point of wiew this is finished - please let me know if any changes are still required.

@NGPixel NGPixel self-assigned this Aug 10, 2020
@NGPixel
Copy link
Member

NGPixel commented Aug 10, 2020

While this work because you are admin, the renderers query will fail for normal users as the rendering.renderers resolver requires the manage:system permission. Could be changed to write:pages:

https://github.com/Requarks/wiki/blob/95b6a7ad823803b3452169196935426c2a2345f0/server/graph/schemas/rendering.graphql#L21

As for the query itself, inline it directly in editor.vue. External queries are gradually being removed from the project to reduce complexity. e.g.:

import gql from 'graphql-tag'
// ...
query: gql`
{
  rendering {
    ...
  }
}
`

@AGrzes
Copy link
Author

AGrzes commented Aug 11, 2020

@NGPixel - I added commits handling Your suggestions

@AGrzes
Copy link
Author

AGrzes commented Nov 20, 2020

Merget latest changes from dev and did small fix.

@AGrzes
Copy link
Author

AGrzes commented Mar 16, 2021

Merged latest changes from dev

@EugenDueck
Copy link

@NGPixel I too would like to prevent our internal data from going to some external server, so I'm very interested in this PR. What is missing for it to be merged?

@mbedded
Copy link

mbedded commented Nov 1, 2021

@NGPixel I too would like to prevent our internal data from going to some external server, so I'm very interested in this PR. What is missing for it to be merged?

Is there a plan when this PR will be merged to the next version of wikijs? It is a great product and I have the same issue that i want to prevent potential data-leaks to external services i can't control. Do we have to wait until wikijs-vnext is released until this feature is added?

@yumetodo
Copy link

yumetodo commented Nov 1, 2022

any updates?

1 similar comment
@VusalDev
Copy link

VusalDev commented Apr 5, 2024

any updates?

@awschult002
Copy link

this is a significant security issue for many companies, which is preventing widespread adoption. What can we do to get this PR complete?

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.

None yet

8 participants