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.

Sign up for GitHub

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

Jump to bottom

Fix: Custom HTML Headers and static assets got lost on caddy adapt #290

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

Fix: Custom HTML Headers and static assets got lost on caddy adapt #290

wants to merge 2 commits into from

Conversation

poettig
Copy link

@poettig poettig commented Oct 23, 2023

When running caddy adapt, the config of an app gets marshaled into JSON. Because custom html header path and static_asset were not part of the config portal.UI, they did not get marshaled, leading to these settings to be lost when using caddy adapt and then running caddy with the resulting JSON instead of the Caddyfile.

This merge request fixes that problem by moving the actual loading of the assets to go-authcrunch and only saving the values into the configuration when read from a Caddyfile.

I needed this because I run https://github.com/mholt/caddy-l4, which does not work with Caddyfiles.

@poettig poettig mentioned this pull request Oct 23, 2023
Merged
@poettig
Copy link
Author

poettig commented Oct 23, 2023

Dependant pull request in go-authcrunch: greenpau/go-authcrunch#49

@poettig
Copy link
Author

poettig commented Oct 23, 2023

(Kind of) minimal Caddyfile for reproduction:

{
	debug
	order authenticate before respond
	security {
		oauth identity provider google {
			realm google
			driver google
			client_id CLIENT_ID
			client_secret CLIENT_SECRET 
			scopes openid email
		}

		authentication portal testportal {
			crypto key sign-verify JWT_TOKEN
			enable identity provider google

			ui {
				logo url /assets/test.png
				static_asset "assets/test.png" "image/png" /tmp/test.png
				custom html header path /tmp/head.html
			}
		}
	}
}

http://localhost:4000 {
	authenticate with testportal
}

@greenpau
Copy link
Owner

greenpau commented Dec 2, 2023

@poettig , please add CLA to assets/cla/consent.yaml

@poettig poettig force-pushed the fix-caddy-adapt-static-assets-and-custom-html-header branch from fbdae02 to b99262a Compare December 3, 2023 00:03
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 3, 2023 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@poettig
Copy link
Author

poettig commented Dec 3, 2023

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Dec 3, 2023
@github-actions
@poettig has signed the CLA in #290
ea44235
@poettig
Copy link
Author

poettig commented Dec 3, 2023

@greenpau I rebased against upstream and signed using the CLA workflow. Should be cleared up now.

@greenpau
Copy link
Owner

greenpau commented Dec 3, 2023

@poettig , please add your consent to assets/cla/consent.yaml.

@greenpau
Copy link
Owner

greenpau commented Dec 3, 2023

@poettig , please rebase to pick up go-authcrunch v1.0.46

@poettig poettig force-pushed the fix-caddy-adapt-static-assets-and-custom-html-header branch from 4f17815 to 7bbff21 Compare December 3, 2023 00:54
@greenpau
Copy link
Owner

greenpau commented Dec 3, 2023

@poettig . please create PR here and describe how this feature works. https://github.com/authp/authp.github.io

@greenpau
Copy link
Owner

greenpau commented Dec 3, 2023

Once I review the docs, I will merge this one.

@poettig
Copy link
Author

poettig commented Dec 3, 2023 •
edited
Loading

@greenpau I don't think there is anything to add to the documentation. I just made Custom HTML Template Header and Static Assets work when running caddy adapt to convert a Caddyfile into a Caddy JSON configuration file. Before this PR, it only works with a Caddyfile or when writing the Caddy JSON config manually, because these two configuration options were lost when running caddy adapt.

I might have understood something wrong about your request, so I'm open for clarifications :)

@greenpau
Copy link
Owner

greenpau commented Jan 4, 2024

@poettig , please help me understand this.

You removed the following. why? (note that I already don't remember what it does 😄)

			for k, v := range ui.PageTemplates {
					headIndex := strings.Index(v, "<meta name=\"description\"")
					if headIndex < 1 {
						continue
					}
					v = v[:headIndex] + string(b) + v[headIndex:]
					ui.PageTemplates[k] = v
				}

@poettig
Copy link
Author

poettig commented Jan 7, 2024

@greenpau As I understood it, this reads the specified file with custom HTML tags for inside the <head> tag and inserts them after the <meta name="description"> tag.

This behaviour was different to the other cases, which only store the config option into the UI struct (which in turn gets marshalled into JSON by caddy if requested). Therefore, I equalized the behaviour by moving the code to https://github.com/greenpau/go-authcrunch/blob/main/pkg/authn/portal.go#L492

The code is not deleted, just moved to somewhere else :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@poettig @greenpau