From 34aebb9cee60870695cdb227ff970bdf784be37a Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Tue, 5 May 2026 17:08:33 -0400 Subject: [PATCH 1/4] feat: unified (target, matchType, value) match shape for rules and permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the old `slug/owner/name` fields on AccessRule and `path/pathType` on RepoPermission with a single `match:` block shared by both. Config shape is now identical across URL rules and permissions: match: target: SLUG | OWNER | NAME value: type: LITERAL | GLOB | REGEX # optional — defaults to GLOB for rules, # LITERAL for permissions New MatchTarget and MatchType enums carry the match semantics. The SQL column for the pattern is named `match_value` (V5 migration) to avoid the H2 reserved keyword `value`. All YAML config files, Docker fixtures, docs, tests, and migration SQL are updated. closes #221 Co-Authored-By: Claude Sonnet 4.6 --- docker/git-proxy-docker-default.yml | 147 ++++--- docker/git-proxy-ldap.yml | 22 +- docker/git-proxy-local.yml | 11 +- docker/git-proxy-oidc.yml | 22 +- docs/CONFIGURATION.md | 384 ++++++++++-------- .../gitproxy/db/jdbc/DatabaseMigrator.java | 3 +- .../gitproxy/db/jdbc/JdbcUrlRuleRegistry.java | 12 +- .../db/jdbc/mapper/AccessRuleRowMapper.java | 8 +- .../finos/gitproxy/db/model/AccessRule.java | 21 +- .../finos/gitproxy/db/model/MatchTarget.java | 11 + .../finos/gitproxy/db/model/MatchType.java | 11 + .../db/mongo/MongoUrlRuleRegistry.java | 14 +- .../permission/JdbcRepoPermissionStore.java | 22 +- .../permission/MongoRepoPermissionStore.java | 18 +- .../gitproxy/permission/RepoPermission.java | 27 +- .../permission/RepoPermissionService.java | 36 +- .../servlet/filter/UrlRuleEvaluator.java | 56 ++- .../db/migration/V5__unified_rule_shape.sql | 55 +++ .../db/jdbc/mapper/RowMapperTest.java | 26 +- .../finos/gitproxy/db/model/DbModelTest.java | 33 +- .../MongoUrlRuleRegistryIntegrationTest.java | 63 +-- .../git/RepositoryUrlRuleHookTest.java | 64 +-- ...dbcRepoPermissionStoreIntegrationTest.java | 24 +- ...ngoRepoPermissionStoreIntegrationTest.java | 21 +- .../permission/RepoPermissionServiceTest.java | 201 +++------ .../servlet/filter/UrlRuleEvaluatorTest.java | 101 +++-- .../servlet/filter/UrlRuleFilterTest.java | 58 ++- .../controller/PermissionController.java | 34 +- .../controller/PermissionControllerTest.java | 67 +-- .../config/JettyConfigurationBuilder.java | 132 ++---- .../gitproxy/jetty/config/MatchConfig.java | 22 + .../jetty/config/PermissionConfig.java | 18 +- .../gitproxy/jetty/config/RuleConfig.java | 23 +- .../src/main/resources/git-proxy-local.yml | 108 ++--- .../e2e/IdentityResolutionE2ETest.java | 12 +- .../finos/gitproxy/e2e/JettyProxyFixture.java | 6 +- .../finos/gitproxy/e2e/PermissionE2ETest.java | 22 +- .../finos/gitproxy/e2e/UrlRuleE2ETest.java | 22 +- .../config/JettyConfigurationBuilderTest.java | 123 +++--- scripts/migrate-provider-ids.sql | 8 +- 40 files changed, 1129 insertions(+), 939 deletions(-) create mode 100644 git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchTarget.java create mode 100644 git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchType.java create mode 100644 git-proxy-java-core/src/main/resources/db/migration/V5__unified_rule_shape.sql create mode 100644 git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java diff --git a/docker/git-proxy-docker-default.yml b/docker/git-proxy-docker-default.yml index 265ae3ff..7608618c 100644 --- a/docker/git-proxy-docker-default.yml +++ b/docker/git-proxy-docker-default.yml @@ -48,32 +48,44 @@ permissions: # test-user: LITERAL — only /test-owner/test-repo - username: test-user provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: PUSH # user2: GLOB — any repo under otherorg - username: user2 provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: PUSH # user3: REGEX — test-owner repos matching test-repo.* - username: user3 provider: gitea - path: /test-owner/test-repo.* - path-type: REGEX + match: + target: SLUG + value: '/test-owner/test-repo.*' + type: REGEX operations: PUSH # reviewer: APPROVE on all the same paths - username: reviewer provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: REVIEW - username: reviewer provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: REVIEW # LDAP testuser (maps to gitea/test-user): same literal grant as test-user above. @@ -82,18 +94,26 @@ permissions: # so that push-permission-literal.sh still passes the deny-on-other-repo case. - username: testuser provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: PUSH # admin APPROVE grants for LDAP setup (admin approves via dashboard). - username: admin provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: REVIEW - username: admin provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: REVIEW providers: @@ -149,65 +169,70 @@ rules: allow: - enabled: true order: 110 - operations: - - FETCH - - PUSH - providers: - - gitea - slugs: - - /test-owner/test-repo - - /test-owner/test-repo-2 + operations: BOTH + provider: gitea + match: + target: SLUG + value: '/test-owner/test-repo(-2)?' + type: REGEX + - enabled: true order: 111 - operations: - - FETCH - - PUSH - providers: - - gitea - owners: - - otherorg + operations: BOTH + provider: gitea + match: + target: OWNER + value: otherorg + type: GLOB + - enabled: true order: 120 - operations: - - FETCH - providers: - - github - owners: - - finos + operations: FETCH + provider: github + match: + target: OWNER + value: finos + type: GLOB deny: - # Deny a specific repo inside the otherwise-allowed otherorg/* owner glob. - # Demonstrates that deny wins: user2 has GLOB permission on /otherorg/* and + # Deny a specific repo inside the otherwise-allowed otherorg owner. + # Demonstrates that deny wins: user2 has permission on otherorg and # the allow rule covers all of otherorg, but this repo is explicitly off-limits. - enabled: true order: 100 - operations: - - FETCH - - PUSH - providers: - - gitea - slugs: - - /otherorg/other-secret - - # Deny repos whose name ends in -readonly or -archived — these are archival - # copies that should not receive new commits through the proxy. + operations: BOTH + provider: gitea + match: + target: SLUG + value: /otherorg/other-secret + type: LITERAL + + # Deny repos whose name ends in -readonly or -archived — archival copies + # that should not receive new commits through the proxy. + - enabled: true + order: 101 + operations: PUSH + provider: gitea + match: + target: NAME + value: "*-readonly" + type: GLOB + - enabled: true order: 101 - operations: - - PUSH - providers: - - gitea - names: - - "*-readonly" - - "*-archived" - - # Regex deny: block any repo whose name contains the word "secret" as a - # distinct segment (e.g. secret-store, my-secret-keys, not "secretariat"). + operations: PUSH + provider: gitea + match: + target: NAME + value: "*-archived" + type: GLOB + + # Regex deny: block any repo whose name contains "secret" as a distinct segment. - enabled: true order: 102 - operations: - - PUSH - providers: - - gitea - names: - - "regex:(?i)(^|-)secret(-|$).*" + operations: PUSH + provider: gitea + match: + target: NAME + value: "(?i)(^|-)secret(-|$).*" + type: REGEX diff --git a/docker/git-proxy-ldap.yml b/docker/git-proxy-ldap.yml index 0b6a7bc8..c521f360 100644 --- a/docker/git-proxy-ldap.yml +++ b/docker/git-proxy-ldap.yml @@ -30,19 +30,29 @@ auth: permissions: - username: testuser provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: PUSH - username: testuser provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: PUSH - username: admin provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: REVIEW - username: admin provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: REVIEW diff --git a/docker/git-proxy-local.yml b/docker/git-proxy-local.yml index f62b5ba6..e083b678 100644 --- a/docker/git-proxy-local.yml +++ b/docker/git-proxy-local.yml @@ -18,12 +18,17 @@ users: permissions: - username: admin provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: PUSH_AND_REVIEW - username: admin provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: PUSH_AND_REVIEW providers: diff --git a/docker/git-proxy-oidc.yml b/docker/git-proxy-oidc.yml index d73f1f6b..3ce20915 100644 --- a/docker/git-proxy-oidc.yml +++ b/docker/git-proxy-oidc.yml @@ -37,19 +37,29 @@ auth: permissions: - username: testuser provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: PUSH - username: testuser provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: PUSH - username: admin provider: gitea - path: /test-owner/test-repo + match: + target: SLUG + value: /test-owner/test-repo + type: LITERAL operations: REVIEW - username: admin provider: gitea - path: /otherorg/* - path-type: GLOB + match: + target: OWNER + value: otherorg + type: GLOB operations: REVIEW diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 4fdc7fe4..b84d1499 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -811,99 +811,141 @@ URL rules control which repositories are accessible through the proxy. git-proxy rules are configured for a provider, all pushes and fetches to that provider are rejected. At least one allow rule must match for a request to proceed. -Slugs, owners, and names support glob patterns (e.g. `finos/*`, `*-public`) and Java regex via a `regex:` prefix. +Rules use a unified `match` block that specifies what to match against (`target`), the pattern string (`value`), and how +to interpret it (`type`). Evaluation is first-match-wins by `order` — identical to iptables/firewall rule semantics. ```yaml rules: allow: - # Specific repos by slug (owner/name) — both FETCH and PUSH + # Specific repo by exact slug — both operations, scoped to one provider - enabled: true order: 110 - operations: - - FETCH - - PUSH - providers: - - github - slugs: - - /finos/git-proxy - - /coopernetes/test-repo - - # All repos under an owner — FETCH only, any provider + operations: BOTH + provider: github + match: + target: SLUG + value: /finos/git-proxy + type: LITERAL + + # All repos under an owner — fetch only, any provider - enabled: true order: 120 - operations: - - FETCH - owners: - - finos + operations: FETCH + match: + target: OWNER + value: finos + type: GLOB deny: # Block a specific repo across all operations - enabled: true order: 100 - slugs: - - /myorg/forbidden-repo + match: + target: SLUG + value: /myorg/forbidden-repo + type: LITERAL ``` -To allow all repositories on a provider (open mode), use a wildcard slug: +To allow all repositories on a provider (open mode): ```yaml rules: allow: - enabled: true order: 110 - operations: - - FETCH - - PUSH - slugs: - - "*/*" + operations: BOTH + provider: internal-github + match: + target: OWNER + value: "*" + type: GLOB ``` ### URL rule properties -| Property | Type | Default | Description | -| ------------ | ------- | ------- | -------------------------------------------------------------------------- | -| `enabled` | boolean | `true` | Whether this entry is active | -| `order` | int | — | Evaluation order (lower = earlier; first match wins) | -| `operations` | list | _none_ | `FETCH`, `PUSH` — which operations this entry matches | -| `providers` | list | _all_ | Provider names to scope this entry to | -| `slugs` | list | _none_ | `/owner/repo` slugs; supports glob patterns and `regex:` prefix | -| `owners` | list | _none_ | Owner/org names; supports glob patterns and `regex:` prefix | -| `names` | list | _none_ | Repository names; supports glob patterns and `regex:` prefix | +| Property | Type | Default | Description | +| ------------- | ------- | -------- | -------------------------------------------------------------------- | +| `enabled` | boolean | `true` | Whether this entry is active | +| `order` | int | `1100` | Evaluation order (lower = earlier; first match wins) | +| `operations` | string | `BOTH` | `FETCH`, `PUSH`, or `BOTH` — which operations this entry matches | +| `provider` | string | _(all)_ | Provider name to scope this entry to; omit or leave blank for all | +| `match` | object | — | Repository match criteria — see below | +| `match.target`| enum | `SLUG` | What to match: `SLUG` (`/owner/repo`), `OWNER`, or `NAME` | +| `match.value` | string | — | The pattern to match against the chosen target | +| `match.type` | enum | `GLOB` | How to interpret the pattern: `LITERAL`, `GLOB`, or `REGEX` | -### Pattern matching in rules +### Pattern matching -All three list fields (`slugs`, `owners`, `names`) support three matching modes: +Each rule matches exactly one thing — the `match` block selects what URL part to test (`target`) and how to test it +(`type`). To express an AND condition (e.g. specific owner AND specific name prefix), use `target: SLUG` and write a +single glob or regex that covers both parts. -- **Literal** (default): exact string match -- **Glob**: patterns using `*` (any characters) and `?` (single character) — e.g. `finos/*`, `*-public` -- **Regex**: prefix the pattern with `regex:` to use a full Java regular expression — e.g. `regex:(?i)(^|-)secret(-|$).*` +#### LITERAL + +Exact string match after normalising a leading `/`. `/acme/repo` and `acme/repo` are equivalent. + +#### GLOB + +Wildcard matching using `*` (any characters) and `?` (single character). + +| Target | `*` behaviour | +| ------- | ------------------------------------------------ | +| `SLUG` | Does **not** cross `/` — use `acme/*` not `acme/**` | +| `OWNER` | Owner names cannot contain `/` — `*` matches any valid name | +| `NAME` | Repo names cannot contain `/` — `*` matches any valid name | + +| Pattern (GLOB, target=SLUG) | Matches | Does NOT match | +| --------------------------- | ------- | -------------- | +| `/acme/repo` _(LITERAL)_ | `/acme/repo` | `/acme/other` | +| `/acme/*` | `/acme/repo`, `/acme/my-service` | `/other/repo` | +| `/acme/service-*` | `/acme/service-api`, `/acme/service-worker` | `/acme/repo` | +| `/acme/repo-?` | `/acme/repo-1`, `/acme/repo-a` | `/acme/repo-12` | + +#### REGEX + +Full Java regular expression. A few things to know before writing regex rules: + +- **Full-string match**: the pattern must match the entire candidate string — there are no implicit anchors, but + `matches()` semantics apply. A pattern of `acme` does **not** match `/acme/repo`; write `/acme/.*` or `.*acme.*`. +- **`/` does not need escaping**: Java regex uses strings, not a `/pattern/` literal syntax. Write `/acme/.*` not + `\/acme\/.*`. +- **Case-insensitive**: use the `(?i)` inline flag — e.g. `(?i)/acme/.*`. +- **Anchoring**: explicit `^` and `$` are redundant with `matches()` but harmless if included. + +| Pattern (REGEX, target=SLUG) | Matches | Does NOT match | +| ------------------------------------- | -------------------------------- | ------------------- | +| `/acme/.*` | `/acme/repo`, `/acme/my-service` | `/other/repo` | +| `/(acme\|partner)/.*` | `/acme/repo`, `/partner/repo` | `/other/repo` | +| `/acme/service-[0-9]+` | `/acme/service-1`, `/acme/service-42` | `/acme/service-api` | +| `(?i)/acme/.*` | `/acme/repo`, `/ACME/Repo` | `/other/repo` | + +| Pattern (REGEX, target=NAME) | Matches | Does NOT match | +| ------------------------------------------- | ------------------------------ | --------------- | +| `(?i)(^|-)secret(-|$).*` | `secret-config`, `my-secret` | `secretariat` | +| `migrate-.*` | `migrate-app`, `migrate-db` | `old-migrate` | ```yaml rules: deny: - # Block any repo whose name contains "secret" as a distinct word segment + # Block any repo whose name contains "secret" as a distinct word segment (case-insensitive) - enabled: true order: 50 - operations: - - PUSH - names: - - "regex:(?i)(^|-)secret(-|$).*" + operations: PUSH + match: + target: NAME + value: "(?i)(^|-)secret(-|$).*" + type: REGEX - # Block GitHub Pages repos (glob on name) + # Block repos matching multiple owner orgs using alternation - enabled: true order: 51 - operations: - - PUSH - providers: - - github - names: - - "*.github.io" + operations: BOTH + match: + target: OWNER + value: "(blocked-org|suspended-org)" + type: REGEX ``` -> **Glob on `owners` and `names`:** Owner and repository names cannot contain `/`, so `*` reliably matches any -> valid name including hyphens, underscores, and dots. You do not need `**` in these fields — `*` is sufficient -> and the two are equivalent in this context. - ### Real-world URL rule examples **Gateway for a specific SCM — allow all push and fetch:** @@ -913,90 +955,86 @@ rules: allow: - enabled: true order: 110 - operations: - - FETCH - - PUSH - providers: - - internal-github - slugs: - - "*/*" + operations: BOTH + provider: internal-github + match: + target: OWNER + value: "*" + type: GLOB ``` **Allow repos under a set of known owner orgs, identified by a name prefix:** -This is useful when multiple teams or divisions share a SCM and are distinguished by an owner-name convention -(e.g. `teamA-*`, `teamB-*`). - ```yaml rules: allow: - enabled: true order: 110 - operations: - - FETCH - - PUSH - providers: - - internal-github - owners: - - "regex:team-(alpha|beta|gamma)" + operations: BOTH + provider: internal-github + match: + target: OWNER + value: "team-(alpha|beta|gamma)" + type: REGEX ``` **Allow push only for repos whose name starts with a known prefix:** -When application or project repos are identified by a prefix (e.g. a fixed-length code followed by a hyphen), -match on `names` rather than the full slug so the rule applies regardless of which owner org the repo lives under. +When repos are identified by a project code followed by a hyphen, match on `NAME` so the rule applies regardless of +which org the repo lives under. ```yaml rules: allow: - enabled: true order: 110 - operations: - - PUSH - providers: - - internal-github - names: - - "proj0-*" - - "proj1-*" - - "shared-*" + operations: PUSH + provider: internal-github + match: + target: NAME + value: "proj0-*" + type: GLOB + + # Multiple project prefixes — one rule per prefix, or combine with REGEX alternation: + allow: + - enabled: true + order: 111 + operations: PUSH + provider: internal-github + match: + target: NAME + value: "(proj0|proj1|shared)-.*" + type: REGEX ``` -**Combine owner and name matching (SCM-to-SCM migration scenario):** +**Combine owner and name matching (AND condition):** -When acting as a gateway between two SCMs (e.g. during an M&A integration), you may want to allow only repos -that belong to a specific org AND follow a naming pattern. Use `slugs` with a glob or regex for this — each -entry in `owners` and `names` is evaluated independently (OR logic), so combining both in one rule block does -not produce an AND condition. +Use `target: SLUG` with a glob or regex — the slug is `/owner/repo` so a single pattern can constrain both parts. ```yaml rules: allow: - # Allow fetch from the source SCM for a specific org + name prefix — use slug for AND logic + # Allow fetch from the source SCM for a specific org + name prefix (glob AND) - enabled: true order: 110 - operations: - - FETCH - providers: - - source-github - slugs: - - "acquired-org/migrate-*" - - # Allow push to the destination SCM for the same set, using regex for stricter control + operations: FETCH + provider: source-github + match: + target: SLUG + value: "acquired-org/migrate-*" + type: GLOB + + # Allow push to the destination SCM — stricter control with regex - enabled: true order: 120 - operations: - - PUSH - providers: - - dest-gitlab - slugs: - - "regex:/migrated-org/migrate-.*" + operations: PUSH + provider: dest-gitlab + match: + target: SLUG + value: "/migrated-org/migrate-.*" + type: REGEX ``` -> **`owners` and `names` are OR conditions.** An entry under `owners: [acme]` allows any repo whose owner is -> `acme`, regardless of repo name. An entry under `names: [migrate-*]` allows any repo whose name matches, -> regardless of owner. To restrict both owner AND name together, use `slugs` with a glob (e.g. -> `acme/migrate-*`) or a `regex:`-prefixed pattern. - ## Permissions Permissions control which proxy users can push to or review pushes from specific repositories. They are checked @@ -1009,104 +1047,118 @@ permissions: # LITERAL (default): exact /owner/repo match - username: alice provider: github - path: /myorg/myrepo + match: + target: SLUG + value: /myorg/myrepo + type: LITERAL operations: PUSH - # GLOB: wildcard owner or repo + # GLOB: wildcard repo name under a specific owner - username: bob provider: gitlab - path: /myorg/* - path-type: GLOB + match: + target: SLUG + value: /myorg/* + type: GLOB operations: PUSH_AND_REVIEW - # REGEX: full Java regex matched against the /owner/repo path + # OWNER target: grant access to all repos under an org - username: carol provider: github - path: \/myorg\/service-.* - path-type: REGEX + match: + target: OWNER + value: myorg + type: GLOB operations: REVIEW + # REGEX on SLUG: match repos under multiple orgs + - username: dave + provider: github + match: + target: SLUG + value: "/team-(alpha|beta)/.*" + type: REGEX + operations: PUSH_AND_REVIEW + # SELF_CERTIFY: trusted contributor who can approve their own clean pushes. # Requires both this permission entry AND the SELF_CERTIFY role on the user. - username: trusted provider: github - path: /myorg/myrepo + match: + target: SLUG + value: /myorg/myrepo + type: LITERAL operations: SELF_CERTIFY ``` ### Permission properties -| Property | Type | Default | Description | -| ----------- | ------ | --------- | ------------------------------------------------------------------------------------------- | -| `username` | string | — | Proxy username (must match a `users:` entry or a DB user) | -| `provider` | string | — | Provider name as defined in `providers:` config | -| `path` | string | — | Repository path pattern (`/owner/repo`); interpretation depends on `path-type` | -| `path-type` | enum | `LITERAL` | `LITERAL` (exact), `GLOB` (`*`/`?` wildcards), `REGEX` (Java regex against the full path) | -| `operations`| enum | `PUSH` | What the user may do: `PUSH`, `REVIEW`, `PUSH_AND_REVIEW`, `SELF_CERTIFY` | - -### GLOB path semantics +| Property | Type | Default | Description | +| --------------- | ------ | ---------------- | ------------------------------------------------------------------------------- | +| `username` | string | — | Proxy username (must match a `users:` entry or a DB user) | +| `provider` | string | — | Provider name as defined in `providers:` config | +| `match` | object | — | Repository match criteria — see below | +| `match.target` | enum | `SLUG` | What to match: `SLUG` (`/owner/repo`), `OWNER`, or `NAME` | +| `match.value` | string | — | The pattern to match against the chosen target | +| `match.type` | enum | `LITERAL` | How to interpret the pattern: `LITERAL`, `GLOB`, or `REGEX` | +| `operations` | enum | `PUSH_AND_REVIEW`| What the user may do: `PUSH`, `REVIEW`, `PUSH_AND_REVIEW`, `SELF_CERTIFY` | -GLOB matching uses Java NIO `PathMatcher` (`glob:` prefix). Paths follow the `/owner/repo` convention. +> **Default match type differs between rules and permissions.** URL rules default to `GLOB` (safer for broad allow/deny +> policies). Permissions default to `LITERAL` (safer for access grants — explicit intent required to use wildcards). -| Wildcard | Matches | Does NOT match | -| -------- | ------- | -------------- | -| `*` | Any sequence of characters within **one** path segment (no `/` crossing) | Another path segment or the `/` separator itself | -| `**` | Any sequence of characters **including** path separators (zero or more segments) | — | -| `?` | Exactly **one** character within a path segment | `/` or a multi-character sequence | +### Pattern matching -Hyphens, digits, and dots in names are treated as regular characters — no special escaping needed. +Permissions support the same three match types as URL rules (LITERAL, GLOB, REGEX) applied to the same three targets +(SLUG, OWNER, NAME). See [Pattern matching](#pattern-matching) above for full semantics including regex behaviour. -**Pattern examples:** +GLOB on `target: SLUG` follows slug path conventions: -| Pattern | Matches | Does NOT match | -| ------- | ------- | -------------- | -| `/acme/repo` _(LITERAL)_ | `/acme/repo` | `/acme/other` | -| `/acme/*` | `/acme/repo`, `/acme/my-service`, `/acme/repo-v2` | `/acme/sub/repo`, `/other/repo` | -| `/acme/**` | `/acme/repo`, `/acme/sub/repo`, `/acme/a/b/c` | `/other/repo` | -| `/**` | Every path | — | -| `/*/repo` | `/acme/repo`, `/other/repo` | `/acme/other-repo` | -| `/acme/service-*` | `/acme/service-api`, `/acme/service-worker` | `/acme/repo`, `/acme/my-service-api` | -| `/acme/repo-?` | `/acme/repo-1`, `/acme/repo-a` | `/acme/repo-12`, `/acme/repo-` | - -> **Glob on the repo-name segment:** Repository names cannot contain `/`, so `*` will never cross a path separator -> when used in the name position (e.g. `/owner/prefix-*`). In this position `*` and `**` are equivalent — use `*`. +| Pattern (GLOB, target=SLUG) | Matches | Does NOT match | +| ----------------------------- | -------------------------------------- | ------------------- | +| `/acme/repo` | `/acme/repo` | `/acme/other` | +| `/acme/*` | `/acme/repo`, `/acme/my-service` | `/other/repo` | +| `/acme/service-*` | `/acme/service-api`, `/acme/service-worker` | `/acme/repo` | +| `/*/proj0-*` | `/acme/proj0-api`, `/other/proj0-db` | `/acme/other` | > **Conflict detection:** At config load time and when saving via the dashboard API, git-proxy-java rejects any -> new permission entry whose path overlaps with an existing entry for the same user and provider. Two paths overlap -> when they are equal, or when one is a GLOB/REGEX pattern that would match the other path string. This prevents +> new permission entry whose pattern overlaps with an existing entry for the same user and provider. Two entries overlap +> when they are equal, or when one is a GLOB/REGEX pattern that would match the other's value. This prevents > silent misconfiguration where the effective permission depends on evaluation order. ### Real-world permission examples -**Gateway use case — allow a user to push any repo on a specific provider:** +**Allow a user to push any repo on a specific provider:** ```yaml permissions: - username: alice provider: internal-github - path: "/**" - path-type: GLOB + match: + target: OWNER + value: "*" + type: GLOB operations: PUSH_AND_REVIEW ``` -**Allow push to all repos whose name starts with a known prefix (e.g. a project code):** - -Useful when repos are identified by a fixed prefix such as a project or application code. Since repo names cannot -contain `/`, the `*` in the name segment matches any suffix including hyphens and digits. +**Allow push to repos whose name starts with a project code:** ```yaml permissions: - username: alice provider: internal-github - path: "/myorg/proj0-*" - path-type: GLOB + match: + target: NAME + value: "proj0-*" + type: GLOB operations: PUSH - # Or match the same prefix across any owner using a wildcard in the owner position: + # Or match the same prefix across any owner using SLUG: - username: alice provider: internal-github - path: "/*/proj0-*" - path-type: GLOB + match: + target: SLUG + value: "/*/proj0-*" + type: GLOB operations: PUSH ``` @@ -1116,8 +1168,10 @@ permissions: permissions: - username: alice provider: internal-github - path: "/team-(alpha|beta)/.*" - path-type: REGEX + match: + target: SLUG + value: "/team-(alpha|beta)/.*" + type: REGEX operations: PUSH_AND_REVIEW ``` @@ -1131,15 +1185,19 @@ permissions: # Push and review access - username: trusted-dev provider: internal-github - path: "/myorg/proj0-*" - path-type: GLOB + match: + target: NAME + value: "proj0-*" + type: GLOB operations: PUSH_AND_REVIEW # Self-certify on the same scope (requires SELF_CERTIFY role on the user too) - username: trusted-dev provider: internal-github - path: "/myorg/proj0-*" - path-type: GLOB + match: + target: NAME + value: "proj0-*" + type: GLOB operations: SELF_CERTIFY ``` diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/DatabaseMigrator.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/DatabaseMigrator.java index a6f4eda3..3a6fb8f3 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/DatabaseMigrator.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/DatabaseMigrator.java @@ -40,7 +40,8 @@ private record Migration(String version, String description, String resource, bo new Migration( "2.1", "widen provider columns", "db/migration-postgresql/V2_1__widen_provider_columns.sql", true), new Migration("3", "email unique constraint", "db/migration/V3__email_unique.sql", false), - new Migration("4", "spring session tables", "db/migration/V4__spring_session.sql", false)); + new Migration("4", "spring session tables", "db/migration/V4__spring_session.sql", false), + new Migration("5", "unified rule shape", "db/migration/V5__unified_rule_shape.sql", false)); // --------------------------------------------------------------------------- diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcUrlRuleRegistry.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcUrlRuleRegistry.java index 06004905..421edc45 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcUrlRuleRegistry.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/JdbcUrlRuleRegistry.java @@ -34,10 +34,10 @@ public void initialize() { public void save(AccessRule rule) { jdbc.update(""" INSERT INTO access_rules - (id, provider, slug, owner, name, access, operations, + (id, provider, target, match_value, match_type, access, operations, description, enabled, rule_order, source) VALUES - (:id, :provider, :slug, :owner, :name, :access, :operations, + (:id, :provider, :target, :matchValue, :matchType, :access, :operations, :description, :enabled, :ruleOrder, :source) """, params(rule)); } @@ -46,7 +46,7 @@ public void save(AccessRule rule) { public void update(AccessRule rule) { jdbc.update(""" UPDATE access_rules SET - provider = :provider, slug = :slug, owner = :owner, name = :name, + provider = :provider, target = :target, match_value = :matchValue, match_type = :matchType, access = :access, operations = :operations, description = :description, enabled = :enabled, rule_order = :ruleOrder, source = :source WHERE id = :id @@ -84,9 +84,9 @@ private static MapSqlParameterSource params(AccessRule rule) { return new MapSqlParameterSource() .addValue("id", rule.getId()) .addValue("provider", rule.getProvider()) - .addValue("slug", rule.getSlug()) - .addValue("owner", rule.getOwner()) - .addValue("name", rule.getName()) + .addValue("target", rule.getTarget().name()) + .addValue("matchValue", rule.getValue()) + .addValue("matchType", rule.getMatchType().name()) .addValue("access", rule.getAccess().name()) .addValue("operations", rule.getOperations().name()) .addValue("description", rule.getDescription()) diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/mapper/AccessRuleRowMapper.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/mapper/AccessRuleRowMapper.java index 3eb2afa4..05d39b90 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/mapper/AccessRuleRowMapper.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/jdbc/mapper/AccessRuleRowMapper.java @@ -3,6 +3,8 @@ import java.sql.ResultSet; import java.sql.SQLException; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.springframework.jdbc.core.RowMapper; /** Maps an {@code access_rules} result-set row to an {@link AccessRule}. */ @@ -17,9 +19,9 @@ public AccessRule mapRow(ResultSet rs, int rowNum) throws SQLException { return AccessRule.builder() .id(rs.getString("id")) .provider(rs.getString("provider")) - .slug(rs.getString("slug")) - .owner(rs.getString("owner")) - .name(rs.getString("name")) + .target(MatchTarget.valueOf(rs.getString("target"))) + .value(rs.getString("match_value")) + .matchType(MatchType.valueOf(rs.getString("match_type"))) .access(AccessRule.Access.valueOf(rs.getString("access"))) .operations(AccessRule.Operations.valueOf(rs.getString("operations"))) .description(rs.getString("description")) diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/AccessRule.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/AccessRule.java index a66ae57b..e7603235 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/AccessRule.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/AccessRule.java @@ -7,11 +7,10 @@ /** * A single access control rule governing which repositories may be fetched from or pushed to through the proxy. Rules - * are evaluated by {@code UrlAccessControlFilter} (see #60). + * are evaluated by {@code UrlRuleEvaluator} (see #60). * - *

