-
Notifications
You must be signed in to change notification settings - Fork 8
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
Create Site Preview JWT in the API #2554
Changes from all commits
11eb1ed
49f0995
dea0a1d
791b769
5d8c99a
df45f4d
ced0d11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
--- | ||
"@comet/cms-admin": minor | ||
"@comet/cms-site": minor | ||
"@comet/cms-api": minor | ||
--- | ||
|
||
Create site preview JWT in the API | ||
|
||
With this change the site preview can be deployed unprotected. Authentication is made via a JWT created in the API and validated in the site. A separate domain for the site preview is still necessary. | ||
|
||
BREAKING: this update of Comet v7 requires to have set sitePreviewSecret (which has to be the same value like possibly already set for site). Please refer to https://github.com/vivid-planet/comet-starter/pull/371 for more information on how to upgrade. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -750,6 +750,7 @@ type Query { | |
pageTreeNodeList(scope: PageTreeNodeScopeInput!, category: String): [PageTreeNode!]! | ||
paginatedPageTreeNodes(scope: PageTreeNodeScopeInput!, category: String, sort: [PageTreeNodeSort!], documentType: String, offset: Int! = 0, limit: Int! = 25): PaginatedPageTreeNodes! | ||
pageTreeNodeSlugAvailable(scope: PageTreeNodeScopeInput!, parentId: ID, slug: String!): SlugAvailability! | ||
sitePreviewJwt(scope: JSONObject!, path: String!, includeInvisible: Boolean!): String! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imho this should be a mutation, not a query:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so too There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could call it createSitePreviewJwt or generateSitePreviewJwt There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the request is polled (due to expiration time of jwt), this is not possible for mutations. Furthermore, useMutation does not provide refetch, which is used when changing the preview settings. Caching however is disabled by fetchPolicy: "network-only". Changing to a mutation requires more handwork to simulate polling and refetching. Do you still think a mutation is necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was thinking about a future cache solution we might add You ave good arguments, leave the query! |
||
redirects(scope: RedirectScopeInput!, query: String, type: RedirectGenerationType, active: Boolean, sortColumnName: String, sortDirection: SortDirection! = ASC): [Redirect!]! @deprecated(reason: "Use paginatedRedirects instead. Will be removed in the next version.") | ||
paginatedRedirects(scope: RedirectScopeInput!, search: String, filter: RedirectFilter, sort: [RedirectSort!], offset: Int! = 0, limit: Int! = 25): PaginatedRedirects! | ||
redirect(id: ID!): Redirect! | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import { gql, useQuery } from "@apollo/client"; | ||
import { CometColor, Domain, DomainLocked } from "@comet/admin-icons"; | ||
import { Grid, Tooltip, Typography } from "@mui/material"; | ||
import { ReactNode, useCallback, useState } from "react"; | ||
|
@@ -14,6 +15,7 @@ import { VisibilityToggle } from "../common/VisibilityToggle"; | |
import { SitePrevewIFrameLocationMessage, SitePreviewIFrameMessageType } from "./iframebridge/SitePreviewIFrameMessage"; | ||
import { useSitePreviewIFrameBridge } from "./iframebridge/useSitePreviewIFrameBridge"; | ||
import { OpenLinkDialog } from "./OpenLinkDialog"; | ||
import { GQLSitePreviewJwtQuery } from "./SitePreview.generated"; | ||
import { ActionsContainer, LogoWrapper, Root, SiteInformation, SiteLink, SiteLinkWrapper } from "./SitePreview.sc"; | ||
|
||
//TODO v4 remove RouteComponentProps | ||
|
@@ -98,6 +100,7 @@ function SitePreview({ resolvePath, logo = <CometColor sx={{ fontSize: 32 }} /> | |
const newShowOnlyVisible = !showOnlyVisible; | ||
setShowOnlyVisible(String(newShowOnlyVisible)); | ||
setIframePath(sitePath); //reload iframe with new settings | ||
refetch(); | ||
}; | ||
|
||
const siteLink = `${siteConfig.url}${sitePath}`; | ||
|
@@ -113,13 +116,26 @@ function SitePreview({ resolvePath, logo = <CometColor sx={{ fontSize: 32 }} /> | |
} | ||
}); | ||
|
||
const initialPageUrl = `${siteConfig.sitePreviewApiUrl}?${new URLSearchParams({ | ||
scope: JSON.stringify(scope), | ||
path: iframePath, | ||
settings: JSON.stringify({ | ||
includeInvisible: showOnlyVisible ? false : true, | ||
}), | ||
}).toString()}`; | ||
const { data, error, refetch } = useQuery<GQLSitePreviewJwtQuery>( | ||
gql` | ||
query SitePreviewJwt($scope: JSONObject!, $path: String!, $includeInvisible: Boolean!) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One downside to a mutation would be that it can't be queried like this here. But we could create it before even navigating to the site preview. |
||
sitePreviewJwt(scope: $scope, path: $path, includeInvisible: $includeInvisible) | ||
} | ||
`, | ||
{ | ||
fetchPolicy: "network-only", | ||
variables: { | ||
scope, | ||
path: iframePath, | ||
includeInvisible: showOnlyVisible ? false : true, | ||
}, | ||
pollInterval: 1000 * 60 * 60 * 24, // due to expiration time of jwt | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this cause a location reset when a user stays too long in the site preview? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it does. Do we ignore it? (as this only happens after 24 hours) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it's 24 hours, do we even need polling? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The expiry date of the JWT (which is stored in the site as cookie) is validated with every request. If we don't reload the SitePreview every click would result in an error. The problem occurs when a user leaves the tab opened for more than 24 hours and then continues navigation in the preview. |
||
}, | ||
); | ||
if (error) throw new Error(error.message); | ||
if (!data) return <div />; | ||
|
||
const initialPageUrl = `${siteConfig.sitePreviewApiUrl}?${new URLSearchParams({ jwt: data.sitePreviewJwt }).toString()}`; | ||
|
||
return ( | ||
<Root> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { Inject } from "@nestjs/common"; | ||
import { Args, Query, Resolver } from "@nestjs/graphql"; | ||
import { GraphQLJSONObject } from "graphql-scalars"; | ||
import { SignJWT } from "jose"; | ||
|
||
import { RequiredPermission } from "../user-permissions/decorators/required-permission.decorator"; | ||
import { ContentScope } from "../user-permissions/interfaces/content-scope.interface"; | ||
import { SITE_PREVIEW_CONFIG } from "./page-tree.constants"; | ||
|
||
export type SitePreviewConfig = { | ||
secret: string; | ||
}; | ||
|
||
@Resolver() | ||
export class SitePreviewResolver { | ||
constructor(@Inject(SITE_PREVIEW_CONFIG) private readonly config: SitePreviewConfig) {} | ||
|
||
@Query(() => String) | ||
@RequiredPermission("pageTree") | ||
async sitePreviewJwt( | ||
@Args("scope", { type: () => GraphQLJSONObject }) scope: ContentScope, | ||
johnnyomair marked this conversation as resolved.
Show resolved
Hide resolved
|
||
@Args("path") path: string, | ||
@Args("includeInvisible") includeInvisible: boolean, | ||
johnnyomair marked this conversation as resolved.
Show resolved
Hide resolved
|
||
): Promise<string> { | ||
return new SignJWT({ | ||
scope, | ||
path, | ||
previewData: { | ||
includeInvisible, | ||
}, | ||
}) | ||
.setProtectedHeader({ alg: "HS256" }) | ||
.setExpirationTime("1 day") | ||
.sign(new TextEncoder().encode(this.config.secret)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change needs a changeset and this change should also be added to the migration guide for projects that are not on v7 yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changeset will be added.
I added another commit to mitigate the effort of handling the breaking change: dea0a1d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changeset: 5d8c99a