[NF] Provisioning API: create members, users and connections over HTTP - #1044
Open
Xen0n6291 wants to merge 10 commits into
Open
[NF] Provisioning API: create members, users and connections over HTTP#1044Xen0n6291 wants to merge 10 commits into
Xen0n6291 wants to merge 10 commits into
Conversation
…tion Adds a stateless, machine-to-machine API surface for provisioning members, users and connections from an external ordering or billing system. The existing POST endpoints under admin/api/v4 cannot serve this purpose: that route group carries the `web` middleware, and VerifyCsrfToken excepts only `login`. An external caller can therefore only issue GET requests there. This registers a separate route group modelled on RouteServiceProvider::mapApiExternalAuthSuperuserRoutes() - API key auth, no session, no CSRF - under admin/api/v4/provisioning. A ForceJsonResponse middleware runs ahead of the api/v4 group so that a ValidationException yields 422 JSON rather than a 302 redirect, which a machine caller cannot follow. The feature is kept additive: holding the route group in its own service provider means the only change to a pre-existing file is one line registering it in config/app.php. Includes a /ping endpoint and tests covering the whole authentication chain: 200 for a superuser key, 401 without a key, 403 for custadmin and custuser keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds POST /member, GET /member/{cust} and POST /member/{cust}/user.
Validation is inherited rather than restated: the API form requests extend the web
ones and add only a declared DELTA, so a rule changed upstream applies here
automatically. RulesParityTest subtracts that delta and asserts the remainder is
identical, turning any future divergence into a failing test at merge time.
The delta covers fillable attributes the web request does not validate. Three are
worth naming: the columns are `dateleave`, `MD5Support` and `isReseller`, while the
web rules refer to `dateleft` and `md5support` - those two rules match nothing and
the values reach Customer::create() unvalidated. The web rules are left alone; the
API validates the names the schema actually uses.
The controllers mirror CustomerController@store and UserController@store rather
than calling them, since those return redirects. MemberContractTest posts the same
payload to both entry points and diffs the resulting rows, so the mirroring cannot
drift unnoticed.
Two deliberate differences from the web path:
- Both creations run in a transaction. The web path does not, so a failure there
can leave orphaned company detail records behind.
- The user endpoint takes `enabled` rather than the web form's `disabled`, whose
value the web controller inverts. UserCreatedEvent fires only after the
transaction commits, so a later failure cannot send a welcome email for a user
that was rolled back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the endpoints an ordering system needs to place a member on the fabric:
GET /switch, /infrastructure, /port/free
GET /vlan/{vlan}/address
POST /member/{cust}/connection
GET /member/{cust}/connection
DELETE /connection/{vi}
ConnectionController extends Interfaces\Common rather than the API base controller.
That is the point of it: setIp() and deletePi() are inherited, so address assignment
and the fanout-aware teardown exist once and keep working when upstream changes
them. Only the orchestration storeWizard() wraps in redirects is reimplemented.
New behaviour with no upstream equivalent:
- IpAllocator resolves "auto" to the next free address in the VLAN. The web wizard
requires an explicit address because a human picks one from a list rendered in
the browser; unattended provisioning has no list. Selection takes a row lock
inside the caller's transaction so two concurrent orders cannot be handed the
same address, with the unique indexes on vlaninterface as the backstop.
- Addresses must already exist in the VLAN's pool. setIp() creates one on demand,
which for an unattended caller turns a typo into a stray address record, so
claim() is consulted first and rejects anything not in the pool.
- port/free walks every active switch. The web UI answers this one switch at a
time because an operator has already chosen it from a dropdown.
Only unset and peering ports are ever offered or accepted: core, management,
monitor, fanout and reseller ports are infrastructure, and handing one to an
ordering system would let it reassign the fabric.
There is deliberately no endpoint to reserve an address on its own. An address held
by nothing but a promise leaks if the caller then fails; allocation happens inside
the connection transaction, where it either sticks or is rolled back with the rest.
StoreConnection separates delta() from overrides() so RulesParityTest can tell an
addition from a deliberate replacement, and assert that every override still
corresponds to a rule upstream actually has.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds POST /onboarding, which creates a member, optionally a portal user and optionally a connection in a single transaction. The three dedicated endpoints already exist and can be called in sequence. This exists because that sequence is not atomic: a caller whose second request fails is left with a member and no user, and no clean way back. Here the whole order lands or none of it does - the rollback test asserts exactly that, including that no welcome email goes out for a user which no longer exists. Sections are nested rather than flattened, because `name` means three different things across a member, a user and a virtual interface. Each section is validated with the rules of its own endpoint, re-keyed under its prefix, so there is still one definition of what a valid member is. Idempotency is opt-in via `reference`, the caller's own order identifier. A retry after a timeout returns the original result instead of a second member. The check runs in middleware rather than the controller, because validation happens first and would otherwise reject the retry with "shortname already taken" - true, but useless to a caller asking what became of its order. The controller keeps its own check for the case of two simultaneous retries. References live in their own table rather than a column on `cust`, so that merging an upstream release never has to reconcile a schema change of ours. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documents the endpoints, the authentication model and the deliberate differences from the web UI. Two operational points are called out because neither is obvious from the code: API keys expire after at most twelve months, so an unattended service fails with 401 once a year unless the key is rotated; and `allowed_ips` on an API key is not enforced anywhere, so the endpoint has to be restricted at the web server or firewall. Placed under docs/ for now. Upstream keeps documentation in a separate repository (inex/ixp-manager-docs-md) and asks for a parallel pull request there, so this file is the source for that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An adversarial review of the preceding commits produced 31 confirmed findings.
The two serious ones both let a caller reach state the web UI refuses.
Address required when the family is enabled.
StoreConnection::overrides() replaced the wizard's conditional
`ipv4|required` with a flat `nullable`, dropping the requirement along with the
IP type. A caller could then create a VLAN interface with ipv4enabled=1 and no
address. That row survives into the route server neighbour list - it is skipped
only when disabled - and renders as `neighbor as 65551;`, which BIRD cannot
parse. The generated configuration for the entire VLAN is rejected, not merely
that peer. overrides() is now instance-scoped and keeps the requirement while
still admitting "auto".
Onboarding ran no validation hooks.
Collecting rules() out of the sub-requests does not carry their withValidator()
hooks across: Laravel invokes the hook of the request it is validating. So
User\Store's guard - the only thing refusing AUTH_SUPERUSER on a non-internal
customer - never applied, and an order could create a full IXP Manager
administrator on an arbitrary member, with the welcome email delivering the
password-reset link. Customer\Store::checkReseller() was likewise absent, so a
resold member with no reseller was silently created un-resold. Both checks are
now restated in StoreOnboarding, evaluated against the customer being created,
because the upstream hooks look up a customer which does not exist yet.
Also fixed:
- Onboarding wrote the raw request array into the models, so any fillable column
without a rule (lastupdatedby, channelgroup, notes) was caller-writable. It
now uses validated() throughout, as the dedicated endpoints already did.
- A repeated reference racing itself produced an unhandled unique-constraint
violation; it now resolves to the first result, or 409 if that member is gone.
- An array where an address string belongs raised a 500 on the string cast.
- Onboarding did not check that the switch port belongs to the switch named.
Both paths now share ConnectionController::switchPortRejection().
- port/free reported truncated=true on a result which exactly filled the limit.
- Listing connections was N+1; the response relations are eager loaded.
- An `id` in a user body could steer the username uniqueness rule.
- The provider docblock claimed there is no session cookie. There is: apibase
starts a session and ApiAuthenticate only looks for a key when Auth::check()
is false. Documented rather than overstated.
- data/ci/ci_test_db.sql now carries the new table and its migration row, so
upstream CI - which loads that dump instead of migrating - stays green.
Tests: 49 -> 75. The new ones are regression tests for each defect above, plus
the coverage the review found missing: IPv6-only and dual-stack connections,
rate_limit/autoneg/LAG persistence, repeated deletion, and the address listing
endpoint, which had none at all. Three existing tests asserted nothing useful -
an empty if block, a comment claiming an address returns to the pool while
checking something else, and an ordering check a pool of that shape cannot fail -
and now test what they claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebasing onto main from v7.3.1 made RulesParityTest fail on two counts, which is what it is for: main added `rate_limit` and `autoneg` to StoreVirtualInterfaceWizard after v7.3.1, and the API request was still declaring them as additions of its own. They are now inherited like every other rule. Nothing else changes - the fields were already accepted and persisted; only the question of who defines them moves upstream, which is the better place for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…limited
These are the only endpoints in IXP Manager which let an external caller create
business objects, and the v7 line has spent four releases shrinking exactly that
surface. So they now default to not existing at all, and an operator who wants
them decides how far to open them.
Four independent guards, configured under ixp_api.provisioning:
- enabled (default false). Without it the routes are never registered. An
upgrade cannot quietly expose an installation to something it did not ask
for, and there is nothing to secure because there is nothing there.
- require_api_key (default true). `apibase` starts a session and
ApiAuthenticate only looks for a key when Auth::check() is false, so a
browser already logged in as a superuser reached these endpoints on its
cookie - and, `web` being absent, without a CSRF token. Tolerable for the
read-only export endpoints this group is modelled on; not for endpoints
which create members. The previous commit documented that; this one closes
it.
- allowed_ips, globally and per key. The per-key list honours
api_keys.allowed_ips - a column which has existed for years, is exposed in
the key management UI, and which nothing in IXP Manager has ever read.
- rate_limit (default 60/min), placed after assert.privilege so the count is
per key rather than per source address.
None of this replaces restricting /admin at the web server; it is a second lock
on the same door, for installations behind a proxy whose ACL they do not own.
Also fixes three defects found while reviewing the running code:
- The unique-constraint handler assumed every violation came from the
reference index. A second order taking the same switch port hits
physicalinterface.switchportid instead, and the caller was told to "use a
new reference" - which would burn an order number and retry onto the same
occupied port. The two cases are now distinguished.
- A switch taken out of service between listing free ports and ordering was
accepted. The connection would exist but appear in neither the switch
configuration nor the grapher, with nothing raising an alarm.
- Transactions ran with a single attempt, so a deadlock surfaced as a 500.
GET admin/api/v4/user/json answered with User::byPrivs()->get()->toArray(), and the
User model declared no $hidden - so the response carried the bcrypt hash of every
user of every customer.
That route is in routes/apiv4-ext-auth-superuser.php, which is the group without
`web`: reachable with an API key and with no CSRF token in the way. A superuser key
was therefore enough to retrieve every password hash in the installation.
Two layers, because either alone leaves something open:
- $hidden on the model covers anything which serialises a user, not just the one
endpoint that was found to leak. It affects serialisation only, so direct
access keeps working - which matters, because authentication reads the
attribute through getAuthPassword(), and the console server and RADIUS
templates under resources/views/api/v4/user/formatted/ emit it deliberately.
- The endpoint now names the columns it returns. A column added later, and
whatever sensitivity it carries, will not appear there by accident.
Tests assert both the absence in serialised output and that the formatted endpoint
still emits what it exists for.
Note there is no `remember_token` column on this table, so nothing else needed
hiding.
…ad drifted
Adds docs/provisioning-api.openapi.yaml - OpenAPI 3.1, hand written, no generator
package and no build step. 11 paths, 22 schemas. Every field is derived from the
controllers and from the merged validation rules, including those inherited from
Customer\Store, User\Store and StoreVirtualInterfaceWizard, so the document
describes what the code does rather than what the prose claimed.
Writing it surfaced four places where docs/provisioning-api.md had fallen out of
step with the code, two of them because this branch changed and the prose did not:
- It still said allowed_ips is enforced nowhere and throttling is commented out.
Both were true when written and are not now: RestrictSourceAddress reads the
column and the route group carries a throttle.
- It still listed rate_limit and autoneg as additions of ours. They were, until
the rebase from v7.3.1 onto main, where upstream had added them to the wizard
request itself - which is precisely what RulesParityTest failed on.
- Being off by default was documented only in the config file and the provider
docblock, not where a reader would look for it.
- The conventions mentioned PUT, of which there is none.
Two behaviours are now stated because a client would otherwise guess wrong: 401 and
403 from the upstream authentication middleware are plain text, not JSON, so an
auth failure cannot be parsed as JSON; and a taken switch port at /onboarding
answers 422 rather than 409, since it travels as an IpAllocationException.
Contributor
|
Hey @Xen0n6291, thanks for opening this PR, it's huge, a lot of work must have went into it! We'll have a chat about it and write back soon, just wanted to acknowledge the PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[NF] Provisioning API: create members, users and connections over HTTP
Why
API v4 is, in practice, a read and export surface. It publishes switch configuration,
route server configuration, Nagios targets and DNS data — but nothing in it creates a
member, a portal user or a port. That logic lives only in the web controllers, so an IXP
wanting to provision from an ordering or billing system has to either drive the web UI or
write to the database directly, and writing directly means losing the validation, the
events, the cache invalidation and the IP collision checks that make those controllers
correct.
At KleyReX we want an order to provision itself end to end. This is the part of that which
belongs in IXP Manager; everything else (switch rollout, route server reload, rDNS) stays
outside and talks to these endpoints.
Worth noting for anyone who has tried: the existing POST endpoints under
admin/api/v4cannot be called from outside a browser at all. That group carries the
webmiddleware andVerifyCsrfTokenexcepts onlylogin, so an external caller can issue GET and nothingelse. This PR therefore adds its own route group, modelled on
mapApiExternalAuthSuperuserRoutes().What
Eleven endpoints under
admin/api/v4/provisioning, all requiring a superuser API key:How it stays consistent with the web UI
This is the part I would most like reviewed, because it is the part that decides whether
the two drift apart later.
Logic is inherited, not copied.
ConnectionControllerextendsHttp\Controllers\Interfaces\Common, sosetIp()and the fanout-awaredeletePi()arethe same code the web UI runs. Only the orchestration that
storeWizard()wraps inredirects is reimplemented.
Validation is inherited too. The API form requests extend the web ones and add a
declared
delta().RulesParityTestsubtracts that delta and asserts the remainder isidentical to the web rules. It has already earned its place: rebasing this branch from
v7.3.1 onto main failed exactly there, because main had since added
rate_limitandautonegto the wizard request — see the last commit.Where duplication was unavoidable —
Customer::create()andUser::create(), about adozen lines each — a contract test posts the same payload to both entry points and diffs
the resulting rows attribute by attribute.
Deliberate differences from the web path
ipv4address/ipv6addressaccept"auto", resolved to the next free address in theVLAN. The wizard requires an explicit address because an operator picks one from a list
rendered in the browser; an unattended caller has no list. The conditional requirement is
kept — an enabled address family still requires an address.
setIp()creates one ondemand, which for an unattended caller turns a typo into a stray address record.
can leave orphaned company detail records behind.
enabledrather than the web form'sdisabled, whose valueUserController@storeinverts.Notes for review
config/app.php.Everything else is additive.
data/ci/ci_test_db.sqlalso carries the new table and itsmigration row, so CI keeps loading rather than migrating.
of each field it changes so that restoring is exact, and there is no such place in the
schema. That seemed worth discussing separately rather than inventing here.
if the caller then fails. Allocation happens inside the connection transaction.
apibasestarts a session andApiAuthenticateonly looks for a key whenAuth::check()is false, so a logged-insuperuser's browser reaches these routes on its cookie without a key and — since
webisabsent — without a CSRF token. That is inherited from the existing external route group
rather than introduced here, and it is documented in the provider docblock. If you would
rather these routes were key-only, say so and I will add a middleware for it.
Testing
75 tests in
tests/Api/Provisioning/, covering the auth chain, contract parity with the webUI, rule parity, IPv6-only and dual-stack connections, LAG persistence, concurrent address
allocation, rollback on partial failure, and idempotent retries.
An adversarial review over the finished branch produced 31 confirmed findings, all fixed in
provisioning api: fix defects found by review. Two were serious enough to name here: anoverride had dropped the "address required when the family is enabled" condition, which
would have produced
neighbor as 65551;in the generated route server config and broken itfor the whole VLAN; and the composite endpoint collected
rules()from its sub-requestswithout their
withValidator()hooks, so the guard refusingAUTH_SUPERUSERon anon-internal customer never applied. Both have regression tests.
In addition to the above, I have:
Bgpq3Testand
Bgpq4Test, which need thebgpq3/bgpq4binaries and fail identically on aclean checkout of main here
added to
psalm-baseline.xmlevery response is JSON built from an explicit field list rather than
toArray()assert.privilege:AUTH_SUPERUSERon the group, inheritedauthorize()on eachrequest, and the superuser-privilege guard restated for the composite endpoint
PUT or DELETE. CSRF tokens do not apply: the group deliberately omits
web, asmapApiExternalAuthSuperuserRoutes()does, because a machine caller cannot obtainone. See the session caveat above.
Documentation is in
docs/provisioning-api.mdin this branch; I will open the parallel PRagainst
ixp-manager-docs-mdonce you have had a look at the shape of this.The CLA has not been sent yet. I am aware nothing can be merged until it has been, and will
follow up — I opened this now so the approach can be discussed before more is built on it.
If you would rather review only after the CLA is on file, say so and I will close this and
reopen once it is.