Fix editing an existing OAuth device

[lehmanju]

When editing an existing oauth device, the UI says it is unauthorized even though the authorization is active. The reason is the wrong use of masked values, which are not merged with the original config. As a consequence evcc thinks it is a new device config.

This has been created with Claude, but the gist is:

• if this is an edit, id is not undefined
• forward id to the backend
• query storage for correct config for this id
• check if it is authorized

I wouldn't merge this as is as I am sure this should be further optimized but it serves as basis for solving the issue.

[sourcery-ai]

Hey - I've left some high level feedback:

• The storedDeviceOther function duplicates the same ByName(...).Config().Other pattern for each class; consider extracting a common helper or interface to reduce repetition and make it easier to extend with new device classes.
• When building the config/auth URL in checkAuth, wrap deviceType and id with encodeURIComponent to avoid issues if these values ever contain characters that need URL encoding.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `storedDeviceOther` function duplicates the same `ByName(...).Config().Other` pattern for each class; consider extracting a common helper or interface to reduce repetition and make it easier to extend with new device classes.
- When building the `config/auth` URL in `checkAuth`, wrap `deviceType` and `id` with `encodeURIComponent` to avoid issues if these values ever contain characters that need URL encoding.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[andig]

@naltatis is this an issue with masked vs private? Afair we did have merging implemented?

[naltatis]

is this an issue with masked vs private?

private is only used for exposing configuration for public use. So should not play a role here.

Afair we did have merging implemented?

Yes, we already have this logic and should reuse it. See mergeMasked and testmerged. We use this, when testing/validating an existing device via UI.

[sourcery-ai]

Hey - I've left some high level feedback:

• The new auth/{class}/merge/{id} route uses the regex [0-9.]+ for id but authHandler parses it with strconv.Atoi, so requests containing a dot will match the route but fail parsing; consider changing the pattern to [0-9]+ to align with the integer parsing.
• In checkAuth on the frontend, deviceType is interpolated into the URL segment used as {class:[a-z]+}; double‑check that all possible DeviceType values are lowercase and match the backend templates.Class strings, otherwise the merge path may not be hit for some device types.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new `auth/{class}/merge/{id}` route uses the regex `[0-9.]+` for `id` but `authHandler` parses it with `strconv.Atoi`, so requests containing a dot will match the route but fail parsing; consider changing the pattern to `[0-9]+` to align with the integer parsing.
- In `checkAuth` on the frontend, `deviceType` is interpolated into the URL segment used as `{class:[a-z]+}`; double‑check that all possible `DeviceType` values are lowercase and match the backend `templates.Class` strings, otherwise the merge path may not be hit for some device types.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[naltatis]

Works. Added e2e to guard against regressions.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• The new auth route pattern uses {id:[0-9.]+} but authHandler parses id with strconv.Atoi, so IDs containing dots will match the route but fail at runtime; consider tightening the regex to [0-9]+ to align with the parsing logic.
• The change to plugin/auth/demo.go makes the demo auth flow depend on a hard-coded secret == "topsecret", which alters behavior outside the tests; consider scoping this check to tests (e.g., via a build tag, env flag, or test-only helper) so existing demo usage is not unintentionally broken.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new auth route pattern uses `{id:[0-9.]+}` but `authHandler` parses `id` with `strconv.Atoi`, so IDs containing dots will match the route but fail at runtime; consider tightening the regex to `[0-9]+` to align with the parsing logic.
- The change to `plugin/auth/demo.go` makes the demo auth flow depend on a hard-coded `secret == "topsecret"`, which alters behavior outside the tests; consider scoping this check to tests (e.g., via a build tag, env flag, or test-only helper) so existing demo usage is not unintentionally broken.

## Individual Comments

### Comment 1
<location path="server/http_config_metadata_handler.go" line_range="46-48" />
<code_context>
 		return
 	}

+	// when editing existing device, merge masked values with stored config
+	if vars := mux.Vars(r); vars["class"] != "" && vars["id"] != "" {
+		id, err := strconv.Atoi(vars["id"])
+		if err != nil {
+			jsonError(w, http.StatusBadRequest, err)
</code_context>
<issue_to_address>
**issue (bug_risk):** Route regex allows non-integer IDs but handler enforces strict int parsing

The route pattern `/auth/{class:[a-z]+}/merge/{id:[0-9.]+}` permits values like `1.0`, but `id` is parsed with `strconv.Atoi`, so those requests will fail with a 400 instead of a 404. Align the route with the actual expectations (e.g. change to `[0-9]+`) or update parsing if non-integer IDs are desired.
</issue_to_address>

### Comment 2
<location path="server/http_config_helper.go" line_range="185-194" />
<code_context>
+func mergedMaskedConfig(class templates.Class, id int, conf map[string]any) (map[string]any, error) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handling nil `conf` before cloning and indexing would avoid potential panics

If `conf` is `nil`, `maps.Clone(conf)` will produce a `nil` map and the subsequent `mergeReq[typeTemplate]` access will panic. Normalizing `conf` up front (e.g. `if conf == nil { conf = map[string]any{} }`) would ensure `mergeReq` is never `nil`.

Suggested implementation:

```golang
	// mergedMaskedConfig merges a new configuration with an existing one, replacing masked values with their original counterparts
func mergedMaskedConfig(class templates.Class, id int, conf map[string]any) (map[string]any, error) {
	if conf == nil {
		conf = map[string]any{}
	}

	var (
		old map[string]any
		err error
	)

```

If `mergedMaskedConfig` is ever called with a `nil` map and the caller relies on that to detect "no input config", this behavior change should be verified. Otherwise, no additional changes should be needed: after this normalization, `maps.Clone(conf)` will safely return a non-nil map and `mergeReq[typeTemplate]` indexing will no longer panic due to a nil map.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[naltatis]

@sourcery-ai dismiss

[andig]

Thank you @lehmanju great PR!