The {@code owner}, {@code name}, and {@code slug} fields use the standard {@code {owner}/{repo}} URL shape shared - * by GitHub, GitLab, Gitea, Codeberg, and Forgejo. Generic providers with arbitrary URL paths should use {@code slug} - * with a glob pattern and leave {@code owner}/{@code name} null. + *

{@link #target} selects which part of the repo URL is compared; {@link #value} is the pattern string; + * {@link #matchType} controls how the pattern is interpreted (literal equality, glob, or regex). */ @Data @Builder @@ -24,14 +23,16 @@ public class AccessRule { /** Provider name this rule applies to. Null = applies to all providers. */ private String provider; - /** Glob pattern matching {@code /owner/repo} slug. Null = not used. */ - private String slug; + /** Which part of the repository URL is matched. Defaults to {@link MatchTarget#SLUG}. */ + @Builder.Default + private MatchTarget target = MatchTarget.SLUG; - /** Glob pattern matching the owner/org portion of the URL. Null = not used. */ - private String owner; + /** Pattern to match against the {@link #target} portion of the URL. */ + private String value; - /** Glob pattern matching the repository name portion of the URL. Null = not used. */ - private String name; + /** How {@link #value} is interpreted when matching. Defaults to {@link MatchType#GLOB}. */ + @Builder.Default + private MatchType matchType = MatchType.GLOB; /** Whether this rule allows or denies matched repositories. */ @Builder.Default diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchTarget.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchTarget.java new file mode 100644 index 00000000..5251c4db --- /dev/null +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchTarget.java @@ -0,0 +1,11 @@ +package org.finos.gitproxy.db.model; + +/** Which part of the repository URL path the match pattern is applied to. */ +public enum MatchTarget { + /** Full {@code /owner/repo} slug. */ + SLUG, + /** Owner or organisation portion only (e.g. {@code myorg}). */ + OWNER, + /** Repository name portion only (e.g. {@code my-repo}). */ + NAME +} diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchType.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchType.java new file mode 100644 index 00000000..a1503abc --- /dev/null +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/model/MatchType.java @@ -0,0 +1,11 @@ +package org.finos.gitproxy.db.model; + +/** How a pattern value is matched against a repository path component. */ +public enum MatchType { + /** Exact string equality (case-sensitive). Leading {@code /} is normalised before comparison. */ + LITERAL, + /** Shell glob: {@code *} matches within one path segment; {@code **} matches any depth. */ + GLOB, + /** Full Java regex matched against the value as-is. */ + REGEX +} diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistry.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistry.java index 73e2ceff..39b5cc63 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistry.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistry.java @@ -12,6 +12,8 @@ import org.bson.Document; import org.finos.gitproxy.db.UrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -85,9 +87,9 @@ private MongoCollection getCollection() { private static Document toDocument(AccessRule r) { return new Document("_id", r.getId()) .append("provider", r.getProvider()) - .append("slug", r.getSlug()) - .append("owner", r.getOwner()) - .append("name", r.getName()) + .append("target", r.getTarget().name()) + .append("value", r.getValue()) + .append("matchType", r.getMatchType().name()) .append("access", r.getAccess().name()) .append("operations", r.getOperations().name()) .append("description", r.getDescription()) @@ -100,9 +102,9 @@ private static AccessRule fromDocument(Document doc) { return AccessRule.builder() .id(doc.getString("_id")) .provider(doc.getString("provider")) - .slug(doc.getString("slug")) - .owner(doc.getString("owner")) - .name(doc.getString("name")) + .target(MatchTarget.valueOf(doc.getString("target"))) + .value(doc.getString("value")) + .matchType(MatchType.valueOf(doc.getString("matchType"))) .access(AccessRule.Access.valueOf(doc.getString("access"))) .operations(AccessRule.Operations.valueOf(doc.getString("operations"))) .description(doc.getString("description")) diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/JdbcRepoPermissionStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/JdbcRepoPermissionStore.java index ae2ec6b0..cb33e9e9 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/JdbcRepoPermissionStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/JdbcRepoPermissionStore.java @@ -6,6 +6,8 @@ import java.util.Map; import java.util.Optional; import javax.sql.DataSource; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.springframework.jdbc.core.RowMapper; import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; @@ -22,8 +24,8 @@ public JdbcRepoPermissionStore(DataSource dataSource) { @Override public void save(RepoPermission p) { jdbc.update(""" - INSERT INTO repo_permissions (id, username, provider, path, path_type, operations, source) - VALUES (:id, :username, :provider, :path, :pathType, :operations, :source) + INSERT INTO repo_permissions (id, username, provider, target, match_value, match_type, operations, source) + VALUES (:id, :username, :provider, :target, :matchValue, :matchType, :operations, :source) """, params(p)); } @@ -40,13 +42,13 @@ public Optional findById(String id) { @Override public List findAll() { - return jdbc.query("SELECT * FROM repo_permissions ORDER BY provider, path, username", ROW_MAPPER); + return jdbc.query("SELECT * FROM repo_permissions ORDER BY provider, match_value, username", ROW_MAPPER); } @Override public List findByUsername(String username) { return jdbc.query( - "SELECT * FROM repo_permissions WHERE username = :username ORDER BY provider, path", + "SELECT * FROM repo_permissions WHERE username = :username ORDER BY provider, match_value", Map.of("username", username), ROW_MAPPER); } @@ -54,7 +56,7 @@ public List findByUsername(String username) { @Override public List findByProvider(String provider) { return jdbc.query( - "SELECT * FROM repo_permissions WHERE provider = :provider ORDER BY path, username", + "SELECT * FROM repo_permissions WHERE provider = :provider ORDER BY match_value, username", Map.of("provider", provider), ROW_MAPPER); } @@ -64,8 +66,9 @@ private static MapSqlParameterSource params(RepoPermission p) { .addValue("id", p.getId()) .addValue("username", p.getUsername()) .addValue("provider", p.getProvider()) - .addValue("path", p.getPath()) - .addValue("pathType", p.getPathType().name()) + .addValue("target", p.getTarget().name()) + .addValue("matchValue", p.getValue()) + .addValue("matchType", p.getMatchType().name()) .addValue("operations", p.getOperations().name()) .addValue("source", p.getSource().name()); } @@ -77,8 +80,9 @@ private static RepoPermission mapRow(ResultSet rs, int i) throws SQLException { .id(rs.getString("id")) .username(rs.getString("username")) .provider(rs.getString("provider")) - .path(rs.getString("path")) - .pathType(RepoPermission.PathType.valueOf(rs.getString("path_type"))) + .target(MatchTarget.valueOf(rs.getString("target"))) + .value(rs.getString("match_value")) + .matchType(MatchType.valueOf(rs.getString("match_type"))) .operations(RepoPermission.Operations.valueOf(rs.getString("operations"))) .source(RepoPermission.Source.valueOf(rs.getString("source"))) .build(); diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/MongoRepoPermissionStore.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/MongoRepoPermissionStore.java index 68defc5f..10b76a4d 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/MongoRepoPermissionStore.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/MongoRepoPermissionStore.java @@ -10,6 +10,8 @@ import java.util.List; import java.util.Optional; import org.bson.Document; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -54,7 +56,7 @@ public List findAll() { List results = new ArrayList<>(); getCollection() .find() - .sort(Sorts.ascending("provider", "path", "username")) + .sort(Sorts.ascending("provider", "value", "username")) .forEach(doc -> results.add(fromDocument(doc))); return results; } @@ -64,7 +66,7 @@ public List findByUsername(String username) { List results = new ArrayList<>(); getCollection() .find(Filters.eq("username", username)) - .sort(Sorts.ascending("provider", "path")) + .sort(Sorts.ascending("provider", "value")) .forEach(doc -> results.add(fromDocument(doc))); return results; } @@ -74,7 +76,7 @@ public List findByProvider(String provider) { List results = new ArrayList<>(); getCollection() .find(Filters.eq("provider", provider)) - .sort(Sorts.ascending("path", "username")) + .sort(Sorts.ascending("value", "username")) .forEach(doc -> results.add(fromDocument(doc))); return results; } @@ -87,8 +89,9 @@ private static Document toDocument(RepoPermission p) { return new Document("_id", p.getId()) .append("username", p.getUsername()) .append("provider", p.getProvider()) - .append("path", p.getPath()) - .append("pathType", p.getPathType().name()) + .append("target", p.getTarget().name()) + .append("value", p.getValue()) + .append("matchType", p.getMatchType().name()) .append("operations", p.getOperations().name()) .append("source", p.getSource().name()); } @@ -98,8 +101,9 @@ private static RepoPermission fromDocument(Document doc) { .id(doc.getString("_id")) .username(doc.getString("username")) .provider(doc.getString("provider")) - .path(doc.getString("path")) - .pathType(RepoPermission.PathType.valueOf(doc.getString("pathType"))) + .target(MatchTarget.valueOf(doc.getString("target"))) + .value(doc.getString("value")) + .matchType(MatchType.valueOf(doc.getString("matchType"))) .operations(RepoPermission.Operations.valueOf(doc.getString("operations"))) .source(RepoPermission.Source.valueOf(doc.getString("source"))) .build(); diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermission.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermission.java index bdd659f0..6921cea3 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermission.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermission.java @@ -5,14 +5,16 @@ import lombok.Builder; import lombok.Data; import lombok.NoArgsConstructor; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; /** * A single authorization grant: {@link #username} is permitted to perform {@link #operations} on repos matching - * {@link #path} at {@link #provider}. + * {@link #value} at {@link #provider}. * - *

{@link #path} is a pattern of the form {@code /owner/repo}. {@link #pathType} controls how it is matched: - * {@code LITERAL} for exact equality, {@code GLOB} for {@code *}/{@code ?} wildcards, {@code REGEX} for full Java regex - * matched against the path string. + *

{@link #target} selects which part of the repo URL is compared (default {@link MatchTarget#SLUG}); + * {@link #matchType} controls how {@link #value} is interpreted: {@code LITERAL} for exact equality, {@code GLOB} for + * {@code *}/{@code ?} wildcards, {@code REGEX} for full Java regex. */ @Data @Builder @@ -25,10 +27,17 @@ public class RepoPermission { private String username; private String provider; - private String path; + /** Which part of the repository URL is matched. Defaults to {@link MatchTarget#SLUG}. */ @Builder.Default - private PathType pathType = PathType.LITERAL; + private MatchTarget target = MatchTarget.SLUG; + + /** Pattern to match against the {@link #target} portion of the URL. */ + private String value; + + /** How {@link #value} is interpreted when matching. Defaults to {@link MatchType#LITERAL}. */ + @Builder.Default + private MatchType matchType = MatchType.LITERAL; @Builder.Default private Operations operations = Operations.PUSH; @@ -36,12 +45,6 @@ public class RepoPermission { @Builder.Default private Source source = Source.DB; - public enum PathType { - LITERAL, - GLOB, - REGEX - } - public enum Operations { /** Can submit pushes for review. */ PUSH, diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermissionService.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermissionService.java index 85bad823..779436b7 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermissionService.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/permission/RepoPermissionService.java @@ -9,6 +9,7 @@ import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; import lombok.extern.slf4j.Slf4j; +import org.finos.gitproxy.db.model.MatchType; /** * Service that evaluates whether a proxy user is authorised to push to or approve a push for a given repository. @@ -22,7 +23,7 @@ *

Path matching

* *

Paths use the {@code /owner/repo} convention (leading slash, no {@code .git} suffix). Matching is controlled by - * {@link RepoPermission.PathType}: + * {@link MatchType}: * *

    *
  • {@code LITERAL} — exact string equality @@ -58,9 +59,10 @@ public boolean isAllowedToReview(String username, String provider, String path) } /** - * Returns {@code true} when {@code username} has an explicit {@link RepoPermission.Operations#BYPASS_REVIEW} grant - * for {@code path} at {@code provider}. Unlike push/approve checks, {@link RepoPermission.Operations#ALL} does - * not imply bypass — the grant must be explicit. + * Returns {@code true} when {@code username} has an explicit {@link RepoPermission.Operations#SELF_CERTIFY} grant + * for {@code path} at {@code provider}. Unlike push/approve checks, + * {@link RepoPermission.Operations#PUSH_AND_REVIEW} does not imply self-certify — the grant must be + * explicit. */ public boolean isBypassReviewAllowed(String username, String provider, String path) { List forProvider = store.findByProvider(provider); @@ -143,13 +145,15 @@ public void seedFromConfig(List permissions) { if (conflict.isPresent()) { RepoPermission c = conflict.get(); throw new IllegalStateException(String.format( - "Conflicting permission for user '%s' provider '%s': path '%s' (%s) overlaps with '%s' (%s) [%s] — fix config and restart", + "Conflicting permission for user '%s' provider '%s': value '%s' (%s/%s) overlaps with '%s' (%s/%s) [%s] — fix config and restart", p.getUsername(), p.getProvider(), - p.getPath(), - p.getPathType(), - c.getPath(), - c.getPathType(), + p.getValue(), + p.getTarget(), + p.getMatchType(), + c.getValue(), + c.getTarget(), + c.getMatchType(), c.getSource())); } store.save(p); @@ -175,9 +179,9 @@ private boolean operationsOverlap(RepoPermission.Operations a, RepoPermission.Op } private boolean pathsOverlap(RepoPermission a, RepoPermission b) { - if (a.getPath().equals(b.getPath())) return true; - if (matchesPath(a, b.getPath())) return true; - if (matchesPath(b, a.getPath())) return true; + if (a.getValue().equals(b.getValue())) return true; + if (matchesPath(a, b.getValue())) return true; + if (matchesPath(b, a.getValue())) return true; return false; } @@ -207,10 +211,10 @@ private boolean isAllowed(String username, String provider, String path, RepoPer } private boolean matchesPath(RepoPermission perm, String path) { - return switch (perm.getPathType()) { - case LITERAL -> perm.getPath().equals(path); - case GLOB -> matchesGlob(perm.getPath(), path); - case REGEX -> matchesRegex(perm.getPath(), path); + return switch (perm.getMatchType()) { + case LITERAL -> perm.getValue().equals(path); + case GLOB -> matchesGlob(perm.getValue(), path); + case REGEX -> matchesRegex(perm.getValue(), path); }; } diff --git a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluator.java b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluator.java index 35055415..ef188208 100644 --- a/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluator.java +++ b/git-proxy-java-core/src/main/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluator.java @@ -10,6 +10,7 @@ import lombok.extern.slf4j.Slf4j; import org.finos.gitproxy.db.UrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.git.HttpOperation; import org.finos.gitproxy.provider.GitProxyProvider; @@ -96,42 +97,37 @@ static boolean operationMatches(AccessRule rule, HttpOperation operation) { }; } - /** - * Returns {@code true} if the given {@link AccessRule} matches the repository reference. - * - *

    For slug rules: literal and glob comparisons strip leading {@code /} from both pattern and value so that - * {@code /owner/repo} and {@code owner/repo} are treated identically. Regex patterns receive the raw slug (with - * leading {@code /} if present) so users can write them relative to the full path form (e.g. - * {@code regex:/myorg/.*}). - */ + /** Returns {@code true} if the given {@link AccessRule} matches the repository reference. */ static boolean matchesRepo(AccessRule rule, String slug, String owner, String name) { - if (rule.getSlug() != null) return matchPattern(rule.getSlug(), slug); - if (rule.getOwner() != null) return matchPattern(rule.getOwner(), owner); - if (rule.getName() != null) return matchPattern(rule.getName(), name); - return false; + String candidate = + switch (rule.getTarget()) { + case SLUG -> slug; + case OWNER -> owner; + case NAME -> name; + }; + return matchPattern(rule.getValue(), rule.getMatchType(), candidate); } /** - * Matches a pattern string against a value. Regex patterns (prefixed with {@code regex:}) are matched against the - * raw value as-is. Literal and glob patterns normalise leading {@code /} from both sides before comparison. + * Matches a pattern string against a value using the specified {@link MatchType}. LITERAL and GLOB normalise + * leading {@code /} before comparison; REGEX receives the raw value as-is. */ - static boolean matchPattern(String pattern, String value) { + static boolean matchPattern(String pattern, MatchType matchType, String value) { if (pattern == null || value == null) return false; - if (pattern.startsWith("regex:")) { - return REGEX_CACHE - .computeIfAbsent(pattern.substring(6), Pattern::compile) - .matcher(value) - .matches(); - } - // Literal / glob: normalise leading slashes so /owner/repo and owner/repo are equivalent - String p = strip(pattern); - String v = strip(value); - if (p.equals(v)) return true; - if (p.contains("*") || p.contains("?") || p.contains("[")) { - PathMatcher matcher = FileSystems.getDefault().getPathMatcher("glob:" + p); - return matcher.matches(Paths.get(v)); - } - return false; + return switch (matchType) { + case REGEX -> + REGEX_CACHE + .computeIfAbsent(pattern, Pattern::compile) + .matcher(value) + .matches(); + case GLOB -> { + String p = strip(pattern); + String v = strip(value); + PathMatcher matcher = FileSystems.getDefault().getPathMatcher("glob:" + p); + yield matcher.matches(Paths.get(v)); + } + case LITERAL -> strip(pattern).equals(strip(value)); + }; } private static String strip(String s) { diff --git a/git-proxy-java-core/src/main/resources/db/migration/V5__unified_rule_shape.sql b/git-proxy-java-core/src/main/resources/db/migration/V5__unified_rule_shape.sql new file mode 100644 index 00000000..82a1fdc6 --- /dev/null +++ b/git-proxy-java-core/src/main/resources/db/migration/V5__unified_rule_shape.sql @@ -0,0 +1,55 @@ +-- Unify access_rules and repo_permissions onto a common (target, match_value, match_type) shape. +-- Closes #221. +-- +-- access_rules: replaces slug/owner/name columns with target + match_value + match_type. +-- Data migration: whichever of slug/owner/name is non-null becomes the new target+match_value. +-- Match type: values prefixed with 'regex:' become REGEX (prefix stripped); all others become GLOB +-- (the previous implicit behaviour — glob syntax was always the default for non-regex patterns). +-- +-- repo_permissions: renames path → match_value, path_type → match_type; adds target column (default SLUG). + +-- --------------------------------------------------------------------------- +-- access_rules +-- --------------------------------------------------------------------------- + +ALTER TABLE access_rules ADD COLUMN target VARCHAR(10); +ALTER TABLE access_rules ADD COLUMN match_value VARCHAR(512); +ALTER TABLE access_rules ADD COLUMN match_type VARCHAR(10); + +-- Migrate slug rows +UPDATE access_rules SET target = 'SLUG', match_value = slug WHERE slug IS NOT NULL; +UPDATE access_rules SET target = 'OWNER', match_value = owner WHERE owner IS NOT NULL; +UPDATE access_rules SET target = 'NAME', match_value = name WHERE name IS NOT NULL; + +-- Rows with a 'regex:' prefix become REGEX; strip the prefix from match_value +UPDATE access_rules + SET match_type = 'REGEX', match_value = SUBSTRING(match_value, 8) + WHERE match_value LIKE 'regex:%'; + +-- Everything else was implicitly glob +UPDATE access_rules SET match_type = 'GLOB' WHERE match_type IS NULL; + +-- Apply NOT NULL after data migration +ALTER TABLE access_rules ALTER COLUMN target SET NOT NULL; +ALTER TABLE access_rules ALTER COLUMN match_value SET NOT NULL; +ALTER TABLE access_rules ALTER COLUMN match_type SET NOT NULL; + +ALTER TABLE access_rules DROP COLUMN slug; +ALTER TABLE access_rules DROP COLUMN owner; +ALTER TABLE access_rules DROP COLUMN name; + +-- --------------------------------------------------------------------------- +-- repo_permissions +-- --------------------------------------------------------------------------- + +ALTER TABLE repo_permissions ADD COLUMN target VARCHAR(10) NOT NULL DEFAULT 'SLUG'; +ALTER TABLE repo_permissions ADD COLUMN match_value VARCHAR(512); +ALTER TABLE repo_permissions ADD COLUMN match_type VARCHAR(10); + +UPDATE repo_permissions SET match_value = path, match_type = path_type; + +ALTER TABLE repo_permissions ALTER COLUMN match_value SET NOT NULL; +ALTER TABLE repo_permissions ALTER COLUMN match_type SET NOT NULL; + +ALTER TABLE repo_permissions DROP COLUMN path; +ALTER TABLE repo_permissions DROP COLUMN path_type; diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/jdbc/mapper/RowMapperTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/jdbc/mapper/RowMapperTest.java index 08e753bd..ef93621c 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/jdbc/mapper/RowMapperTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/jdbc/mapper/RowMapperTest.java @@ -9,6 +9,8 @@ import java.time.Instant; import org.finos.gitproxy.db.model.AccessRule; import org.finos.gitproxy.db.model.FetchRecord; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.junit.jupiter.api.Test; class RowMapperTest { @@ -20,9 +22,9 @@ void accessRule_allFields_mapped() throws SQLException { ResultSet rs = mock(ResultSet.class); when(rs.getString("id")).thenReturn("rule-1"); when(rs.getString("provider")).thenReturn("github"); - when(rs.getString("slug")).thenReturn("/org/*"); - when(rs.getString("owner")).thenReturn("org"); - when(rs.getString("name")).thenReturn("*"); + when(rs.getString("target")).thenReturn("OWNER"); + when(rs.getString("match_value")).thenReturn("org"); + when(rs.getString("match_type")).thenReturn("GLOB"); when(rs.getString("access")).thenReturn("DENY"); when(rs.getString("operations")).thenReturn("PUSH"); when(rs.getString("description")).thenReturn("Block all pushes"); @@ -34,9 +36,9 @@ void accessRule_allFields_mapped() throws SQLException { assertEquals("rule-1", rule.getId()); assertEquals("github", rule.getProvider()); - assertEquals("/org/*", rule.getSlug()); - assertEquals("org", rule.getOwner()); - assertEquals("*", rule.getName()); + assertEquals(MatchTarget.OWNER, rule.getTarget()); + assertEquals("org", rule.getValue()); + assertEquals(MatchType.GLOB, rule.getMatchType()); assertEquals(AccessRule.Access.DENY, rule.getAccess()); assertEquals(AccessRule.Operations.PUSH, rule.getOperations()); assertEquals("Block all pushes", rule.getDescription()); @@ -50,9 +52,9 @@ void accessRule_nullableFields_null() throws SQLException { ResultSet rs = mock(ResultSet.class); when(rs.getString("id")).thenReturn("rule-2"); when(rs.getString("provider")).thenReturn(null); - when(rs.getString("slug")).thenReturn(null); - when(rs.getString("owner")).thenReturn(null); - when(rs.getString("name")).thenReturn(null); + when(rs.getString("target")).thenReturn("SLUG"); + when(rs.getString("match_value")).thenReturn("/org/**"); + when(rs.getString("match_type")).thenReturn("GLOB"); when(rs.getString("access")).thenReturn("ALLOW"); when(rs.getString("operations")).thenReturn("BOTH"); when(rs.getString("description")).thenReturn(null); @@ -63,9 +65,9 @@ void accessRule_nullableFields_null() throws SQLException { AccessRule rule = AccessRuleRowMapper.INSTANCE.mapRow(rs, 1); assertNull(rule.getProvider()); - assertNull(rule.getSlug()); - assertNull(rule.getOwner()); - assertNull(rule.getName()); + assertEquals(MatchTarget.SLUG, rule.getTarget()); + assertEquals("/org/**", rule.getValue()); + assertEquals(MatchType.GLOB, rule.getMatchType()); assertNull(rule.getDescription()); assertEquals(AccessRule.Access.ALLOW, rule.getAccess()); assertEquals(AccessRule.Operations.BOTH, rule.getOperations()); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/model/DbModelTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/model/DbModelTest.java index c5ea17b8..be3d593b 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/model/DbModelTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/model/DbModelTest.java @@ -24,9 +24,9 @@ void accessRule_defaults() { assertEquals(100, rule.getRuleOrder()); assertEquals(AccessRule.Source.DB, rule.getSource()); assertNull(rule.getProvider()); - assertNull(rule.getSlug()); - assertNull(rule.getOwner()); - assertNull(rule.getName()); + assertEquals(MatchTarget.SLUG, rule.getTarget()); + assertNull(rule.getValue()); + assertEquals(MatchType.GLOB, rule.getMatchType()); assertNull(rule.getDescription()); } @@ -35,9 +35,9 @@ void accessRule_builderOverridesDefaults() { AccessRule rule = AccessRule.builder() .id("fixed-id") .provider("github") - .slug("/org/*") - .owner("org") - .name("repo") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.LITERAL) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.FETCH) .description("test rule") @@ -48,9 +48,9 @@ void accessRule_builderOverridesDefaults() { assertEquals("fixed-id", rule.getId()); assertEquals("github", rule.getProvider()); - assertEquals("/org/*", rule.getSlug()); - assertEquals("org", rule.getOwner()); - assertEquals("repo", rule.getName()); + assertEquals(MatchTarget.OWNER, rule.getTarget()); + assertEquals("myorg", rule.getValue()); + assertEquals(MatchType.LITERAL, rule.getMatchType()); assertEquals(AccessRule.Access.DENY, rule.getAccess()); assertEquals(AccessRule.Operations.FETCH, rule.getOperations()); assertEquals("test rule", rule.getDescription()); @@ -78,11 +78,24 @@ void accessRule_allSourceEnumValues() { assertNotNull(AccessRule.Source.valueOf("DB")); } + @Test + void matchTarget_allValues() { + assertNotNull(MatchTarget.valueOf("SLUG")); + assertNotNull(MatchTarget.valueOf("OWNER")); + assertNotNull(MatchTarget.valueOf("NAME")); + } + + @Test + void matchType_allValues() { + assertNotNull(MatchType.valueOf("LITERAL")); + assertNotNull(MatchType.valueOf("GLOB")); + assertNotNull(MatchType.valueOf("REGEX")); + } + @Test void accessRule_equalsAndHashCode() { AccessRule a = AccessRule.builder().id("id-1").build(); AccessRule b = AccessRule.builder().id("id-1").build(); - // Lombok @Data generates equals/hashCode on all fields; same id + same defaults are equal assertEquals(a, b); assertEquals(a.hashCode(), b.hashCode()); } diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistryIntegrationTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistryIntegrationTest.java index 0dd26770..8a1fcdaf 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistryIntegrationTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/db/mongo/MongoUrlRuleRegistryIntegrationTest.java @@ -6,6 +6,8 @@ import java.util.List; import java.util.UUID; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.junit.jupiter.api.*; import org.testcontainers.containers.MongoDBContainer; import org.testcontainers.junit.jupiter.Container; @@ -29,11 +31,12 @@ void setUp() { registry.initialize(); } - private AccessRule rule(String provider, String owner, String name) { + private AccessRule rule(String provider, String value) { return AccessRule.builder() .provider(provider) - .owner(owner) - .name(name) + .target(MatchTarget.OWNER) + .value(value) + .matchType(MatchType.GLOB) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) .enabled(true) @@ -44,9 +47,9 @@ private AccessRule rule(String provider, String owner, String name) { void save_andFindById_roundTripsAllFields() { AccessRule r = AccessRule.builder() .provider("github") - .slug("org/repo") - .owner("org") - .name("repo") + .target(MatchTarget.SLUG) + .value("org/repo") + .matchType(MatchType.LITERAL) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.PUSH) .description("block pushes") @@ -59,9 +62,9 @@ void save_andFindById_roundTripsAllFields() { AccessRule found = registry.findById(r.getId()).orElseThrow(); assertEquals(r.getId(), found.getId()); assertEquals("github", found.getProvider()); - assertEquals("org/repo", found.getSlug()); - assertEquals("org", found.getOwner()); - assertEquals("repo", found.getName()); + assertEquals(MatchTarget.SLUG, found.getTarget()); + assertEquals("org/repo", found.getValue()); + assertEquals(MatchType.LITERAL, found.getMatchType()); assertEquals(AccessRule.Access.DENY, found.getAccess()); assertEquals(AccessRule.Operations.PUSH, found.getOperations()); assertEquals("block pushes", found.getDescription()); @@ -79,16 +82,18 @@ void findById_unknownId_returnsEmpty() { void findAll_returnsSortedByRuleOrderThenId() { AccessRule high = AccessRule.builder() .id("aaa") - .owner("org") - .name("repo") + .target(MatchTarget.OWNER) + .value("org") + .matchType(MatchType.GLOB) .ruleOrder(10) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) .build(); AccessRule low = AccessRule.builder() .id("zzz") - .owner("org") - .name("repo2") + .target(MatchTarget.OWNER) + .value("org") + .matchType(MatchType.GLOB) .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) @@ -104,14 +109,15 @@ void findAll_returnsSortedByRuleOrderThenId() { @Test void update_replacesExistingDocument() { - AccessRule r = rule("github", "org", "repo"); + AccessRule r = rule("github", "org"); registry.save(r); AccessRule updated = AccessRule.builder() .id(r.getId()) .provider("gitlab") - .owner("org") - .name("repo") + .target(MatchTarget.OWNER) + .value("org") + .matchType(MatchType.GLOB) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.FETCH) .enabled(false) @@ -126,7 +132,7 @@ void update_replacesExistingDocument() { @Test void delete_removesDocument() { - AccessRule r = rule("github", "org", "repo"); + AccessRule r = rule("github", "org"); registry.save(r); registry.delete(r.getId()); assertTrue(registry.findById(r.getId()).isEmpty()); @@ -134,19 +140,21 @@ void delete_removesDocument() { @Test void findEnabledForProvider_returnsMatchingAndNullProviderRules() { - AccessRule githubRule = rule("github", "org", "gh-repo"); - AccessRule gitlabRule = rule("gitlab", "org", "gl-repo"); + AccessRule githubRule = rule("github", "gh-org"); + AccessRule gitlabRule = rule("gitlab", "gl-org"); AccessRule globalRule = AccessRule.builder() - .owner("org") - .name("global-repo") + .target(MatchTarget.OWNER) + .value("global-org") + .matchType(MatchType.GLOB) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) .enabled(true) .build(); AccessRule disabledRule = AccessRule.builder() .provider("github") - .owner("org") - .name("disabled-repo") + .target(MatchTarget.OWNER) + .value("disabled-org") + .matchType(MatchType.GLOB) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) .enabled(false) @@ -157,11 +165,10 @@ void findEnabledForProvider_returnsMatchingAndNullProviderRules() { registry.save(disabledRule); List results = registry.findEnabledForProvider("github"); - // Should include github-specific and global (null provider), but not gitlab or disabled - assertTrue(results.stream().anyMatch(r -> "gh-repo".equals(r.getName()))); - assertTrue(results.stream().anyMatch(r -> "global-repo".equals(r.getName()))); - assertTrue(results.stream().noneMatch(r -> "gl-repo".equals(r.getName()))); - assertTrue(results.stream().noneMatch(r -> "disabled-repo".equals(r.getName()))); + assertTrue(results.stream().anyMatch(r -> "gh-org".equals(r.getValue()))); + assertTrue(results.stream().anyMatch(r -> "global-org".equals(r.getValue()))); + assertTrue(results.stream().noneMatch(r -> "gl-org".equals(r.getValue()))); + assertTrue(results.stream().noneMatch(r -> "disabled-org".equals(r.getValue()))); } @Test diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/RepositoryUrlRuleHookTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/RepositoryUrlRuleHookTest.java index 71109b4d..e31860ae 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/RepositoryUrlRuleHookTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/git/RepositoryUrlRuleHookTest.java @@ -11,6 +11,8 @@ import org.eclipse.jgit.transport.ReceivePack; import org.finos.gitproxy.db.memory.InMemoryUrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.db.model.StepStatus; import org.finos.gitproxy.provider.GitHubProvider; import org.finos.gitproxy.provider.GitProxyProvider; @@ -39,10 +41,15 @@ private ReceiveCommand makeCmd() { return new ReceiveCommand(ObjectId.zeroId(), ObjectId.zeroId(), "refs/heads/main"); } - private RepositoryUrlRuleHook hookWith(AccessRule... rules) throws Exception { - var registry = new InMemoryUrlRuleRegistry(); - for (AccessRule r : rules) registry.save(r); - return new RepositoryUrlRuleHook(registry, GITHUB, null, new PushContext()); + private static AccessRule ownerRule(AccessRule.Access access, AccessRule.Operations ops, String owner, int order) { + return AccessRule.builder() + .ruleOrder(order) + .access(access) + .operations(ops) + .target(MatchTarget.OWNER) + .value(owner) + .matchType(MatchType.GLOB) + .build(); } @Test @@ -61,12 +68,7 @@ void noRepoSlug_blocksWithFailClosed() { @Test void withRepoSlug_allowRuleMatches_recordsPass() throws Exception { - var allowRule = AccessRule.builder() - .ruleOrder(100) - .access(AccessRule.Access.ALLOW) - .operations(AccessRule.Operations.BOTH) - .owner("myorg") - .build(); + var allowRule = ownerRule(AccessRule.Access.ALLOW, AccessRule.Operations.BOTH, "myorg", 100); var pushContext = new PushContext(); pushContext.setRepoSlug("/myorg/myrepo"); var registry = new InMemoryUrlRuleRegistry(); @@ -83,12 +85,7 @@ void withRepoSlug_allowRuleMatches_recordsPass() throws Exception { @Test void withRepoSlug_noMatchingAllowRule_rejectsCommand() throws Exception { - var allowRule = AccessRule.builder() - .ruleOrder(100) - .access(AccessRule.Access.ALLOW) - .operations(AccessRule.Operations.BOTH) - .owner("other-org") - .build(); + var allowRule = ownerRule(AccessRule.Access.ALLOW, AccessRule.Operations.BOTH, "other-org", 100); var pushContext = new PushContext(); pushContext.setRepoSlug("/myorg/myrepo"); var registry = new InMemoryUrlRuleRegistry(); @@ -105,18 +102,8 @@ void withRepoSlug_noMatchingAllowRule_rejectsCommand() throws Exception { @Test void withRepoSlug_denyRuleAtLowerOrder_rejectsEvenWithAllowRule() throws Exception { - var denyRule = AccessRule.builder() - .ruleOrder(100) - .access(AccessRule.Access.DENY) - .operations(AccessRule.Operations.BOTH) - .owner("myorg") - .build(); - var allowRule = AccessRule.builder() - .ruleOrder(200) - .access(AccessRule.Access.ALLOW) - .operations(AccessRule.Operations.BOTH) - .owner("myorg") - .build(); + var denyRule = ownerRule(AccessRule.Access.DENY, AccessRule.Operations.BOTH, "myorg", 100); + var allowRule = ownerRule(AccessRule.Access.ALLOW, AccessRule.Operations.BOTH, "myorg", 200); var pushContext = new PushContext(); pushContext.setRepoSlug("/myorg/myrepo"); var registry = new InMemoryUrlRuleRegistry(); @@ -134,12 +121,7 @@ void withRepoSlug_denyRuleAtLowerOrder_rejectsEvenWithAllowRule() throws Excepti @Test void fetchOnlyAllowRule_doesNotEngageForPush() throws Exception { - var fetchOnlyAllow = AccessRule.builder() - .ruleOrder(100) - .access(AccessRule.Access.ALLOW) - .operations(AccessRule.Operations.FETCH) - .owner("myorg") - .build(); + var fetchOnlyAllow = ownerRule(AccessRule.Access.ALLOW, AccessRule.Operations.FETCH, "myorg", 100); var pushContext = new PushContext(); pushContext.setRepoSlug("/myorg/myrepo"); var registry = new InMemoryUrlRuleRegistry(); @@ -156,18 +138,8 @@ void fetchOnlyAllowRule_doesNotEngageForPush() throws Exception { @Test void fetchOnlyDenyRule_doesNotBlockPush() throws Exception { - var fetchDeny = AccessRule.builder() - .ruleOrder(100) - .access(AccessRule.Access.DENY) - .operations(AccessRule.Operations.FETCH) - .owner("myorg") - .build(); - var pushAllow = AccessRule.builder() - .ruleOrder(200) - .access(AccessRule.Access.ALLOW) - .operations(AccessRule.Operations.BOTH) - .owner("myorg") - .build(); + var fetchDeny = ownerRule(AccessRule.Access.DENY, AccessRule.Operations.FETCH, "myorg", 100); + var pushAllow = ownerRule(AccessRule.Access.ALLOW, AccessRule.Operations.BOTH, "myorg", 200); var pushContext = new PushContext(); pushContext.setRepoSlug("/myorg/myrepo"); var registry = new InMemoryUrlRuleRegistry(); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/JdbcRepoPermissionStoreIntegrationTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/JdbcRepoPermissionStoreIntegrationTest.java index e725e072..c823e37b 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/JdbcRepoPermissionStoreIntegrationTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/JdbcRepoPermissionStoreIntegrationTest.java @@ -7,6 +7,8 @@ import javax.sql.DataSource; import org.finos.gitproxy.db.jdbc.DataSourceFactory; import org.finos.gitproxy.db.jdbc.DatabaseMigrator; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -26,12 +28,13 @@ void setUp() { store = new JdbcRepoPermissionStore(ds); } - private RepoPermission perm(String username, String provider, String path) { + private RepoPermission perm(String username, String provider, String value) { return RepoPermission.builder() .username(username) .provider(provider) - .path(path) - .pathType(RepoPermission.PathType.LITERAL) + .target(MatchTarget.SLUG) + .value(value) + .matchType(MatchType.LITERAL) .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.DB) .build(); @@ -48,8 +51,9 @@ void save_findById_roundTrip() { assertTrue(found.isPresent()); assertEquals("alice", found.get().getUsername()); assertEquals("github", found.get().getProvider()); - assertEquals("/owner/repo", found.get().getPath()); - assertEquals(RepoPermission.PathType.LITERAL, found.get().getPathType()); + assertEquals(MatchTarget.SLUG, found.get().getTarget()); + assertEquals("/owner/repo", found.get().getValue()); + assertEquals(MatchType.LITERAL, found.get().getMatchType()); assertEquals(RepoPermission.Operations.PUSH_AND_REVIEW, found.get().getOperations()); assertEquals(RepoPermission.Source.DB, found.get().getSource()); } @@ -107,15 +111,16 @@ void findByProvider_filtersCorrectly() { assertEquals("bob", gitlabPerms.get(0).getUsername()); } - // ---- operations and path_type round-trip ---- + // ---- operations and matchType round-trip ---- @Test void pushOnlyOperation_persistedAndRetrieved() { RepoPermission p = RepoPermission.builder() .username("alice") .provider("github") - .path("/owner/repo") - .pathType(RepoPermission.PathType.GLOB) + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH) .source(RepoPermission.Source.CONFIG) .build(); @@ -123,7 +128,8 @@ void pushOnlyOperation_persistedAndRetrieved() { var found = store.findById(p.getId()).orElseThrow(); assertEquals(RepoPermission.Operations.PUSH, found.getOperations()); - assertEquals(RepoPermission.PathType.GLOB, found.getPathType()); + assertEquals(MatchTarget.OWNER, found.getTarget()); + assertEquals(MatchType.GLOB, found.getMatchType()); assertEquals(RepoPermission.Source.CONFIG, found.getSource()); } } diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/MongoRepoPermissionStoreIntegrationTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/MongoRepoPermissionStoreIntegrationTest.java index 9175814c..8f889f78 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/MongoRepoPermissionStoreIntegrationTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/MongoRepoPermissionStoreIntegrationTest.java @@ -5,6 +5,8 @@ import com.mongodb.client.MongoClients; import java.util.List; import java.util.UUID; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.junit.jupiter.api.*; import org.testcontainers.containers.MongoDBContainer; import org.testcontainers.junit.jupiter.Container; @@ -28,11 +30,13 @@ void setUp() { store.initialize(); } - private RepoPermission perm(String username, String provider, String path) { + private RepoPermission perm(String username, String provider, String value) { return RepoPermission.builder() .username(username) .provider(provider) - .path(path) + .target(MatchTarget.SLUG) + .value(value) + .matchType(MatchType.LITERAL) .build(); } @@ -41,8 +45,9 @@ void save_andFindById_roundTripsAllFields() { RepoPermission p = RepoPermission.builder() .username("alice") .provider("github") - .path("/org/repo") - .pathType(RepoPermission.PathType.GLOB) + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH) .source(RepoPermission.Source.CONFIG) .build(); @@ -52,8 +57,9 @@ void save_andFindById_roundTripsAllFields() { assertEquals(p.getId(), found.getId()); assertEquals("alice", found.getUsername()); assertEquals("github", found.getProvider()); - assertEquals("/org/repo", found.getPath()); - assertEquals(RepoPermission.PathType.GLOB, found.getPathType()); + assertEquals(MatchTarget.OWNER, found.getTarget()); + assertEquals("myorg", found.getValue()); + assertEquals(MatchType.GLOB, found.getMatchType()); assertEquals(RepoPermission.Operations.PUSH, found.getOperations()); assertEquals(RepoPermission.Source.CONFIG, found.getSource()); } @@ -64,14 +70,13 @@ void findById_unknownId_returnsEmpty() { } @Test - void findAll_returnsSortedByProviderPathUsername() { + void findAll_returnsSortedByProviderValueUsername() { store.save(perm("bob", "github", "/org/z-repo")); store.save(perm("alice", "github", "/org/a-repo")); store.save(perm("carol", "gitlab", "/org/repo")); List all = store.findAll(); assertEquals(3, all.size()); - // sorted by provider then path then username assertEquals("alice", all.get(0).getUsername()); assertEquals("bob", all.get(1).getUsername()); assertEquals("carol", all.get(2).getUsername()); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/RepoPermissionServiceTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/RepoPermissionServiceTest.java index 36ffe023..85913fea 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/RepoPermissionServiceTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/permission/RepoPermissionServiceTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.*; import java.util.List; +import org.finos.gitproxy.db.model.MatchType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -21,28 +22,24 @@ void setUp() { svc = new RepoPermissionService(new InMemoryRepoPermissionStore()); } - private RepoPermission grant(String username, String provider, String path) { + private RepoPermission grant(String username, String provider, String value) { return RepoPermission.builder() .username(username) .provider(provider) - .path(path) - .pathType(RepoPermission.PathType.LITERAL) + .value(value) + .matchType(MatchType.LITERAL) .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.DB) .build(); } private RepoPermission grant( - String username, - String provider, - String path, - RepoPermission.PathType pathType, - RepoPermission.Operations ops) { + String username, String provider, String value, MatchType matchType, RepoPermission.Operations ops) { return RepoPermission.builder() .username(username) .provider(provider) - .path(path) - .pathType(pathType) + .value(value) + .matchType(matchType) .operations(ops) .source(RepoPermission.Source.DB) .build(); @@ -87,28 +84,21 @@ void pathExistsButNoUserMatch_denied() { @Test void pushOnlyGrant_allowsPush_deniesApprove() { - svc.save(grant( - "alice", "github", "/owner/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/owner/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/owner/repo")); assertFalse(svc.isAllowedToReview("alice", "github", "/owner/repo")); } @Test void approveOnlyGrant_allowsApprove_deniesPush() { - svc.save(grant( - "alice", "github", "/owner/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.REVIEW)); + svc.save(grant("alice", "github", "/owner/repo", MatchType.LITERAL, RepoPermission.Operations.REVIEW)); assertFalse(svc.isAllowedToPush("alice", "github", "/owner/repo")); assertTrue(svc.isAllowedToReview("alice", "github", "/owner/repo")); } @Test void allGrant_allowsBothOperations() { - svc.save(grant( - "alice", - "github", - "/owner/repo", - RepoPermission.PathType.LITERAL, - RepoPermission.Operations.PUSH_AND_REVIEW)); + svc.save(grant("alice", "github", "/owner/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH_AND_REVIEW)); assertTrue(svc.isAllowedToPush("alice", "github", "/owner/repo")); assertTrue(svc.isAllowedToReview("alice", "github", "/owner/repo")); } @@ -125,31 +115,20 @@ void grantForDifferentProvider_doesNotAllow() { @Test void globGrant_matchesAllReposUnderOwner_allowed() { - svc.save(grant( - "alice", - "github", - "/owner/*", - RepoPermission.PathType.GLOB, - RepoPermission.Operations.PUSH_AND_REVIEW)); + svc.save(grant("alice", "github", "/owner/*", MatchType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW)); assertTrue(svc.isAllowedToPush("alice", "github", "/owner/repo-a")); assertTrue(svc.isAllowedToPush("alice", "github", "/owner/repo-b")); } @Test void globGrant_doesNotMatchOtherOwner() { - svc.save(grant( - "alice", - "github", - "/owner/*", - RepoPermission.PathType.GLOB, - RepoPermission.Operations.PUSH_AND_REVIEW)); + svc.save(grant("alice", "github", "/owner/*", MatchType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW)); assertFalse(svc.isAllowedToPush("alice", "github", "/other/repo")); } @Test void globGrant_doubleWildcard_matchesAnyPath() { - svc.save(grant( - "alice", "github", "/**", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW)); + svc.save(grant("alice", "github", "/**", MatchType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW)); assertTrue(svc.isAllowedToPush("alice", "github", "/owner/repo")); assertTrue(svc.isAllowedToPush("alice", "github", "/other/thing")); } @@ -165,33 +144,33 @@ void globGrant_doubleWildcard_matchesAnyPath() { @Test void glob_singleStar_matchesRepoName() { - svc.save(grant("alice", "github", "/acme/*", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/*", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo")); } @Test void glob_singleStar_matchesHyphenatedName() { - svc.save(grant("alice", "github", "/acme/*", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/*", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/my-service")); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo-v2")); } @Test void glob_singleStar_doesNotCrossPathSeparator() { - svc.save(grant("alice", "github", "/acme/*", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/*", MatchType.GLOB, RepoPermission.Operations.PUSH)); // /acme/sub/repo has two segments after /acme — single * does not match assertFalse(svc.isAllowedToPush("alice", "github", "/acme/sub/repo")); } @Test void glob_singleStar_doesNotMatchOtherOwner() { - svc.save(grant("alice", "github", "/acme/*", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/*", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertFalse(svc.isAllowedToPush("alice", "github", "/other/repo")); } @Test void glob_doubleStar_matchesAcrossSegments() { - svc.save(grant("alice", "github", "/acme/**", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/**", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo")); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/sub/repo")); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/a/b/c")); @@ -199,20 +178,20 @@ void glob_doubleStar_matchesAcrossSegments() { @Test void glob_doubleStar_doesNotMatchOtherOwner() { - svc.save(grant("alice", "github", "/acme/**", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/**", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertFalse(svc.isAllowedToPush("alice", "github", "/other/repo")); } @Test void glob_leadingDoubleStar_matchesAllPaths() { - svc.save(grant("alice", "github", "/**", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/**", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo")); assertTrue(svc.isAllowedToPush("alice", "github", "/other/thing")); } @Test void glob_wildcardOwner_matchesSpecificRepo() { - svc.save(grant("alice", "github", "/*/repo", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/*/repo", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo")); assertTrue(svc.isAllowedToPush("alice", "github", "/other/repo")); assertFalse(svc.isAllowedToPush("alice", "github", "/acme/other-repo")); @@ -220,8 +199,7 @@ void glob_wildcardOwner_matchesSpecificRepo() { @Test void glob_prefixSuffix_matchesNames() { - svc.save(grant( - "alice", "github", "/acme/service-*", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/service-*", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/service-api")); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/service-worker")); assertFalse(svc.isAllowedToPush("alice", "github", "/acme/repo")); @@ -230,8 +208,7 @@ void glob_prefixSuffix_matchesNames() { @Test void glob_questionMark_matchesSingleChar() { - svc.save( - grant("alice", "github", "/acme/repo-?", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/repo-?", MatchType.GLOB, RepoPermission.Operations.PUSH)); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo-1")); assertTrue(svc.isAllowedToPush("alice", "github", "/acme/repo-a")); assertFalse(svc.isAllowedToPush("alice", "github", "/acme/repo-12")); @@ -246,7 +223,7 @@ void regexGrant_matchesPattern() { "alice", "github", "/coopernetes/test-repo-.*", - RepoPermission.PathType.REGEX, + MatchType.REGEX, RepoPermission.Operations.PUSH_AND_REVIEW)); assertTrue(svc.isAllowedToPush("alice", "github", "/coopernetes/test-repo-codeberg")); assertTrue(svc.isAllowedToPush("alice", "github", "/coopernetes/test-repo-gitlab")); @@ -256,25 +233,13 @@ void regexGrant_matchesPattern() { @Test void regexGrant_invalidPattern_treatedAsNoMatch() { - svc.save(grant( - "alice", - "github", - "[invalid", - RepoPermission.PathType.REGEX, - RepoPermission.Operations.PUSH_AND_REVIEW)); + svc.save(grant("alice", "github", "[invalid", MatchType.REGEX, RepoPermission.Operations.PUSH_AND_REVIEW)); assertFalse(svc.isAllowedToPush("alice", "github", "/owner/repo")); } @Test void regexGrant_patternCompiledOnce_multipleEvaluations() { - // Verifies that repeated evaluation of the same regex pattern does not throw and returns - // consistent results — a proxy for the cache being exercised rather than recompiling. - svc.save(grant( - "alice", - "github", - "/org/repo-.*", - RepoPermission.PathType.REGEX, - RepoPermission.Operations.PUSH_AND_REVIEW)); + svc.save(grant("alice", "github", "/org/repo-.*", MatchType.REGEX, RepoPermission.Operations.PUSH_AND_REVIEW)); for (int i = 0; i < 10; i++) { assertTrue(svc.isAllowedToPush("alice", "github", "/org/repo-" + i)); assertFalse(svc.isAllowedToPush("alice", "github", "/org/other")); @@ -285,31 +250,27 @@ void regexGrant_patternCompiledOnce_multipleEvaluations() { @Test void seedFromConfig_replacesConfigRows_keepsDbRows() { - // DB-sourced row — should survive reseed RepoPermission dbRow = RepoPermission.builder() .username("bob") .provider("github") - .path("/owner/repo") + .value("/owner/repo") .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.DB) .build(); svc.save(dbRow); - // Seed with a config row for alice RepoPermission configRow = RepoPermission.builder() .username("alice") .provider("github") - .path("/owner/repo") + .value("/owner/repo") .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.CONFIG) .build(); svc.seedFromConfig(List.of(configRow)); - // Bob (DB) still allowed; alice (CONFIG) also allowed assertTrue(svc.isAllowedToPush("bob", "github", "/owner/repo")); assertTrue(svc.isAllowedToPush("alice", "github", "/owner/repo")); - // Re-seed without alice — alice should be removed, bob stays svc.seedFromConfig(List.of()); assertTrue(svc.isAllowedToPush("bob", "github", "/owner/repo")); assertFalse(svc.isAllowedToPush("alice", "github", "/owner/repo")); @@ -321,66 +282,39 @@ void seedFromConfig_replacesConfigRows_keepsDbRows() { void findConflict_exactDuplicatePath_sameOps_detected() { svc.save(grant("alice", "github", "/acme/repo")); RepoPermission incoming = - grant("alice", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH); + grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH); assertTrue(svc.findConflict(incoming).isPresent()); } @Test void findConflict_pushVsPushAndReview_detected() { - svc.save(grant( - "alice", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH)); - RepoPermission incoming = grant( - "alice", - "github", - "/acme/repo", - RepoPermission.PathType.LITERAL, - RepoPermission.Operations.PUSH_AND_REVIEW); + svc.save(grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH)); + RepoPermission incoming = + grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH_AND_REVIEW); assertTrue(svc.findConflict(incoming).isPresent()); } @Test void findConflict_pushAndReviewVsSelfCertify_noConflict() { - // Trusted committer pattern: PUSH_AND_REVIEW + SELF_CERTIFY on the same path must coexist. - // They are evaluated by separate code paths (isAllowedToPush vs isBypassReviewAllowed). - svc.save(grant( - "alice", - "github", - "/acme/repo", - RepoPermission.PathType.LITERAL, - RepoPermission.Operations.PUSH_AND_REVIEW)); - RepoPermission incoming = grant( - "alice", - "github", - "/acme/repo", - RepoPermission.PathType.LITERAL, - RepoPermission.Operations.SELF_CERTIFY); + svc.save(grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH_AND_REVIEW)); + RepoPermission incoming = + grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.SELF_CERTIFY); assertTrue(svc.findConflict(incoming).isEmpty()); } @Test void findConflict_selfCertifyVsSelfCertify_detected() { - svc.save(grant( - "alice", - "github", - "/acme/repo", - RepoPermission.PathType.LITERAL, - RepoPermission.Operations.SELF_CERTIFY)); - RepoPermission incoming = grant( - "alice", - "github", - "/acme/repo", - RepoPermission.PathType.LITERAL, - RepoPermission.Operations.SELF_CERTIFY); + svc.save(grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.SELF_CERTIFY)); + RepoPermission incoming = + grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.SELF_CERTIFY); assertTrue(svc.findConflict(incoming).isPresent()); } @Test void findConflict_pushVsReview_noConflict() { - // PUSH and REVIEW affect different permission checks and can coexist. - svc.save(grant( - "alice", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH)); - RepoPermission incoming = grant( - "alice", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.REVIEW); + svc.save(grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH)); + RepoPermission incoming = + grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.REVIEW); assertTrue(svc.findConflict(incoming).isEmpty()); } @@ -388,7 +322,7 @@ void findConflict_pushVsReview_noConflict() { void findConflict_differentUser_noConflict() { svc.save(grant("alice", "github", "/acme/repo")); RepoPermission incoming = - grant("bob", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH); + grant("bob", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH); assertTrue(svc.findConflict(incoming).isEmpty()); } @@ -396,64 +330,58 @@ void findConflict_differentUser_noConflict() { void findConflict_differentProvider_noConflict() { svc.save(grant("alice", "github", "/acme/repo")); RepoPermission incoming = - grant("alice", "gitlab", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH); + grant("alice", "gitlab", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH); assertTrue(svc.findConflict(incoming).isEmpty()); } @Test void findConflict_literalMatchedByExistingGlob_detected() { - svc.save(grant("alice", "github", "/acme/**", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/**", MatchType.GLOB, RepoPermission.Operations.PUSH)); RepoPermission incoming = - grant("alice", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH); + grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH); assertTrue(svc.findConflict(incoming).isPresent()); } @Test void findConflict_incomingGlobMatchesExistingLiteral_detected() { svc.save(grant("alice", "github", "/acme/repo")); - RepoPermission incoming = grant( - "alice", "github", "/acme/**", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW); + RepoPermission incoming = + grant("alice", "github", "/acme/**", MatchType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW); assertTrue(svc.findConflict(incoming).isPresent()); } @Test void findConflict_globOverlap_subsetDetected() { - svc.save(grant( - "alice", - "github", - "/acme/**", - RepoPermission.PathType.GLOB, - RepoPermission.Operations.PUSH_AND_REVIEW)); - RepoPermission incoming = grant( - "alice", "github", "/acme/*", RepoPermission.PathType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW); + svc.save(grant("alice", "github", "/acme/**", MatchType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW)); + RepoPermission incoming = + grant("alice", "github", "/acme/*", MatchType.GLOB, RepoPermission.Operations.PUSH_AND_REVIEW); assertTrue(svc.findConflict(incoming).isPresent()); } @Test void findConflict_noOverlap_noConflict() { svc.save(grant("alice", "github", "/acme/repo-a")); - RepoPermission incoming = grant( - "alice", "github", "/acme/repo-b", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH); + RepoPermission incoming = + grant("alice", "github", "/acme/repo-b", MatchType.LITERAL, RepoPermission.Operations.PUSH); assertTrue(svc.findConflict(incoming).isEmpty()); } @Test void seedFromConfig_conflictingRows_throwsIllegalStateException() { - // Two CONFIG rows for the same user+provider+path with overlapping operations List permissions = List.of( RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/**") - .pathType(RepoPermission.PathType.GLOB) + .value("/acme/**") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.CONFIG) .build(), RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/*") - .pathType(RepoPermission.PathType.GLOB) + .value("/acme/*") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH) .source(RepoPermission.Source.CONFIG) .build()); @@ -462,14 +390,12 @@ void seedFromConfig_conflictingRows_throwsIllegalStateException() { @Test void seedFromConfig_configConflictsWithExistingDb_throwsIllegalStateException() { - // Existing DB row for PUSH; CONFIG row tries to add PUSH_AND_REVIEW on an overlapping path - svc.save(grant( - "alice", "github", "/acme/repo", RepoPermission.PathType.LITERAL, RepoPermission.Operations.PUSH)); + svc.save(grant("alice", "github", "/acme/repo", MatchType.LITERAL, RepoPermission.Operations.PUSH)); List permissions = List.of(RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/**") - .pathType(RepoPermission.PathType.GLOB) + .value("/acme/**") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.CONFIG) .build()); @@ -478,21 +404,20 @@ void seedFromConfig_configConflictsWithExistingDb_throwsIllegalStateException() @Test void seedFromConfig_selfCertifyAlongsidePushAndReview_noConflict() { - // Trusted committer pattern — both entries are needed and must coexist List permissions = List.of( RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/**") - .pathType(RepoPermission.PathType.GLOB) + .value("/acme/**") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.CONFIG) .build(), RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/**") - .pathType(RepoPermission.PathType.GLOB) + .value("/acme/**") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.SELF_CERTIFY) .source(RepoPermission.Source.CONFIG) .build()); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluatorTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluatorTest.java index 8e8522ed..35cae6d9 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluatorTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleEvaluatorTest.java @@ -7,6 +7,8 @@ import org.finos.gitproxy.db.UrlRuleRegistry; import org.finos.gitproxy.db.memory.InMemoryUrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.git.HttpOperation; import org.finos.gitproxy.provider.GitHubProvider; import org.finos.gitproxy.provider.GitProxyProvider; @@ -26,21 +28,25 @@ private static UrlRuleEvaluator evaluatorWith(AccessRule... rules) { return new UrlRuleEvaluator(registry, GITHUB); } - private static AccessRule allow(String owner) { + private static AccessRule allow(MatchTarget target, String value) { return AccessRule.builder() .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner(owner) + .target(target) + .value(value) + .matchType(MatchType.GLOB) .build(); } - private static AccessRule deny(String owner) { + private static AccessRule deny(MatchTarget target, String value) { return AccessRule.builder() .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .owner(owner) + .target(target) + .value(value) + .matchType(MatchType.GLOB) .build(); } @@ -66,7 +72,7 @@ void emptyRegistry_notAllowed() { @Test void allowRule_ownerMatch_allowed() { - var evaluator = evaluatorWith(allow("myorg")); + var evaluator = evaluatorWith(allow(MatchTarget.OWNER, "myorg")); assertInstanceOf( UrlRuleEvaluator.Result.Allowed.class, evaluator.evaluate("myorg/repo", "myorg", "repo", HttpOperation.PUSH)); @@ -78,7 +84,9 @@ void allowRule_slugMatch_allowed() { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .slug("/myorg/repo") + .target(MatchTarget.SLUG) + .value("/myorg/repo") + .matchType(MatchType.LITERAL) .build(); var evaluator = evaluatorWith(rule); assertInstanceOf( @@ -92,7 +100,9 @@ void allowRule_nameGlob_allowed() { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .name("feature-*") + .target(MatchTarget.NAME) + .value("feature-*") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(rule); assertInstanceOf( @@ -105,7 +115,7 @@ void allowRule_nameGlob_allowed() { @Test void allowRule_noMatch_notAllowed() { - var evaluator = evaluatorWith(allow("myorg")); + var evaluator = evaluatorWith(allow(MatchTarget.OWNER, "myorg")); assertInstanceOf( UrlRuleEvaluator.Result.NotAllowed.class, evaluator.evaluate("otherorg/repo", "otherorg", "repo", HttpOperation.PUSH)); @@ -115,18 +125,21 @@ void allowRule_noMatch_notAllowed() { @Test void denyRule_lowerOrderBeatsAllowRule_denied() { - // deny at order 100, allow at order 200 — deny wins var denyRule = AccessRule.builder() .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .owner("blocked") + .target(MatchTarget.OWNER) + .value("blocked") + .matchType(MatchType.GLOB) .build(); var allowRule = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("blocked") + .target(MatchTarget.OWNER) + .value("blocked") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(denyRule, allowRule); assertInstanceOf( @@ -136,18 +149,21 @@ void denyRule_lowerOrderBeatsAllowRule_denied() { @Test void allowRule_lowerOrderBeatsDenyRule_allowed() { - // allow at order 100, deny at order 200 — allow wins var allowRule = AccessRule.builder() .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var denyRule = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(allowRule, denyRule); assertInstanceOf( @@ -157,7 +173,7 @@ void allowRule_lowerOrderBeatsDenyRule_allowed() { @Test void denyRule_noMatch_allowRuleChecked() { - var evaluator = evaluatorWith(deny("blocked"), allow("allowed")); + var evaluator = evaluatorWith(deny(MatchTarget.OWNER, "blocked"), allow(MatchTarget.OWNER, "allowed")); assertInstanceOf( UrlRuleEvaluator.Result.Allowed.class, evaluator.evaluate("allowed/repo", "allowed", "repo", HttpOperation.PUSH)); @@ -171,7 +187,9 @@ void fetchOnlyAllowRule_doesNotEngageForPush() { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.FETCH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(rule); assertInstanceOf( @@ -186,7 +204,9 @@ void pushOnlyAllowRule_doesNotEngageForFetch() { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.PUSH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(rule); assertInstanceOf( @@ -201,13 +221,17 @@ void fetchOnlyDenyRule_doesNotBlockPush() { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.FETCH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var pushAllow = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(fetchDeny, pushAllow); assertInstanceOf( @@ -222,13 +246,17 @@ void pushOnlyDenyRule_doesNotBlockFetch() { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.PUSH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var fetchAllow = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("myorg") + .target(MatchTarget.OWNER) + .value("myorg") + .matchType(MatchType.GLOB) .build(); var evaluator = evaluatorWith(pushDeny, fetchAllow); assertInstanceOf( @@ -254,33 +282,33 @@ void registry_fetchedOnce() { @Test void matchPattern_literal_exactMatch() { - assertTrue(UrlRuleEvaluator.matchPattern("myorg", "myorg")); - assertFalse(UrlRuleEvaluator.matchPattern("myorg", "otherorg")); + assertTrue(UrlRuleEvaluator.matchPattern("myorg", MatchType.LITERAL, "myorg")); + assertFalse(UrlRuleEvaluator.matchPattern("myorg", MatchType.LITERAL, "otherorg")); } @Test void matchPattern_literal_leadingSlashNormalised() { - assertTrue(UrlRuleEvaluator.matchPattern("/owner/repo", "owner/repo")); - assertTrue(UrlRuleEvaluator.matchPattern("owner/repo", "/owner/repo")); + assertTrue(UrlRuleEvaluator.matchPattern("/owner/repo", MatchType.LITERAL, "owner/repo")); + assertTrue(UrlRuleEvaluator.matchPattern("owner/repo", MatchType.LITERAL, "/owner/repo")); } @Test void matchPattern_glob_wildcard() { - assertTrue(UrlRuleEvaluator.matchPattern("myorg-*", "myorg-internal")); - assertFalse(UrlRuleEvaluator.matchPattern("myorg-*", "otherorg-internal")); + assertTrue(UrlRuleEvaluator.matchPattern("myorg-*", MatchType.GLOB, "myorg-internal")); + assertFalse(UrlRuleEvaluator.matchPattern("myorg-*", MatchType.GLOB, "otherorg-internal")); } @Test void matchPattern_regex_matchesRawValue() { - assertTrue(UrlRuleEvaluator.matchPattern("regex:^(myorg|partnerorg)$", "myorg")); - assertTrue(UrlRuleEvaluator.matchPattern("regex:/myorg/.*", "/myorg/any-repo")); - assertFalse(UrlRuleEvaluator.matchPattern("regex:^(myorg|partnerorg)$", "otherog")); + assertTrue(UrlRuleEvaluator.matchPattern("^(myorg|partnerorg)$", MatchType.REGEX, "myorg")); + assertTrue(UrlRuleEvaluator.matchPattern("/myorg/.*", MatchType.REGEX, "/myorg/any-repo")); + assertFalse(UrlRuleEvaluator.matchPattern("^(myorg|partnerorg)$", MatchType.REGEX, "otherog")); } @Test void matchPattern_nullInputs_returnsFalse() { - assertFalse(UrlRuleEvaluator.matchPattern(null, "value")); - assertFalse(UrlRuleEvaluator.matchPattern("pattern", null)); + assertFalse(UrlRuleEvaluator.matchPattern(null, MatchType.LITERAL, "value")); + assertFalse(UrlRuleEvaluator.matchPattern("pattern", MatchType.LITERAL, null)); } // ── operationMatches helper ─────────────────────────────────────────────── @@ -288,7 +316,8 @@ void matchPattern_nullInputs_returnsFalse() { @Test void operationMatches_both_alwaysTrue() { var rule = AccessRule.builder() - .slug("x") + .target(MatchTarget.SLUG) + .value("x") .operations(AccessRule.Operations.BOTH) .build(); assertTrue(UrlRuleEvaluator.operationMatches(rule, HttpOperation.PUSH)); @@ -298,7 +327,8 @@ void operationMatches_both_alwaysTrue() { @Test void operationMatches_pushOnly() { var rule = AccessRule.builder() - .slug("x") + .target(MatchTarget.SLUG) + .value("x") .operations(AccessRule.Operations.PUSH) .build(); assertTrue(UrlRuleEvaluator.operationMatches(rule, HttpOperation.PUSH)); @@ -308,7 +338,8 @@ void operationMatches_pushOnly() { @Test void operationMatches_fetchOnly() { var rule = AccessRule.builder() - .slug("x") + .target(MatchTarget.SLUG) + .value("x") .operations(AccessRule.Operations.FETCH) .build(); assertFalse(UrlRuleEvaluator.operationMatches(rule, HttpOperation.PUSH)); diff --git a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleFilterTest.java b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleFilterTest.java index 46037432..94b7e090 100644 --- a/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleFilterTest.java +++ b/git-proxy-java-core/src/test/java/org/finos/gitproxy/servlet/filter/UrlRuleFilterTest.java @@ -20,6 +20,8 @@ import org.finos.gitproxy.db.memory.InMemoryUrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; import org.finos.gitproxy.db.model.FetchRecord; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.git.GitRequestDetails; import org.finos.gitproxy.git.HttpOperation; import org.finos.gitproxy.provider.GenericProxyProvider; @@ -171,7 +173,9 @@ void aggregate_ruleMatches_passes() throws Exception { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("owner") + .target(MatchTarget.OWNER) + .value("owner") + .matchType(MatchType.GLOB) .build()); GitRequestDetails details = makeDetails("owner", "repo", "/owner/repo"); FakeResponse resp = new FakeResponse(); @@ -187,7 +191,9 @@ void aggregate_noRuleMatch_blocks() throws Exception { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("allowed") + .target(MatchTarget.OWNER) + .value("allowed") + .matchType(MatchType.GLOB) .build()); GitRequestDetails details = makeDetails("not-allowed", "repo", "/not-allowed/repo"); FakeResponse resp = new FakeResponse(); @@ -214,13 +220,17 @@ void aggregate_denyAtLowerOrder_blocksEvenWithAllowRule() throws Exception { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .owner("blocked-owner") + .target(MatchTarget.OWNER) + .value("blocked-owner") + .matchType(MatchType.GLOB) .build(); var allow = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("blocked-owner") + .target(MatchTarget.OWNER) + .value("blocked-owner") + .matchType(MatchType.GLOB) .build(); var aggregate = aggregateWith(deny, allow); GitRequestDetails details = makeDetails("blocked-owner", "repo", "/blocked-owner/repo"); @@ -237,13 +247,17 @@ void aggregate_allowAtLowerOrder_passesEvenWithDenyRule() throws Exception { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("allowed-owner") + .target(MatchTarget.OWNER) + .value("allowed-owner") + .matchType(MatchType.GLOB) .build(); var deny = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .owner("allowed-owner") + .target(MatchTarget.OWNER) + .value("allowed-owner") + .matchType(MatchType.GLOB) .build(); var aggregate = aggregateWith(allow, deny); GitRequestDetails details = makeDetails("allowed-owner", "repo", "/allowed-owner/repo"); @@ -260,7 +274,9 @@ void aggregate_denyOnlyRules_nonMatchedBlocks() throws Exception { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .owner("blocked-owner") + .target(MatchTarget.OWNER) + .value("blocked-owner") + .matchType(MatchType.GLOB) .build(); var aggregate = aggregateWith(deny); GitRequestDetails details = makeDetails("other-owner", "repo", "/other-owner/repo"); @@ -291,7 +307,9 @@ void infoRefs_allowedByRule_passes() throws Exception { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build()); GitRequestDetails details = makeInfoDetails("owner", "repo", "/owner/repo"); FakeResponse resp = new FakeResponse(); @@ -308,13 +326,17 @@ void infoRefs_pushOnlyDenyRule_doesNotBlockFetchInfoRefs() throws Exception { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.PUSH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build(); var fetchAllow = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build(); var aggregate = aggregateWith(pushDeny, fetchAllow); GitRequestDetails details = makeInfoDetails("owner", "repo", "/owner/repo"); @@ -331,13 +353,17 @@ void infoRefs_receivePack_deniedByRule_returns403() throws Exception { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build(); var allow = AccessRule.builder() .ruleOrder(200) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build(); var aggregate = aggregateWith(deny, allow); GitRequestDetails details = makeInfoDetails("owner", "repo", "/owner/repo"); @@ -374,7 +400,9 @@ void infoRefs_fetchBlocked_denyRule_recordsFetch() throws Exception { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build()); var aggregate = new UrlRuleAggregateFilter(50, GITHUB, "/proxy", fetchStore, registry); GitRequestDetails details = makeInfoDetails("owner", "repo", "/owner/repo"); @@ -410,7 +438,9 @@ void infoRefs_fetchAllowed_doesNotRecordFetch() throws Exception { .ruleOrder(100) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .slug("/owner/repo") + .target(MatchTarget.SLUG) + .value("/owner/repo") + .matchType(MatchType.LITERAL) .build()); var aggregate = new UrlRuleAggregateFilter(50, GITHUB, "/proxy", fetchStore, registry); GitRequestDetails details = makeInfoDetails("owner", "repo", "/owner/repo"); diff --git a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PermissionController.java b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PermissionController.java index 1c25f751..196b9386 100644 --- a/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PermissionController.java +++ b/git-proxy-java-dashboard/src/main/java/org/finos/gitproxy/dashboard/controller/PermissionController.java @@ -3,6 +3,8 @@ import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; import java.util.Map; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.permission.RepoPermission; import org.finos.gitproxy.permission.RepoPermissionService; import org.finos.gitproxy.user.ReadOnlyUserStore; @@ -40,17 +42,23 @@ public ResponseEntity add(@PathVariable String username, @RequestBody AddPerm if (req.provider() == null || req.provider().isBlank()) { return ResponseEntity.badRequest().body(Map.of("error", "provider is required")); } - if (req.path() == null || req.path().isBlank()) { - return ResponseEntity.badRequest().body(Map.of("error", "path is required")); + if (req.value() == null || req.value().isBlank()) { + return ResponseEntity.badRequest().body(Map.of("error", "value is required")); } - RepoPermission.PathType pathType; + MatchTarget target; try { - pathType = req.pathType() != null - ? RepoPermission.PathType.valueOf(req.pathType().toUpperCase()) - : RepoPermission.PathType.LITERAL; + target = req.target() != null ? MatchTarget.valueOf(req.target().toUpperCase()) : MatchTarget.SLUG; } catch (IllegalArgumentException e) { - return ResponseEntity.badRequest().body(Map.of("error", "invalid pathType: " + req.pathType())); + return ResponseEntity.badRequest().body(Map.of("error", "invalid target: " + req.target())); + } + + MatchType matchType; + try { + matchType = + req.matchType() != null ? MatchType.valueOf(req.matchType().toUpperCase()) : MatchType.LITERAL; + } catch (IllegalArgumentException e) { + return ResponseEntity.badRequest().body(Map.of("error", "invalid matchType: " + req.matchType())); } RepoPermission.Operations operations; @@ -65,8 +73,9 @@ public ResponseEntity add(@PathVariable String username, @RequestBody AddPerm var permission = RepoPermission.builder() .username(username) .provider(req.provider().trim()) - .path(req.path().trim()) - .pathType(pathType) + .target(target) + .value(req.value().trim()) + .matchType(matchType) .operations(operations) .source(RepoPermission.Source.DB) .build(); @@ -77,8 +86,8 @@ public ResponseEntity add(@PathVariable String username, @RequestBody AddPerm .body(Map.of( "error", String.format( - "Conflicts with existing permission: path '%s' (%s)", - c.getPath(), c.getPathType()))); + "Conflicts with existing permission: value '%s' (%s/%s)", + c.getValue(), c.getTarget(), c.getMatchType()))); } permissionService.save(permission); return ResponseEntity.status(HttpStatus.CREATED).body(permission); @@ -106,5 +115,6 @@ public ResponseEntity delete(@PathVariable String username, @PathVariable Str return ResponseEntity.noContent().build(); } - public record AddPermissionRequest(String provider, String path, String pathType, String operations) {} + public record AddPermissionRequest( + String provider, String target, String value, String matchType, String operations) {} } diff --git a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PermissionControllerTest.java b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PermissionControllerTest.java index 61a609d8..010c1cb5 100644 --- a/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PermissionControllerTest.java +++ b/git-proxy-java-dashboard/src/test/java/org/finos/gitproxy/dashboard/controller/PermissionControllerTest.java @@ -8,6 +8,8 @@ import java.util.List; import java.util.Optional; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.permission.RepoPermission; import org.finos.gitproxy.permission.RepoPermissionService; import org.finos.gitproxy.user.ReadOnlyUserStore; @@ -43,8 +45,9 @@ class PermissionControllerTest { private static final RepoPermission DB_PERM = RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/repo") - .pathType(RepoPermission.PathType.LITERAL) + .target(MatchTarget.SLUG) + .value("/acme/repo") + .matchType(MatchType.LITERAL) .operations(RepoPermission.Operations.PUSH) .source(RepoPermission.Source.DB) .build(); @@ -52,8 +55,9 @@ class PermissionControllerTest { private static final RepoPermission CONFIG_PERM = RepoPermission.builder() .username("alice") .provider("github") - .path("/acme/*") - .pathType(RepoPermission.PathType.GLOB) + .target(MatchTarget.OWNER) + .value("acme") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH_AND_REVIEW) .source(RepoPermission.Source.CONFIG) .build(); @@ -82,12 +86,16 @@ void list_knownUser_returnsPermissions() { // ── POST /api/users/{username}/permissions ─────────────────────────────────── + private static PermissionController.AddPermissionRequest req( + String provider, String target, String value, String matchType, String operations) { + return new PermissionController.AddPermissionRequest(provider, target, value, matchType, operations); + } + @Test void add_unknownUser_returns404() { when(userStore.findByUsername("nobody")).thenReturn(Optional.empty()); - var resp = - controller.add("nobody", new PermissionController.AddPermissionRequest("github", "/a/b", null, null)); + var resp = controller.add("nobody", req("github", null, "/a/b", null, null)); assertEquals(HttpStatus.NOT_FOUND, resp.getStatusCode()); verify(permissionService, never()).save(any()); @@ -97,26 +105,34 @@ void add_unknownUser_returns404() { void add_missingProvider_returns400() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); - var resp = controller.add("alice", new PermissionController.AddPermissionRequest("", "/a/b", null, null)); + var resp = controller.add("alice", req("", null, "/a/b", null, null)); + + assertEquals(HttpStatus.BAD_REQUEST, resp.getStatusCode()); + } + + @Test + void add_missingValue_returns400() { + when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); + + var resp = controller.add("alice", req("github", null, " ", null, null)); assertEquals(HttpStatus.BAD_REQUEST, resp.getStatusCode()); } @Test - void add_missingPath_returns400() { + void add_invalidMatchType_returns400() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); - var resp = controller.add("alice", new PermissionController.AddPermissionRequest("github", " ", null, null)); + var resp = controller.add("alice", req("github", null, "/a/b", "WILDCARD", null)); assertEquals(HttpStatus.BAD_REQUEST, resp.getStatusCode()); } @Test - void add_invalidPathType_returns400() { + void add_invalidTarget_returns400() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); - var resp = controller.add( - "alice", new PermissionController.AddPermissionRequest("github", "/a/b", "WILDCARD", null)); + var resp = controller.add("alice", req("github", "BRANCH", "/a/b", null, null)); assertEquals(HttpStatus.BAD_REQUEST, resp.getStatusCode()); } @@ -125,26 +141,26 @@ void add_invalidPathType_returns400() { void add_invalidOperations_returns400() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); - var resp = - controller.add("alice", new PermissionController.AddPermissionRequest("github", "/a/b", null, "READ")); + var resp = controller.add("alice", req("github", null, "/a/b", null, "READ")); assertEquals(HttpStatus.BAD_REQUEST, resp.getStatusCode()); } @Test - void add_defaults_literalAndPush() { + void add_defaults_slugLiteralAndPush() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); var captor = ArgumentCaptor.forClass(RepoPermission.class); - var resp = controller.add("alice", new PermissionController.AddPermissionRequest("github", "/a/b", null, null)); + var resp = controller.add("alice", req("github", null, "/a/b", null, null)); assertEquals(HttpStatus.CREATED, resp.getStatusCode()); verify(permissionService).save(captor.capture()); var saved = captor.getValue(); assertEquals("alice", saved.getUsername()); assertEquals("github", saved.getProvider()); - assertEquals("/a/b", saved.getPath()); - assertEquals(RepoPermission.PathType.LITERAL, saved.getPathType()); + assertEquals("/a/b", saved.getValue()); + assertEquals(MatchTarget.SLUG, saved.getTarget()); + assertEquals(MatchType.LITERAL, saved.getMatchType()); assertEquals(RepoPermission.Operations.PUSH, saved.getOperations()); assertEquals(RepoPermission.Source.DB, saved.getSource()); } @@ -154,25 +170,24 @@ void add_conflictingPermission_returns400() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); when(permissionService.findConflict(any())).thenReturn(Optional.of(CONFIG_PERM)); - var resp = controller.add( - "alice", new PermissionController.AddPermissionRequest("github", "/acme/repo", "LITERAL", "PUSH")); + var resp = controller.add("alice", req("github", "SLUG", "/acme/repo", "LITERAL", "PUSH")); assertEquals(HttpStatus.BAD_REQUEST, resp.getStatusCode()); verify(permissionService, never()).save(any()); } @Test - void add_explicitGlobAndPush_saved() { + void add_explicitOwnerGlobAndPush_saved() { when(userStore.findByUsername("alice")).thenReturn(Optional.of(ALICE)); var captor = ArgumentCaptor.forClass(RepoPermission.class); - var resp = controller.add( - "alice", new PermissionController.AddPermissionRequest("github", "/acme/*", "GLOB", "PUSH")); + var resp = controller.add("alice", req("github", "OWNER", "acme", "GLOB", "PUSH")); assertEquals(HttpStatus.CREATED, resp.getStatusCode()); verify(permissionService).save(captor.capture()); var saved = captor.getValue(); - assertEquals(RepoPermission.PathType.GLOB, saved.getPathType()); + assertEquals(MatchTarget.OWNER, saved.getTarget()); + assertEquals(MatchType.GLOB, saved.getMatchType()); assertEquals(RepoPermission.Operations.PUSH, saved.getOperations()); } @@ -204,7 +219,9 @@ void delete_permissionBelongsToDifferentUser_returns403() { var bobPerm = RepoPermission.builder() .username("bob") .provider("github") - .path("/a/b") + .target(MatchTarget.SLUG) + .value("/a/b") + .matchType(MatchType.LITERAL) .source(RepoPermission.Source.DB) .build(); when(permissionService.findById(bobPerm.getId())).thenReturn(Optional.of(bobPerm)); diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java index 0238e2e9..356ef673 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java @@ -32,6 +32,8 @@ import org.finos.gitproxy.db.jdbc.JdbcUrlRuleRegistry; import org.finos.gitproxy.db.memory.InMemoryUrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.git.LocalRepositoryCache; import org.finos.gitproxy.jetty.GitProxyContext; import org.finos.gitproxy.jetty.reload.ConfigHolder; @@ -196,13 +198,11 @@ public void validateProviderReferences() { config.getRules() .getAllow() - .forEach(rule -> rule.getProviders() - .forEach(pid -> resolveProviderName("ALLOW rule (order=" + rule.getOrder() + ")", pid))); + .forEach(rule -> resolveProviderName("ALLOW rule (order=" + rule.getOrder() + ")", rule.getProvider())); config.getRules() .getDeny() - .forEach(rule -> rule.getProviders() - .forEach(pid -> resolveProviderName("DENY rule (order=" + rule.getOrder() + ")", pid))); + .forEach(rule -> resolveProviderName("DENY rule (order=" + rule.getOrder() + ")", rule.getProvider())); log.debug( "Provider reference validation passed ({} users, {} permissions, {} allow rules, {} deny rules)", @@ -241,66 +241,11 @@ private String resolveProviderName(String context, String name) { */ public List buildConfigRules() { List rules = new ArrayList<>(); - buildConfigRulesForAccess(rules, config.getRules().getAllow(), AccessRule.Access.ALLOW); - buildConfigRulesForAccess(rules, config.getRules().getDeny(), AccessRule.Access.DENY); + appendAccessRules(rules, config.getRules().getAllow(), AccessRule.Access.ALLOW); + appendAccessRules(rules, config.getRules().getDeny(), AccessRule.Access.DENY); return rules; } - private void buildConfigRulesForAccess( - List rules, List ruleConfigs, AccessRule.Access access) { - for (RuleConfig rule : ruleConfigs) { - if (!rule.isEnabled()) continue; - - // Validate provider names and resolve to stored IDs — validates at startup - List resolvedProviderIds = rule.getProviders().stream() - .map(n -> resolveProviderName(access.name() + " rule (order=" + rule.getOrder() + ")", n)) - .toList(); - // null provider = applies to all providers; specific IDs = scoped - List providerScopes = - resolvedProviderIds.isEmpty() ? java.util.Collections.singletonList(null) : resolvedProviderIds; - - int order = rule.getOrder(); - AccessRule.Operations ops = toOperations(rule.getOperations()); - String accessLabel = access.name().toLowerCase(); - - for (String providerId : providerScopes) { - for (String slug : rule.getSlugs()) { - rules.add(AccessRule.builder() - .ruleOrder(order) - .access(access) - .operations(ops) - .provider(providerId) - .slug(slug) - .source(AccessRule.Source.CONFIG) - .build()); - log.debug("Added slug {} rule for provider {}: {}", accessLabel, providerId, slug); - } - for (String owner : rule.getOwners()) { - rules.add(AccessRule.builder() - .ruleOrder(order) - .access(access) - .operations(ops) - .provider(providerId) - .owner(owner) - .source(AccessRule.Source.CONFIG) - .build()); - log.debug("Added owner {} rule for provider {}: {}", accessLabel, providerId, owner); - } - for (String name : rule.getNames()) { - rules.add(AccessRule.builder() - .ruleOrder(order) - .access(access) - .operations(ops) - .provider(providerId) - .name(name) - .source(AccessRule.Source.CONFIG) - .build()); - log.debug("Added name {} rule for provider {}: {}", accessLabel, providerId, name); - } - } - } - } - /** * Builds a {@link CommitConfig} from the {@code commit:} YAML section. Pattern strings are compiled here; absent or * blank strings produce permissive defaults (no restriction). @@ -571,12 +516,13 @@ public List buildConfigPermissions(GitProxyConfig cfg) { .map(p -> { String resolvedId = resolveProviderName("Permission for user '" + p.getUsername() + "'", p.getProvider()); + MatchConfig m = p.getMatch(); return RepoPermission.builder() .username(p.getUsername()) .provider(resolvedId) - .path(p.getPath()) - .pathType(RepoPermission.PathType.valueOf( - p.getPathType().toUpperCase())) + .target(MatchTarget.valueOf(m.getTarget().toUpperCase())) + .value(m.getValue()) + .matchType(MatchType.valueOf((m.getType() != null ? m.getType() : "LITERAL").toUpperCase())) .operations(RepoPermission.Operations.valueOf( p.getOperations().toUpperCase())) .source(RepoPermission.Source.CONFIG) @@ -627,50 +573,26 @@ private void appendAccessRules(List result, List rules, for (RuleConfig rule : rules) { if (!rule.isEnabled()) continue; AccessRule.Operations ops = toOperations(rule.getOperations()); - List rawProviders = - rule.getProviders().isEmpty() ? java.util.Collections.singletonList(null) : rule.getProviders(); - for (String rawProvider : rawProviders) { - // null → applies to all providers (no resolution needed) - String resolvedId = - resolveProviderName(access.name() + " rule (order=" + rule.getOrder() + ")", rawProvider); - for (String rawSlug : rule.getSlugs()) { - String slug = rawSlug.startsWith("/") ? rawSlug : "/" + rawSlug; - result.add(AccessRule.builder() - .provider(resolvedId) - .slug(slug) - .access(access) - .operations(ops) - .source(AccessRule.Source.CONFIG) - .ruleOrder(rule.getOrder()) - .build()); - } - for (String owner : rule.getOwners()) { - result.add(AccessRule.builder() - .provider(resolvedId) - .owner(owner) - .access(access) - .operations(ops) - .source(AccessRule.Source.CONFIG) - .ruleOrder(rule.getOrder()) - .build()); - } - for (String name : rule.getNames()) { - result.add(AccessRule.builder() - .provider(resolvedId) - .name(name) - .access(access) - .operations(ops) - .source(AccessRule.Source.CONFIG) - .ruleOrder(rule.getOrder()) - .build()); - } - } + String rawProvider = rule.getProvider().isBlank() ? null : rule.getProvider(); + String resolvedId = + resolveProviderName(access.name() + " rule (order=" + rule.getOrder() + ")", rawProvider); + MatchConfig m = rule.getMatch(); + result.add(AccessRule.builder() + .provider(resolvedId) + .target(MatchTarget.valueOf(m.getTarget().toUpperCase())) + .value(m.getValue()) + .matchType(MatchType.valueOf((m.getType() != null ? m.getType() : "GLOB").toUpperCase())) + .access(access) + .operations(ops) + .source(AccessRule.Source.CONFIG) + .ruleOrder(rule.getOrder()) + .build()); } } - private static AccessRule.Operations toOperations(List ops) { - if (ops == null || ops.isEmpty() || ops.size() > 1) return AccessRule.Operations.BOTH; - return switch (ops.get(0).toUpperCase()) { + private static AccessRule.Operations toOperations(String ops) { + if (ops == null || ops.isBlank()) return AccessRule.Operations.BOTH; + return switch (ops.toUpperCase()) { case "FETCH" -> AccessRule.Operations.FETCH; case "PUSH" -> AccessRule.Operations.PUSH; default -> AccessRule.Operations.BOTH; diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java new file mode 100644 index 00000000..59058f06 --- /dev/null +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java @@ -0,0 +1,22 @@ +package org.finos.gitproxy.jetty.config; + +import lombok.Data; + +/** Binds the {@code match:} block shared by rule and permission config entries. */ +@Data +public class MatchConfig { + + /** Which part of the repository URL to match against. One of: {@code SLUG}, {@code OWNER}, {@code NAME}. */ + private String target = "SLUG"; + + /** Pattern string — interpreted according to {@link #type}. */ + private String value = ""; + + /** + * How {@link #value} is matched. One of: {@code LITERAL}, {@code GLOB}, {@code REGEX}. + * + *

    When omitted, the caller is responsible for applying a context-specific default: {@code GLOB} for URL rules, + * {@code LITERAL} for user permissions. + */ + private String type = null; +} diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/PermissionConfig.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/PermissionConfig.java index 2ebed19b..b7220b7f 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/PermissionConfig.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/PermissionConfig.java @@ -11,9 +11,11 @@ * permissions: * - username: alice * provider: github - * path: /owner/repo - * operations: PUSH # PUSH | APPROVE | ALL (default: ALL) - * path-type: LITERAL # LITERAL | GLOB (default: LITERAL) + * match: + * target: SLUG + * value: /owner/repo + * type: LITERAL + * operations: PUSH * */ @Data @@ -25,14 +27,8 @@ public class PermissionConfig { /** Provider name (e.g. {@code github}, {@code gitea}). */ private String provider = ""; - /** - * Repository path pattern. For {@code LITERAL}: exact match (e.g. {@code /owner/repo}). For {@code GLOB}: shell - * glob (e.g. {@code /owner/*}). - */ - private String path = ""; - - /** How {@code path} is matched. {@code LITERAL} (default) or {@code GLOB}. */ - private String pathType = "LITERAL"; + /** Repository match criteria — target, value, and type. */ + private MatchConfig match = new MatchConfig(); /** * Which operations are granted. {@code PUSH}, {@code REVIEW}, {@code PUSH_AND_REVIEW}, or {@code SELF_CERTIFY} diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/RuleConfig.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/RuleConfig.java index f5696aa4..d8a5b4d9 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/RuleConfig.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/RuleConfig.java @@ -1,7 +1,5 @@ package org.finos.gitproxy.jetty.config; -import java.util.ArrayList; -import java.util.List; import lombok.Data; /** Binds a single entry under {@code rules.allow[]} (or {@code rules.deny[]}) in git-proxy.yml. */ @@ -11,21 +9,12 @@ public class RuleConfig { private boolean enabled = true; private int order = 1100; - /** Git operations this entry matches: {@code FETCH}, {@code PUSH}. Empty = none. */ - private List operations = new ArrayList<>(); + /** Git operations this entry matches: {@code FETCH}, {@code PUSH}, or {@code BOTH} (default). */ + private String operations = "BOTH"; - /** Provider names to scope this entry to. Empty = all providers. */ - private List providers = new ArrayList<>(); + /** Provider name to scope this entry to. Omit (or leave blank) to match all providers. */ + private String provider = ""; - /** - * Repository slugs ({@code owner/repo}). Supports glob patterns and {@code regex:}-prefixed Java regular - * expressions. - */ - private List slugs = new ArrayList<>(); - - /** Repository owner/org names. Supports glob patterns and {@code regex:}-prefixed Java regular expressions. */ - private List owners = new ArrayList<>(); - - /** Repository names. Supports glob patterns and {@code regex:}-prefixed Java regular expressions. */ - private List names = new ArrayList<>(); + /** Repository match criteria — target, value, and type. */ + private MatchConfig match = new MatchConfig(); } diff --git a/git-proxy-java-server/src/main/resources/git-proxy-local.yml b/git-proxy-java-server/src/main/resources/git-proxy-local.yml index bb54aeb9..463edd46 100644 --- a/git-proxy-java-server/src/main/resources/git-proxy-local.yml +++ b/git-proxy-java-server/src/main/resources/git-proxy-local.yml @@ -100,29 +100,39 @@ attestations: # independently of the pusher identity; no SCM identity needed. # (admin can also approve via the dashboard by virtue of its ADMIN role.) # -# Path types intentionally exercise all three matching modes: -# GitHub → LITERAL (exact repo path) -# GitLab → GLOB (/coopernetes/* matches any repo under that owner) +# Match types intentionally exercise all three modes: +# GitHub → LITERAL (exact slug) +# GitLab → GLOB (all repos under the coopernetes owner) # Codeberg → REGEX (anchored Java regex — permission exists but identity will # block before it's evaluated in push-identity-codeberg.sh) permissions: - username: thomas-cooper provider: github - path: /finos/git-proxy + match: + target: SLUG + value: /finos/git-proxy + type: LITERAL operations: PUSH - username: thomas-cooper provider: github - path: /coopernetes/test-repo + match: + target: SLUG + value: /coopernetes/test-repo + type: LITERAL operations: PUSH - username: thomas-cooper provider: gitlab - path: /coopernetes/* - path-type: GLOB + match: + target: OWNER + value: coopernetes + type: GLOB operations: PUSH - username: thomas-cooper provider: codeberg - path: \/coopernetes\/test\-repo\-([\w]+) # matches /coopernetes/test-repo-codeberg & any test-repo- - path-type: REGEX + match: + target: SLUG + value: '/coopernetes/test-repo-([\w]+)' # matches /coopernetes/test-repo-codeberg & any test-repo- + type: REGEX operations: PUSH rules: @@ -139,57 +149,57 @@ rules: # /finos/git-proxy is allowed above (for dev testing), but git-proxy-java itself is not. - enabled: true order: 10 - operations: - - PUSH - providers: - - github - slugs: - - /coopernetes/git-proxy-java + operations: PUSH + provider: github + match: + target: SLUG + value: /coopernetes/git-proxy-java + type: LITERAL # GitHub Pages repositories are presentation sites — block pushes even if the # owner is covered by an allow rule (e.g. a future finos/* allow entry). - enabled: true order: 20 - operations: - - PUSH - providers: - - github - names: - - "*.github.io" + operations: PUSH + provider: github + match: + target: NAME + value: "*.github.io" + type: GLOB + allow: - enabled: true order: 30 - operations: - - FETCH - providers: - - github - slugs: - - /finos/git-proxy - - /coopernetes/git-proxy-java - - /coopernetes/test-repo + operations: FETCH + provider: github + match: + target: SLUG + value: '/(finos|coopernetes)/(git-proxy|git-proxy-java|test-repo)' + type: REGEX + - enabled: true order: 40 - operations: - - FETCH - - PUSH - providers: - - gitlab - slugs: - - /coopernetes/test-repo-gitlab + operations: BOTH + provider: gitlab + match: + target: SLUG + value: /coopernetes/test-repo-gitlab + type: LITERAL + - enabled: true order: 50 - operations: - - FETCH - - PUSH - providers: - - codeberg - slugs: - - /coopernetes/test-repo-codeberg + operations: BOTH + provider: codeberg + match: + target: SLUG + value: /coopernetes/test-repo-codeberg + type: LITERAL + - enabled: true order: 60 - operations: - - FETCH - owners: - - finos - providers: - - github + operations: FETCH + provider: github + match: + target: OWNER + value: finos + type: GLOB diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/IdentityResolutionE2ETest.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/IdentityResolutionE2ETest.java index 91fd5eb1..87c2b579 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/IdentityResolutionE2ETest.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/IdentityResolutionE2ETest.java @@ -12,6 +12,8 @@ import org.finos.gitproxy.approval.UiApprovalGateway; import org.finos.gitproxy.config.CommitConfig; import org.finos.gitproxy.db.model.Attestation; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.permission.InMemoryRepoPermissionStore; import org.finos.gitproxy.permission.RepoPermission; import org.finos.gitproxy.permission.RepoPermissionService; @@ -78,8 +80,9 @@ static void startInfrastructure() throws Exception { permissionStore.save(RepoPermission.builder() .username(GiteaContainer.TEST_USER) .provider(proxy.getProviderId()) - .path("/" + GiteaContainer.TEST_ORG + "/" + GiteaContainer.TEST_REPO) - .pathType(RepoPermission.PathType.LITERAL) + .target(MatchTarget.SLUG) + .value("/" + GiteaContainer.TEST_ORG + "/" + GiteaContainer.TEST_REPO) + .matchType(MatchType.LITERAL) .operations(RepoPermission.Operations.PUSH) .build()); @@ -197,8 +200,9 @@ void linkedUser_wrongCommitEmail_strictMode_rejected() throws Exception { strictPermissionStore.save(RepoPermission.builder() .username(GiteaContainer.TEST_USER) .provider(strictProxy.getProviderId()) - .path("/" + GiteaContainer.TEST_ORG + "/" + GiteaContainer.TEST_REPO) - .pathType(RepoPermission.PathType.LITERAL) + .target(MatchTarget.SLUG) + .value("/" + GiteaContainer.TEST_ORG + "/" + GiteaContainer.TEST_REPO) + .matchType(MatchType.LITERAL) .operations(RepoPermission.Operations.PUSH) .build()); diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/JettyProxyFixture.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/JettyProxyFixture.java index 38cc8fbc..5bfba329 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/JettyProxyFixture.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/JettyProxyFixture.java @@ -25,6 +25,8 @@ import org.finos.gitproxy.db.PushStoreFactory; import org.finos.gitproxy.db.memory.InMemoryUrlRuleRegistry; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.git.*; import org.finos.gitproxy.permission.RepoPermissionService; import org.finos.gitproxy.provider.GenericProxyProvider; @@ -139,7 +141,9 @@ class JettyProxyFixture implements AutoCloseable { .ruleOrder(1) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("*") + .target(MatchTarget.OWNER) + .value("*") + .matchType(MatchType.GLOB) .build()); } else { configRules.forEach(urlRuleRegistry::save); diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/PermissionE2ETest.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/PermissionE2ETest.java index 91ff83b3..93468664 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/PermissionE2ETest.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/PermissionE2ETest.java @@ -10,6 +10,8 @@ import java.util.List; import java.util.Optional; import org.finos.gitproxy.approval.UiApprovalGateway; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.permission.InMemoryRepoPermissionStore; import org.finos.gitproxy.permission.RepoPermission; import org.finos.gitproxy.permission.RepoPermissionService; @@ -151,8 +153,9 @@ void literal_grant_allows_push() throws Exception { permissionStore.save(RepoPermission.builder() .username(GiteaContainer.TEST_USER) .provider(proxy.getProviderId()) - .path(path) - .pathType(RepoPermission.PathType.LITERAL) + .target(MatchTarget.SLUG) + .value(path) + .matchType(MatchType.LITERAL) .operations(RepoPermission.Operations.PUSH) .build()); @@ -174,8 +177,9 @@ void glob_grant_allows_push() throws Exception { permissionStore.save(RepoPermission.builder() .username(GiteaContainer.TEST_USER) .provider(proxy.getProviderId()) - .path("/" + GiteaContainer.TEST_ORG + "/*") - .pathType(RepoPermission.PathType.GLOB) + .target(MatchTarget.SLUG) + .value("/" + GiteaContainer.TEST_ORG + "/*") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH) .build()); @@ -196,8 +200,9 @@ void regex_grant_allows_push() throws Exception { permissionStore.save(RepoPermission.builder() .username(GiteaContainer.TEST_USER) .provider(proxy.getProviderId()) - .path("^/" + GiteaContainer.TEST_ORG + "/.+") - .pathType(RepoPermission.PathType.REGEX) + .target(MatchTarget.SLUG) + .value("^/" + GiteaContainer.TEST_ORG + "/.+") + .matchType(MatchType.REGEX) .operations(RepoPermission.Operations.PUSH) .build()); @@ -219,8 +224,9 @@ void glob_grant_does_not_match_different_owner() throws Exception { permissionStore.save(RepoPermission.builder() .username(GiteaContainer.TEST_USER) .provider(proxy.getProviderId()) - .path("/other-owner/*") - .pathType(RepoPermission.PathType.GLOB) + .target(MatchTarget.SLUG) + .value("/other-owner/*") + .matchType(MatchType.GLOB) .operations(RepoPermission.Operations.PUSH) .build()); diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/UrlRuleE2ETest.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/UrlRuleE2ETest.java index 831f8b56..4e19f433 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/UrlRuleE2ETest.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/UrlRuleE2ETest.java @@ -9,6 +9,8 @@ import java.time.Instant; import java.util.List; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchTarget; +import org.finos.gitproxy.db.model.MatchType; import org.junit.jupiter.api.*; /** @@ -318,31 +320,41 @@ private static List buildRules(java.net.URI giteaUri) { .ruleOrder(100) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.BOTH) - .slug("/otherorg/other-secret") + .target(MatchTarget.SLUG) + .value("/otherorg/other-secret") + .matchType(MatchType.LITERAL) .build(), AccessRule.builder() .ruleOrder(101) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.PUSH) - .name("*-readonly") + .target(MatchTarget.NAME) + .value("*-readonly") + .matchType(MatchType.GLOB) .build(), AccessRule.builder() .ruleOrder(102) .access(AccessRule.Access.DENY) .operations(AccessRule.Operations.PUSH) - .name("regex:(?i)(^|-)secret(-|$).*") + .target(MatchTarget.NAME) + .value("(?i)(^|-)secret(-|$).*") + .matchType(MatchType.REGEX) .build(), AccessRule.builder() .ruleOrder(110) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .slug("/test-owner/test-repo") + .target(MatchTarget.SLUG) + .value("/test-owner/test-repo") + .matchType(MatchType.LITERAL) .build(), AccessRule.builder() .ruleOrder(111) .access(AccessRule.Access.ALLOW) .operations(AccessRule.Operations.BOTH) - .owner("otherorg") + .target(MatchTarget.OWNER) + .value("otherorg") + .matchType(MatchType.GLOB) .build()); } } diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java index a6fa81a7..c4d16ac3 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java @@ -8,6 +8,7 @@ import org.finos.gitproxy.approval.UiApprovalGateway; import org.finos.gitproxy.db.PushStoreFactory; import org.finos.gitproxy.db.model.AccessRule; +import org.finos.gitproxy.db.model.MatchType; import org.finos.gitproxy.permission.RepoPermission; import org.finos.gitproxy.provider.GitHubProvider; import org.finos.gitproxy.provider.GitProxyProvider; @@ -43,11 +44,7 @@ void buildApprovalGateway_unknownMode_fallsBackToAuto() { @Test void validateProviderReferences_validConfig_doesNotThrow() { var config = configWithGithub(); - var permission = new PermissionConfig(); - permission.setUsername("alice"); - permission.setProvider("github"); - permission.setPath("/org/repo"); - config.setPermissions(List.of(permission)); + config.setPermissions(List.of(slugPerm("alice", "github", "/org/repo"))); assertDoesNotThrow(() -> new JettyConfigurationBuilder(config).validateProviderReferences()); } @@ -55,26 +52,18 @@ void validateProviderReferences_validConfig_doesNotThrow() { @Test void validateProviderReferences_unknownPermissionProvider_throws() { var config = configWithGithub(); - var permission = new PermissionConfig(); - permission.setUsername("alice"); - permission.setProvider("not-a-provider"); - permission.setPath("/org/repo"); - config.setPermissions(List.of(permission)); + config.setPermissions(List.of(slugPerm("alice", "not-a-provider", "/org/repo"))); var builder = new JettyConfigurationBuilder(config); var ex = assertThrows(IllegalStateException.class, builder::validateProviderReferences); assertTrue(ex.getMessage().contains("not-a-provider")); - // Should list the configured providers in the error assertTrue(ex.getMessage().contains("github")); } @Test void validateProviderReferences_unknownRuleProvider_throws() { var config = configWithGithub(); - var ruleConfig = new RuleConfig(); - ruleConfig.setProviders(List.of("typo-provider")); - ruleConfig.setSlugs(List.of("/org/repo")); - config.getRules().setAllow(List.of(ruleConfig)); + config.getRules().setAllow(List.of(slugRule("typo-provider", "/org/repo", 1100))); var builder = new JettyConfigurationBuilder(config); assertThrows(IllegalStateException.class, builder::validateProviderReferences); @@ -82,7 +71,6 @@ void validateProviderReferences_unknownRuleProvider_throws() { @Test void validateProviderReferences_emptyConfig_doesNotThrow() { - // No providers, no cross-references → nothing to validate assertDoesNotThrow(() -> new JettyConfigurationBuilder(new GitProxyConfig()).validateProviderReferences()); } @@ -93,15 +81,12 @@ void buildProviderRegistry_keyedByFriendlyName() { var builder = new JettyConfigurationBuilder(configWithGithub()); var registry = builder.buildProviderRegistry(); - // Lookup by name assertTrue(registry.getProvider("github").isPresent()); assertInstanceOf(GitHubProvider.class, registry.getProvider("github").orElseThrow()); - // Lookup by name via resolveProvider assertTrue(registry.resolveProvider("github").isPresent()); assertSame( registry.getProvider("github").orElseThrow(), registry.resolveProvider("github").orElseThrow()); - // Name resolves to a stable provider ID assertEquals( registry.resolveProvider("github").orElseThrow().getProviderId(), registry.resolveProvider("github").orElseThrow().getProviderId()); @@ -112,16 +97,11 @@ void buildProviderRegistry_keyedByFriendlyName() { @Test void buildConfigPermissions_name_stored_on_permission() { var config = configWithGithub(); - var permission = new PermissionConfig(); - permission.setUsername("alice"); - permission.setProvider("github"); - permission.setPath("/org/repo"); - config.setPermissions(List.of(permission)); + config.setPermissions(List.of(slugPerm("alice", "github", "/org/repo"))); List perms = new JettyConfigurationBuilder(config).buildConfigPermissions(config); assertEquals(1, perms.size()); - assertEquals("github", perms.get(0).getProvider()); assertEquals("alice", perms.get(0).getUsername()); } @@ -129,11 +109,7 @@ void buildConfigPermissions_name_stored_on_permission() { @Test void buildConfigPermissions_unknownProvider_throwsWithHelpfulMessage() { var config = configWithGithub(); - var permission = new PermissionConfig(); - permission.setUsername("carol"); - permission.setProvider("nonexistent"); - permission.setPath("/org/repo"); - config.setPermissions(List.of(permission)); + config.setPermissions(List.of(slugPerm("carol", "nonexistent", "/org/repo"))); var builder = new JettyConfigurationBuilder(config); var ex = assertThrows(IllegalStateException.class, () -> builder.buildConfigPermissions(config)); @@ -146,30 +122,23 @@ void buildConfigPermissions_unknownProvider_throwsWithHelpfulMessage() { @Test void buildConfigRules_name_stored_on_rule() { var config = configWithGithub(); - var ruleConfig = new RuleConfig(); - ruleConfig.setProviders(List.of("github")); - ruleConfig.setSlugs(List.of("/org/repo")); - config.getRules().setAllow(List.of(ruleConfig)); + config.getRules().setAllow(List.of(slugRule("github", "/org/repo", 1100))); List rules = new JettyConfigurationBuilder(config).buildConfigRules(config); assertFalse(rules.isEmpty()); - assertEquals("github", rules.get(0).getProvider()); } @Test void buildConfigRules_noProviderFilter_storesNullProvider() { var config = configWithGithub(); - var ruleConfig = new RuleConfig(); - // no providers → applies to all - ruleConfig.setSlugs(List.of("/org/repo")); - config.getRules().setAllow(List.of(ruleConfig)); + config.getRules().setAllow(List.of(slugRule("", "/org/repo", 1100))); List rules = new JettyConfigurationBuilder(config).buildConfigRules(config); assertFalse(rules.isEmpty()); - assertNull(rules.get(0).getProvider()); // null = all providers + assertNull(rules.get(0).getProvider()); } // ---- buildConfigRules — provider name scoping ---- @@ -177,11 +146,7 @@ void buildConfigRules_noProviderFilter_storesNullProvider() { @Test void buildConfigRules_name_scopes_rule_to_provider() { var config = configWithGithub(); - var ruleConfig = new RuleConfig(); - ruleConfig.setOrder(110); - ruleConfig.setProviders(List.of("github")); - ruleConfig.setSlugs(List.of("/org/repo")); - config.getRules().setAllow(List.of(ruleConfig)); + config.getRules().setAllow(List.of(slugRule("github", "/org/repo", 110))); var builder = new JettyConfigurationBuilder(config); var githubProviderId = builder.buildProviderRegistry() @@ -199,11 +164,7 @@ void buildConfigRules_name_scopes_rule_to_provider() { @Test void buildConfigRules_name_excludes_other_provider() { var config = configWithGithubAndGitlab(); - var ruleConfig = new RuleConfig(); - ruleConfig.setOrder(110); - ruleConfig.setProviders(List.of("github")); // only github - ruleConfig.setSlugs(List.of("/org/repo")); - config.getRules().setAllow(List.of(ruleConfig)); + config.getRules().setAllow(List.of(slugRule("github", "/org/repo", 110))); var builder = new JettyConfigurationBuilder(config); var gitlabProviderId = builder.buildProviderRegistry() @@ -238,15 +199,9 @@ void twoProvidersOfSameType_differentNames_haveDistinctProviderIds() { @Test void twoProvidersOfSameType_permissionsAreKeptSeparate() { var config = configWithTwoGitHubProviders(); - var publicPerm = new PermissionConfig(); - publicPerm.setUsername("alice"); - publicPerm.setProvider("github"); - publicPerm.setPath("/org/public-repo"); - var internalPerm = new PermissionConfig(); - internalPerm.setUsername("bob"); - internalPerm.setProvider("internal-github"); - internalPerm.setPath("/corp/internal-repo"); - config.setPermissions(List.of(publicPerm, internalPerm)); + config.setPermissions(List.of( + slugPerm("alice", "github", "/org/public-repo"), + slugPerm("bob", "internal-github", "/corp/internal-repo"))); var perms = new JettyConfigurationBuilder(config).buildConfigPermissions(config); @@ -263,8 +218,58 @@ void twoProvidersOfSameType_permissionsAreKeptSeparate() { assertEquals("internal-github", bobPerm.getProvider()); } + // ---- MatchConfig.type defaults ---- + + @Test + void buildConfigPermissions_nullType_defaultsToLiteral() { + var config = configWithGithub(); + var perm = slugPerm("alice", "github", "/org/repo"); + perm.getMatch().setType(null); + config.setPermissions(List.of(perm)); + + List perms = new JettyConfigurationBuilder(config).buildConfigPermissions(config); + + assertEquals(MatchType.LITERAL, perms.get(0).getMatchType()); + } + + @Test + void buildConfigRules_nullType_defaultsToGlob() { + var config = configWithGithub(); + var rule = slugRule("github", "/org/repo", 1100); + rule.getMatch().setType(null); + config.getRules().setAllow(List.of(rule)); + + List rules = new JettyConfigurationBuilder(config).buildConfigRules(config); + + assertEquals(MatchType.GLOB, rules.get(0).getMatchType()); + } + // ---- helpers ---- + private static PermissionConfig slugPerm(String username, String provider, String value) { + var perm = new PermissionConfig(); + perm.setUsername(username); + perm.setProvider(provider); + var m = new MatchConfig(); + m.setTarget("SLUG"); + m.setValue(value); + m.setType("LITERAL"); + perm.setMatch(m); + return perm; + } + + private static RuleConfig slugRule(String provider, String value, int order) { + var rule = new RuleConfig(); + rule.setProvider(provider); + rule.setOrder(order); + var m = new MatchConfig(); + m.setTarget("SLUG"); + m.setValue(value); + m.setType("LITERAL"); + rule.setMatch(m); + return rule; + } + private static GitProxyConfig configWithApprovalMode(String mode) { var config = new GitProxyConfig(); config.getServer().setApprovalMode(mode); diff --git a/scripts/migrate-provider-ids.sql b/scripts/migrate-provider-ids.sql index 827ed32b..c668c959 100644 --- a/scripts/migrate-provider-ids.sql +++ b/scripts/migrate-provider-ids.sql @@ -16,15 +16,15 @@ -- user_scm_identities has a composite PK that includes provider, so rows must be -- inserted with the new key and old rows deleted (UPDATE would violate the PK constraint). -INSERT INTO user_scm_identities (username, provider, scm_username, verified, source) - SELECT username, 'new_value', scm_username, verified, source +INSERT INTO user_scm_identities (username, provider, scm_username, verified) + SELECT username, 'new_value', scm_username, verified FROM user_scm_identities WHERE provider = 'old_value'; DELETE FROM user_scm_identities WHERE provider = 'old_value'; -- scm_token_cache also has a composite PK including provider -INSERT INTO scm_token_cache (token_hash, provider, username, cached_at, expires_at) - SELECT token_hash, 'new_value', username, cached_at, expires_at +INSERT INTO scm_token_cache (token_hash, provider, proxy_username, cached_at) + SELECT token_hash, 'new_value', proxy_username, cached_at FROM scm_token_cache WHERE provider = 'old_value'; DELETE FROM scm_token_cache WHERE provider = 'old_value'; From 6a483b828f1f1e036c67a0eeb5dd25ef88a999fd Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Tue, 5 May 2026 17:12:27 -0400 Subject: [PATCH 2/4] fix: unify match type default to GLOB for both rules and permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing the asymmetry where permissions defaulted to LITERAL and rules to GLOB. A single universal default (GLOB) is simpler: operators don't have to remember which context they're in, and the practical risk is negligible — a permission value with no wildcard chars behaves identically under GLOB and LITERAL. Co-Authored-By: Claude Sonnet 4.6 --- docs/CONFIGURATION.md | 5 +---- .../gitproxy/jetty/config/JettyConfigurationBuilder.java | 2 +- .../java/org/finos/gitproxy/jetty/config/MatchConfig.java | 3 +-- .../gitproxy/jetty/config/JettyConfigurationBuilderTest.java | 4 ++-- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b84d1499..886dc1dd 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1100,12 +1100,9 @@ permissions: | `match` | object | — | Repository match criteria — see below | | `match.target` | enum | `SLUG` | What to match: `SLUG` (`/owner/repo`), `OWNER`, or `NAME` | | `match.value` | string | — | The pattern to match against the chosen target | -| `match.type` | enum | `LITERAL` | How to interpret the pattern: `LITERAL`, `GLOB`, or `REGEX` | +| `match.type` | enum | `GLOB` | How to interpret the pattern: `LITERAL`, `GLOB`, or `REGEX` | | `operations` | enum | `PUSH_AND_REVIEW`| What the user may do: `PUSH`, `REVIEW`, `PUSH_AND_REVIEW`, `SELF_CERTIFY` | -> **Default match type differs between rules and permissions.** URL rules default to `GLOB` (safer for broad allow/deny -> policies). Permissions default to `LITERAL` (safer for access grants — explicit intent required to use wildcards). - ### Pattern matching Permissions support the same three match types as URL rules (LITERAL, GLOB, REGEX) applied to the same three targets diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java index 356ef673..0e200dc4 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilder.java @@ -522,7 +522,7 @@ public List buildConfigPermissions(GitProxyConfig cfg) { .provider(resolvedId) .target(MatchTarget.valueOf(m.getTarget().toUpperCase())) .value(m.getValue()) - .matchType(MatchType.valueOf((m.getType() != null ? m.getType() : "LITERAL").toUpperCase())) + .matchType(MatchType.valueOf((m.getType() != null ? m.getType() : "GLOB").toUpperCase())) .operations(RepoPermission.Operations.valueOf( p.getOperations().toUpperCase())) .source(RepoPermission.Source.CONFIG) diff --git a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java index 59058f06..133623c4 100644 --- a/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java +++ b/git-proxy-java-server/src/main/java/org/finos/gitproxy/jetty/config/MatchConfig.java @@ -15,8 +15,7 @@ public class MatchConfig { /** * How {@link #value} is matched. One of: {@code LITERAL}, {@code GLOB}, {@code REGEX}. * - *

    When omitted, the caller is responsible for applying a context-specific default: {@code GLOB} for URL rules, - * {@code LITERAL} for user permissions. + *

    When omitted, defaults to {@code GLOB} for both URL rules and permissions. */ private String type = null; } diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java index c4d16ac3..54f6fd61 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/jetty/config/JettyConfigurationBuilderTest.java @@ -221,7 +221,7 @@ void twoProvidersOfSameType_permissionsAreKeptSeparate() { // ---- MatchConfig.type defaults ---- @Test - void buildConfigPermissions_nullType_defaultsToLiteral() { + void buildConfigPermissions_nullType_defaultsToGlob() { var config = configWithGithub(); var perm = slugPerm("alice", "github", "/org/repo"); perm.getMatch().setType(null); @@ -229,7 +229,7 @@ void buildConfigPermissions_nullType_defaultsToLiteral() { List perms = new JettyConfigurationBuilder(config).buildConfigPermissions(config); - assertEquals(MatchType.LITERAL, perms.get(0).getMatchType()); + assertEquals(MatchType.GLOB, perms.get(0).getMatchType()); } @Test From d712ee2b5134acfc5a07d62e08c8b6131be677f1 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Tue, 5 May 2026 17:14:30 -0400 Subject: [PATCH 3/4] fix: default match type dropdown to GLOB in rule and permission forms Aligns the UI default with the backend: omitting type now means GLOB in both the YAML config and the dashboard add forms. Co-Authored-By: Claude Sonnet 4.6 --- git-proxy-java-dashboard/frontend/src/pages/Repos.tsx | 2 +- git-proxy-java-dashboard/frontend/src/pages/UserDetail.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-proxy-java-dashboard/frontend/src/pages/Repos.tsx b/git-proxy-java-dashboard/frontend/src/pages/Repos.tsx index d1dd65cc..75fce865 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/Repos.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/Repos.tsx @@ -111,7 +111,7 @@ interface AddRuleForm { const DEFAULT_FORM: AddRuleForm = { access: 'ALLOW', targetType: 'slug', - patternType: 'LITERAL', + patternType: 'GLOB', pattern: '', provider: '', operations: 'BOTH', diff --git a/git-proxy-java-dashboard/frontend/src/pages/UserDetail.tsx b/git-proxy-java-dashboard/frontend/src/pages/UserDetail.tsx index dffd713e..e7dabd4e 100644 --- a/git-proxy-java-dashboard/frontend/src/pages/UserDetail.tsx +++ b/git-proxy-java-dashboard/frontend/src/pages/UserDetail.tsx @@ -576,7 +576,7 @@ function AddPermissionModal({ const [providers, setProviders] = useState([]) const [provider, setProvider] = useState('') const [path, setPath] = useState('') - const [pathType, setPathType] = useState<'LITERAL' | 'GLOB' | 'REGEX'>('LITERAL') + const [pathType, setPathType] = useState<'LITERAL' | 'GLOB' | 'REGEX'>('GLOB') const [operations, setOperations] = useState('PUSH_AND_REVIEW') const [submitting, setSubmitting] = useState(false) const [error, setError] = useState(null) From 411faf0c967fd8629537c164be7728b7d0317f0b Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Tue, 5 May 2026 17:21:27 -0400 Subject: [PATCH 4/4] fix: update ConfigHotReloadE2ETest inline YAML to unified match shape Inline rule snippets still used the old slugs:/operations:[list] shape, causing GestaltException on reload. Updated to match: block with BOTH/PUSH. Co-Authored-By: Claude Sonnet 4.6 --- .../gitproxy/e2e/ConfigHotReloadE2ETest.java | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/ConfigHotReloadE2ETest.java b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/ConfigHotReloadE2ETest.java index fc648608..ae393133 100644 --- a/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/ConfigHotReloadE2ETest.java +++ b/git-proxy-java-server/src/test/java/org/finos/gitproxy/e2e/ConfigHotReloadE2ETest.java @@ -75,8 +75,10 @@ void setUp() throws Exception { Files.writeString(allowAll, """ rules: allow: - - operations: [PUSH, FETCH] - slugs: ["/**"] + - operations: BOTH + match: + target: SLUG + value: '/**' deny: [] secret-scan: enabled: false @@ -255,10 +257,10 @@ void rulesReload() throws Exception { rules: allow: [] deny: - - operations: - - PUSH - slugs: - - "%s" + - operations: PUSH + match: + target: SLUG + value: '%s' secret-scan: enabled: false """.formatted(repoSlug)); @@ -270,8 +272,10 @@ void rulesReload() throws Exception { Path permissive = writeOverride(""" rules: allow: - - operations: [PUSH, FETCH] - slugs: ["/**"] + - operations: BOTH + match: + target: SLUG + value: '/**' deny: [] secret-scan: enabled: false