diff --git a/README.md b/README.md deleted file mode 100644 index b2c98ba..0000000 --- a/README.md +++ /dev/null @@ -1,54 +0,0 @@ -# Notr -Simple Go application for organizing and referencing notes. Loosely based on Obsidian. - -**Implementation:** See `jade-depo/` directory for the CLI tool. - -## Workflow -I take notes in two primary ways: -### Phone -- Quick notes, on the go. -- View and search notes. - -### Workstation -- Using NeoVim for notetaking - -### Other infrastructure -- I host a VPS with a Nextcloud and Gitea instances. - -## What I want -- A Obsidian Vault like structure. A folder where notes live. -- A file is a note -- Can also store attachments, such as images. These files can then be referenced in the relevant notes. -- Directories is the main organization method, although tags and links can seam-lesly cross directories boundries. -- Markdown syntax (this can be handled by NeoVim and a markdown editor on other devices.) -- Tags: Syntax +tag -- Note links for referencing other notes or any other vault files. Syntax uncertain. Obsidian uses [[]]? -- See reports about the vault. Tag report -- At some point I would like to have a web-app and host it on my server. This would integrate with my authentik service for auth, and would be a live view of a users vault -- OCR would be great. - -## Implementation -I have a tendency to scope creep and never actually getting a usable product, so an important goal here is practicing getting a usable app up and running. This should not have to be the biggest project, so I'll try to predict the process: - -### Version 0.1 -Here I use other tools for the note-taking and accept that any searching is on a directory basis only. -- [ ] I create a directory in Nextcloud. This I will start using immediately. -- [ ] Find a good Markdown editor for android. -- [ ] Adopt any crutial Obsidian notes - -### Version 1.0 ✓ -This is where I can use Notr to find and search notes on my workstation. CLI implementation complete! -- [x] Process notes. Metadata and diffs -- [x] Search and Filter by tags -- [x] Search and Filter by content -- [x] Add, edit, delete notes -- [x] List all notes and tags - -### Version 2.0 -Here I can do the same on my phone. - -Also: -- [ ] OCR - -## Metadata approach -Multiple approaches possible. diff --git a/docs/design/config-redesign.md b/docs/design/config-redesign.md new file mode 100644 index 0000000..c1765ae --- /dev/null +++ b/docs/design/config-redesign.md @@ -0,0 +1,482 @@ +# Config System Redesign + +## 1. Problem Statement + +`LoadConfig()` panics on the server because the config system assumes a writable +config directory. On the server, `OPAL_CONFIG_DIR=/etc/opal` is read-only +(systemd `ReadOnlyPaths`), `opal.yml` doesn't exist (Ansible only deploys +`opal.env`), and the attempt to create it fails. The nil `*Config` propagates +unchecked through `GetConfig()` callers to `BuildUrgencyCoefficients(nil)`, +causing a nil-pointer panic. + +### Panic chain + +``` +sortByUrgency() report.go:426 + cfg, _ := GetConfig() ← error discarded, cfg = nil + coeffs := BuildUrgencyCoefficients(cfg) + return &UrgencyCoefficients{ + Due: cfg.UrgencyDue, ← nil dereference → PANIC + } +``` + +### Root cause + +The config system was designed for the CLI (user-writable `~/.config/opal/`) +and has three interacting issues: + +1. **Write-on-read**: `LoadConfig()` creates directories and writes a default + `opal.yml` as a side effect of *reading* config. This fails when the + filesystem is read-only. +2. **Error swallowing**: All internal callers use `cfg, _ := GetConfig()`, + turning a load failure into a nil-pointer panic instead of a graceful error. +3. **No mode awareness**: The same code path runs for CLI users (writable home + dir, interactive, config file is useful) and for the server (read-only + `/etc/opal`, headless, defaults are fine). + +--- + +## 2. Current Architecture + +```mermaid +graph TD + subgraph "Config Sources" + ENV["opal.env
(systemd EnvironmentFile)"] + YML["opal.yml
(Viper / YAML)"] + DEFAULTS["Hardcoded defaults
(in LoadConfig)"] + end + + subgraph "Loading Mechanisms" + AUTH_LOAD["auth.LoadConfig()
os.Getenv() → auth.Config"] + ENGINE_LOAD["engine.LoadConfig()
Viper → engine.Config"] + end + + subgraph "Consumers" + SERVER["API Server
(handlers, middleware)"] + CLI["CLI Commands
(cmd/ package)"] + end + + ENV --> AUTH_LOAD + YML --> ENGINE_LOAD + DEFAULTS --> ENGINE_LOAD + + AUTH_LOAD --> SERVER + ENGINE_LOAD --> SERVER + ENGINE_LOAD --> CLI +``` + +### Two independent config subsystems + +| Aspect | `engine.Config` | `auth.Config` | +|--------|----------------|---------------| +| **Source** | `opal.yml` via Viper | Environment variables | +| **Loaded by** | `engine.LoadConfig()` | `auth.LoadConfig()` | +| **Caching** | Singleton (`globalConfig`) | None (re-read each call) | +| **Write side effects** | Creates dir + file on load | None | +| **Error model** | Returns `(*Config, error)` | Returns `*Config` (no error) | +| **Used by** | CLI + Server (lazy) | Server only | + +### `engine.Config` field categories + +| Category | Fields | CLI | Server | Notes | +|----------|--------|-----|--------|-------| +| **Display** | `DefaultFilter`, `DefaultSort`, `DefaultReport`, `ColorOutput` | Yes | No | Terminal-only | +| **Date** | `WeekStartDay`, `DefaultDueTime` | Yes | Yes | Used by `ParseDate()` | +| **Urgency** | 13 urgency coefficients | Yes | Yes | Core scoring logic | +| **Limits** | `NextLimit` | Yes | Indirectly | Report limit | +| **Sync** | 6 sync fields | Yes | No | Client-side sync config | + +Key observation: The server only needs **urgency coefficients** and **date +settings** from `engine.Config`. It doesn't need display preferences or sync +settings. But all 30 fields are loaded through the same mechanism. + +### `engine.LoadConfig()` flow + +``` +1. Check globalConfig singleton → return if cached +2. GetConfigDir() → resolve directory path +3. os.MkdirAll(configDir) ← FAILS on read-only FS +4. Viper.ReadInConfig() +5. If read fails: + a. Viper.WriteConfigAs() ← FAILS on read-only FS + b. Re-read written file +6. Unmarshal → Config struct +7. Cache in globalConfig +``` + +Steps 3 and 5a are the failure points on the server. + +### Caller error handling audit + +| Caller | File:Line | Error handling | +|--------|-----------|----------------| +| `FormatTaskListWithFormat` | display.go:24 | `cfg, _ := GetConfig()` — **ignored** | +| `formatMinimalLine` | display.go:119 | `cfg, _ := GetConfig()` — **ignored** | +| `NextReport.SortFunc` | report.go:168 | `cfg, _ := GetConfig()` — **ignored** | +| `NextReport.LimitFunc` | report.go:187 | `cfg, _ := GetConfig()` — **ignored** | +| `sortByUrgency` | report.go:426 | `cfg, _ := GetConfig()` — **ignored** | +| `PopulateUrgency` | task.go:769 | `cfg, _ := GetConfig()` — **ignored** | +| `getWeekStart` | dateparse.go:484 | Checked — falls back to Monday | +| All cmd/ callers | root.go, sync.go | Checked — exits on error | + +Every engine-internal caller except `dateparse.go` discards the error. + +--- + +## 3. Additional Issues Found + +### 3a. Config file clobbering + +`LoadConfig()` line 296-306 catches **any** `ReadInConfig` error (not just +"file not found") and overwrites the config file with defaults. A YAML syntax +error in the user's `opal.yml` silently destroys their customizations. + +### 3b. `SaveConfig()` manual field sync + +Adding a new config field requires updating three places: +1. `Config` struct definition +2. `LoadConfig()` — `v.SetDefault(...)` call +3. `SaveConfig()` — `v.Set(...)` call + +Missing any one of these causes silent data loss on save or missing defaults. + +### 3c. `auth.LoadConfig()` silent parse failures + +```go +jwtExpiry, _ := strconv.Atoi(getEnv("JWT_EXPIRY", "3600")) +``` + +If `JWT_EXPIRY=abc`, `Atoi` returns `(0, error)`, error is discarded, and +`JWTExpiry` silently becomes 0 (tokens expire immediately). + +### 3d. Insecure JWT default + +`JWT_SECRET` defaults to `"change-me-in-production"`. While `validateServerConfig()` +checks for empty values, it doesn't check for the insecure default. + +### 3e. Sync API key in plaintext + +`sync_api_key` is stored in `opal.yml` with no special file permissions. + +### 3f. No env-var override of YAML config + +Viper's `AutomaticEnv()` is never called, so there's no way to override YAML +config values via environment variables. This is an obstacle for 12-factor +server deployments. + +--- + +## 4. Design Options + +### Option A: Minimal fix — graceful fallback to defaults + +**Change**: Make `LoadConfig()` return a `DefaultConfig()` when it can't read +or write the config file, instead of returning nil. + +```go +func DefaultConfig() *Config { + return &Config{ + DefaultFilter: "status:pending", + DefaultSort: "due,priority", + // ... all defaults ... + UrgencyDue: 12.0, + // ... + } +} + +func LoadConfig() (*Config, error) { + if globalConfig != nil { + return globalConfig, nil + } + + cfg, err := loadFromFile() + if err != nil { + // File not available — use defaults (common in server mode) + cfg = DefaultConfig() + } + + globalConfig = cfg + return cfg, nil +} +``` + +**Pros**: Smallest change, fixes the panic, no behavior change for CLI users. +**Cons**: Doesn't address the architectural confusion. Error-swallowing callers +remain. Write-on-read side effect remains for CLI. Three-source config stays. + +--- + +### Option B: Separate load paths for CLI and server + +**Change**: Split `LoadConfig()` into two functions: +- `LoadConfigFromFile()` — current behavior for CLI (read/write YAML) +- `LoadConfigFromDefaults()` — returns `DefaultConfig()` for server + +The server startup code calls `LoadConfigFromDefaults()` eagerly during init, +populating the singleton before any lazy `GetConfig()` call. + +```go +// cmd/server.go — in serverStartCmd.Run, before InitDB() +engine.LoadConfigFromDefaults() +``` + +**Pros**: Fixes the panic. Server path never touches the filesystem for config. +Explicit about which mode is running. CLI behavior unchanged. +**Cons**: Two code paths to maintain. Server can't be customized without code +changes (urgency tuning, etc.). + +--- + +### Option C: Read-only load with env-var overrides (recommended) + +**Change**: Restructure `LoadConfig()` to: +1. Start with hardcoded defaults (always succeeds) +2. Layer YAML file on top **if it exists** (read-only, never create) +3. Layer environment variables on top (via Viper `AutomaticEnv`) +4. Never write to the filesystem as a side effect of loading + +```go +func LoadConfig() (*Config, error) { + if globalConfig != nil { + return globalConfig, nil + } + + v := viper.New() + v.SetConfigType("yaml") + + // 1. Hardcoded defaults (always present) + setDefaults(v) + + // 2. YAML file overlay (optional, read-only) + if configPath, err := GetConfigPath(); err == nil { + v.SetConfigFile(configPath) + if err := v.ReadInConfig(); err != nil { + if _, ok := err.(viper.ConfigFileNotFoundError); !ok { + // File exists but is malformed — report, don't clobber + if _, statErr := os.Stat(configPath); statErr == nil { + return nil, fmt.Errorf("config file %s is invalid: %w", configPath, err) + } + // File doesn't exist — that's fine, use defaults + } + } + } + + // 3. Environment variable overlay + v.SetEnvPrefix("OPAL") + v.AutomaticEnv() + + cfg := &Config{} + if err := v.Unmarshal(cfg); err != nil { + return nil, fmt.Errorf("failed to unmarshal config: %w", err) + } + + globalConfig = cfg + return cfg, nil +} +``` + +File creation moves to an explicit `InitConfig()` function, called only during +`opal setup` and CLI first-run: + +```go +func InitConfig() error { + configDir, err := GetConfigDir() + if err != nil { + return err + } + if err := os.MkdirAll(configDir, 0755); err != nil { + return fmt.Errorf("failed to create config directory: %w", err) + } + return SaveConfig(DefaultConfig()) +} +``` + +Environment variable mapping: + +| YAML key | Env var | +|----------|---------| +| `urgency_due_coefficient` | `OPAL_URGENCY_DUE_COEFFICIENT` | +| `default_filter` | `OPAL_DEFAULT_FILTER` | +| `week_start_day` | `OPAL_WEEK_START_DAY` | +| ... | `OPAL_` | + +**Pros**: +- Fixes the panic — loading never fails fatally (missing file = defaults) +- Malformed YAML is reported, not silently clobbered +- Server can tune urgency via `opal.env` without deploying `opal.yml` +- 12-factor compatible (env vars override everything) +- CLI behavior unchanged (reads existing `opal.yml` + env overrides) +- Single code path for both CLI and server +- No filesystem write side effects during load + +**Cons**: +- `SaveConfig()` still needs the manual field-sync (separate improvement) +- Slightly more Viper configuration + +--- + +### Option D: Full restructure with typed config sections + +**Change**: Split `Config` into domain-specific sub-configs and unify the +loading mechanism for all config (merge `auth.Config` into the same system). + +```go +type Config struct { + Display DisplayConfig `mapstructure:"display"` + Urgency UrgencyConfig `mapstructure:"urgency"` + Sync SyncConfig `mapstructure:"sync"` + Server ServerConfig `mapstructure:"server"` // absorbs auth.Config +} +``` + +**Pros**: Clean separation. Single config system. Extensible. +**Cons**: Large refactor. Breaks existing `opal.yml` format. `auth.Config` +works fine as-is for its purpose. Over-engineered for the current problem. + +--- + +## 5. Recommendation + +**Option C** — read-only load with env-var overrides. + +It fixes the immediate panic, eliminates the write-on-read footgun, enables +server customization via environment variables, and does it with a focused +change to one function. It doesn't over-engineer or restructure things that +work. + +### Implementation plan + +#### Step 1: Extract `DefaultConfig()` constructor + +Create a `DefaultConfig()` function that returns a `*Config` with all defaults +populated. This is the single source of truth for default values — used by +`LoadConfig()`, `InitConfig()`, and `SaveConfig()`. + +**Files**: `internal/engine/config.go` + +#### Step 2: Rewrite `LoadConfig()` to be read-only + +Remove directory creation and file writing. Layer: defaults → YAML (if +exists) → env vars. Return `DefaultConfig()` on missing file instead of nil. +Return error only on malformed YAML (file exists but can't be parsed). + +**Files**: `internal/engine/config.go` + +#### Step 3: Extract `InitConfig()` for file creation + +Move the "create config directory + write defaults" logic into `InitConfig()`. +Call it from `initializeApp()` (CLI first-run path) and `opal setup`. + +**Files**: `internal/engine/config.go`, `cmd/root.go`, `cmd/setup.go` + +#### Step 4: Fix error-swallowing callers + +Two options, from least to most invasive: + +**4a (recommended)**: Make `GetConfig()` never return nil. Since `LoadConfig()` +now always returns a valid `*Config` (defaults on failure), `GetConfig()` also +always returns non-nil. The `cfg, _ := GetConfig()` pattern becomes safe. The +error return is kept for callers that want to distinguish "loaded from file" vs +"using defaults" but nil is never returned. + +**4b (defensive)**: Also make `BuildUrgencyCoefficients()` accept nil and return +default coefficients. Belt-and-suspenders. + +**Files**: `internal/engine/config.go`, `internal/engine/urgency.go` + +#### Step 5: Enable env-var overrides via Viper + +Add `v.SetEnvPrefix("OPAL")` and `v.AutomaticEnv()` so that any config key +can be overridden by `OPAL_`. Update `opal.env` in deployment docs to +show urgency tuning examples. + +**Files**: `internal/engine/config.go`, `docs/deployment.md` + +#### Step 6: Eliminate `SaveConfig()` field duplication + +Replace the manual `v.Set()` calls with Viper's `mapstructure` round-trip or +a reflect-based helper so new fields are automatically included. + +```go +func SaveConfig(cfg *Config) error { + v := viper.New() + v.SetConfigFile(configPath) + v.SetConfigType("yaml") + + // Use mapstructure tags to populate viper from struct + data, _ := mapstructure.Decode(cfg) // or manual marshal + for k, val := range data { + v.Set(k, val) + } + return v.WriteConfig() +} +``` + +Alternatively, marshal to YAML directly with `yaml.Marshal` and write the +bytes, bypassing Viper for the write path entirely. + +**Files**: `internal/engine/config.go` + +### Deployment changes + +After this redesign, the Ansible role needs **no changes** — `opal.env` is +sufficient. To customize urgency coefficients on the server, add lines to +`opal.env`: + +```bash +# Server-side urgency tuning (optional) +OPAL_URGENCY_DUE_COEFFICIENT=12.0 +OPAL_URGENCY_AGE_MAX=180 +``` + +No `opal.yml` deployment needed. The server runs on defaults + env overrides. + +--- + +## 6. Technical Decisions + +### ADR-1: Config loading must not write to the filesystem + +- **Context**: `LoadConfig()` creates directories and writes `opal.yml` as a + side effect. This fails on read-only filesystems (server) and is surprising + behavior for a "load" function. +- **Decision**: `LoadConfig()` becomes pure read. File creation moves to + `InitConfig()`, called explicitly during setup/first-run. +- **Alternatives**: Deploy `opal.yml` via Ansible; make `/etc/opal` writable. +- **Consequences**: CLI first-run code must call `InitConfig()` explicitly. + `IsFirstRun()` check remains in `initializeApp()`. + +### ADR-2: Missing config file returns defaults, not an error + +- **Context**: On the server, `opal.yml` doesn't exist and doesn't need to. + The current code treats this as a fatal error. +- **Decision**: Missing file = use defaults silently. Malformed file = return + error (don't clobber). `GetConfig()` never returns nil. +- **Alternatives**: Require `opal.yml` everywhere; use separate load functions + per mode. +- **Consequences**: Callers that discard errors (`cfg, _ := GetConfig()`) are + now safe. The error return still exists for callers that need to distinguish + "file loaded" from "using defaults". + +### ADR-3: Environment variables can override any config key + +- **Context**: The server is configured entirely via env vars (`opal.env` + + systemd). Currently urgency coefficients can only be set via `opal.yml`. +- **Decision**: Enable Viper's `AutomaticEnv()` with prefix `OPAL_`. Any YAML + key `foo_bar` can be overridden by `OPAL_FOO_BAR`. +- **Alternatives**: Add a server-specific config file; keep config values + hardcoded for server. +- **Consequences**: Layered config: defaults < YAML file < env vars. Aligns + with 12-factor app principles. Deployment can tune behavior without deploying + additional files. + +### ADR-4: Do not merge auth.Config into engine.Config + +- **Context**: `auth.Config` and `engine.Config` are completely independent + systems. Merging them would create a single unified config. +- **Decision**: Keep them separate. `auth.Config` loads from env vars, works + correctly, and is server-only. No reason to change it. +- **Alternatives**: Unified config struct with sections. +- **Consequences**: Two config types remain. This is acceptable because they + serve different purposes, have different lifecycles, and the current + `auth.Config` has no bugs. diff --git a/opal-task/cmd/server.go b/opal-task/cmd/server.go index 228df52..dc24031 100644 --- a/opal-task/cmd/server.go +++ b/opal-task/cmd/server.go @@ -113,6 +113,12 @@ Examples: os.Exit(1) } + // Load config (read-only — uses defaults if no opal.yml exists) + if _, err := engine.LoadConfig(); err != nil { + fmt.Fprintf(os.Stderr, "Error loading config: %v\n", err) + os.Exit(1) + } + // Initialize database if err := engine.InitDB(); err != nil { fmt.Fprintf(os.Stderr, "Error initializing database: %v\n", err) diff --git a/opal-task/internal/api/handlers/oauth.go b/opal-task/internal/api/handlers/oauth.go index 3dcd5bf..d02b99d 100644 --- a/opal-task/internal/api/handlers/oauth.go +++ b/opal-task/internal/api/handlers/oauth.go @@ -39,7 +39,22 @@ func GetLoginURL(w http.ResponseWriter, r *http.Request) { // OAuthCallback handles the OAuth callback func OAuthCallback(w http.ResponseWriter, r *http.Request) { - code := r.URL.Query().Get("code") + var code string + + // Support both GET (direct OAuth redirect) and POST (frontend exchange) + if r.Method == http.MethodPost { + var req struct { + Code string `json:"code"` + } + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + errorResponse(w, http.StatusBadRequest, "invalid request body") + return + } + code = req.Code + } else { + code = r.URL.Query().Get("code") + } + if code == "" { errorResponse(w, http.StatusBadRequest, "missing code parameter") return diff --git a/opal-task/internal/api/server.go b/opal-task/internal/api/server.go index c8c4b2e..d949971 100644 --- a/opal-task/internal/api/server.go +++ b/opal-task/internal/api/server.go @@ -41,6 +41,7 @@ func (s *Server) setupRoutes() { // OAuth endpoints (no auth required) r.Get("/auth/login", handlers.GetLoginURL) + r.Get("/auth/callback", handlers.OAuthCallback) r.Post("/auth/callback", handlers.OAuthCallback) r.Post("/auth/refresh", handlers.RefreshToken) r.Post("/auth/logout", handlers.Logout) diff --git a/opal-task/internal/auth/oauth.go b/opal-task/internal/auth/oauth.go index fb448b1..73cbc32 100644 --- a/opal-task/internal/auth/oauth.go +++ b/opal-task/internal/auth/oauth.go @@ -6,10 +6,23 @@ import ( "fmt" "io" "net/http" + "net/url" + "path" "golang.org/x/oauth2" ) +// issuerBase resolves ".." to get the base OAuth path from the issuer URL. +// e.g. "https://auth.example.com/application/o/app/" -> "https://auth.example.com/application/o/" +func issuerBase(issuer string) string { + u, err := url.Parse(issuer) + if err != nil { + return issuer + } + u.Path = path.Dir(path.Clean(u.Path)) + "/" + return u.String() +} + type OAuthClient struct { config *oauth2.Config cfg *Config @@ -22,8 +35,8 @@ func NewOAuthClient(cfg *Config) *OAuthClient { ClientSecret: cfg.OAuthClientSecret, RedirectURL: cfg.OAuthRedirectURI, Endpoint: oauth2.Endpoint{ - AuthURL: cfg.OAuthIssuer + "../authorize/", - TokenURL: cfg.OAuthIssuer + "../token/", + AuthURL: issuerBase(cfg.OAuthIssuer) + "authorize/", + TokenURL: issuerBase(cfg.OAuthIssuer) + "token/", }, Scopes: []string{"openid", "profile", "email"}, }, @@ -47,7 +60,7 @@ type UserInfo struct { } func (c *OAuthClient) GetUserInfo(ctx context.Context, accessToken string) (*UserInfo, error) { - req, err := http.NewRequestWithContext(ctx, "GET", c.cfg.OAuthIssuer+"../userinfo/", nil) + req, err := http.NewRequestWithContext(ctx, "GET", issuerBase(c.cfg.OAuthIssuer)+"userinfo/", nil) if err != nil { return nil, err } diff --git a/opal-task/internal/engine/urgency.go b/opal-task/internal/engine/urgency.go index 6bbb0f6..4028680 100644 --- a/opal-task/internal/engine/urgency.go +++ b/opal-task/internal/engine/urgency.go @@ -170,8 +170,12 @@ func (t *Task) urgencyProject(coeff float64) float64 { return 0.0 } -// BuildUrgencyCoefficients creates UrgencyCoefficients from config +// BuildUrgencyCoefficients creates UrgencyCoefficients from config. +// If cfg is nil, uses DefaultConfig() to prevent nil-pointer panics. func BuildUrgencyCoefficients(cfg *Config) *UrgencyCoefficients { + if cfg == nil { + cfg = DefaultConfig() + } return &UrgencyCoefficients{ Due: cfg.UrgencyDue, PriorityH: cfg.UrgencyPriorityH,