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

Preview tokens 7: Main feature #6829

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Dec 2, 2024

🚨 Merge first

Description

Summary of changes

It's finally time for the real thing and main attraction for this PR series. 🎉

This actually:

  • adds that the token is added to all changes preview URLs,
  • validates the token when auto-detecting the changes version from the request and
  • uses the same implementation in $page->renderVersionFromRequest() for the draft detection in $app->resolve()

Reasoning

  • General thought behind this feature: Prevent externals from accessing changes previews unless they received the full preview URL including token
  • Avoid duplicated code by making the draft resolver hook into the same logic as well

Additional context

This is the first PR where it really makes sense to test it by hand as well. All "default" previews should now work. Only custom blueprint preview options that point to other pages don't work yet, those will be added in the last (smaller) PRs 8 and 9.

So for this PR, we can already test the expected behavior for:

  • Site preview URL includes token and renders the homepage correctly
  • Changing or omitting that token makes Kirby render the latest version instead
  • Same for any page previews
  • Draft previews should still work as well (with token the draft gets resolved, without token the draft 404s completely)

Changelog

Enhancements (since alphas)

  • Preview URLs for changes versions now include a _token query param like draft previews. This protects those previews from access by external visitors.

Breaking changes

  • Removed internal $page->isVerified() method in favor of internal $page->renderVersionFromRequest()

Docs

None

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@lukasbestle lukasbestle added type: enhancement ✨ Suggests an enhancement; improves Kirby type: refactoring ♻️ Is about bad code; cleans up code labels Dec 2, 2024
@lukasbestle lukasbestle added this to the 5.0.0-beta.1 milestone Dec 2, 2024
@lukasbestle lukasbestle self-assigned this Dec 2, 2024
Base automatically changed from v5/changes/preview-tokens-6a to v5/develop December 4, 2024 07:37
Copy link
Member

@bastianallgeier bastianallgeier left a comment

Choose a reason for hiding this comment

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

I haven't done any manual tests yet, but I just wanted to leave a comment that the code looks really good to me. I love the idea for the new renderVersionFromRequest method to handle the verification. I was thinking if there could be an even more obvious name for it, but I cannot come up with something which would be more convincing :)

@distantnative
Copy link
Member

Had the same thoughts but also couldn't come up with a great name. Maybe requestedRenderVersion, but it's not really much different.

@bastianallgeier
Copy link
Member

It's an internal method, so we should probably really not waste too much energy on finding a better name when the current one is already perfectly ok.

@bastianallgeier
Copy link
Member

Ok, I just went through the suggested test scenarios and they all work as expected. I would merge this and then fix issues if we still encounter some.

@bastianallgeier bastianallgeier self-requested a review December 4, 2024 12:51
@bastianallgeier bastianallgeier merged commit 4f6ea62 into v5/develop Dec 4, 2024
11 checks passed
@bastianallgeier bastianallgeier deleted the v5/changes/preview-tokens-7 branch December 4, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants