REST API: Token migration #1149

jamilbk · 2022-11-26T03:32:52Z

Adds a new table, api_tokens, that creates a placeholder for a token.

The token itself will be a JWT generated and shown to the user only once -- this table will hold its contained uuid and creation timestamp to allow the user to revoke the token at a later time.

Revoking the token consists sets the revoked_at timestamp, preventing the token from being used further.

Revoked tokens can then be sweeped from the DB at a later time.

apps/fz_http/priv/repo/migrations/20221126044850_create_api_tokens.exs

jamilbk · 2022-11-29T00:18:16Z

apps/fz_http/lib/fz_http/users/user.ex

@@ -35,6 +39,7 @@ defmodule FzHttp.Users.User do

    has_many :devices, Device, on_delete: :delete_all
    has_many :oidc_connections, Connection, on_delete: :delete_all
+    has_many :api_tokens, ApiToken, on_delete: :delete_all


@akrousset Doing the cascade delete here.

@akrousset Actually, hmmm -- looks like this may be incorrect and needs cascade delete on the table too.

Yeah I saw that when doing my initial review, but quickly hit the Viewed checkbox and forgot about it by the time I got to the migration file. Woops!

Oh, hmm, yeah, sounds like the migration will need it.

K, fixed this and the existing migrations. From docs:

:on_delete - The action taken on associations when parent record is deleted. May be :nothing (default), :nilify_all and :delete_all. Using this option is DISCOURAGED for most relational databases. Instead, in your migration, set references(:parent_id, on_delete: :delete_all). Opposite to the migration option, this option cannot guarantee integrity and it is only triggered for Ecto.Repo.delete/2 (and not on Ecto.Repo.delete_all/2) and it never cascades. If posts has many comments, which has many tags, and you delete a post, only comments will be deleted. If your database does not support references, cascading can be manually implemented by using Ecto.Multi or Ecto.Changeset.prepare_changes/2.

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior Remove unneeded functions, add tests REST API: Controllers and Views (#1148) * Checkpoint controllers and views * Checkpoint Guardian pipeline * Fix tests * Add more JSON views * Create APICase template * Fix partial name * Checkpoint device api tests * Checkpoint tests * Add Rule controller tests Add revoked helpers to ApiTokens Remove unneeded yarn.lock; update package-lock.json Refactors all tests to minimize global state (#1192) Tests were intermittently failing for two reasons: * We were modifying the Application env with `restore_env/3` (a custom function) between tests. This is not thread-safe. * The Conf cache was also being modified between tests. This is a global memory store and is also not thread-safe. This PR aims to mock both of these using Mox in order to provide for a manageable way to test the repurcussions of both Application env and Cache calls.

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

@conectado

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior Remove unneeded functions, add tests REST API: Controllers and Views (#1148) * Checkpoint controllers and views * Checkpoint Guardian pipeline * Fix tests * Add more JSON views * Create APICase template * Fix partial name * Checkpoint device api tests * Checkpoint tests * Add Rule controller tests Add revoked helpers to ApiTokens Remove unneeded yarn.lock; update package-lock.json Refactors all tests to minimize global state (#1192) Tests were intermittently failing for two reasons: * We were modifying the Application env with `restore_env/3` (a custom function) between tests. This is not thread-safe. * The Conf cache was also being modified between tests. This is a global memory store and is also not thread-safe. This PR aims to mock both of these using Mox in order to provide for a manageable way to test the repurcussions of both Application env and Cache calls. Make default IP range CGNAT; randomize IP numbering (#1200) ~~Allocating IP addresses for device creation is not a trivial endeavor.~~ ~~Before, we were using a window function and full-table lock in order to select a candidate device IP to use when creating a new device. This behavior was designed to mimic DHCP.~~ ~~Consider the following example.~~ ~~Say you currently have a list of devices in the database with the following IPs:~~ ~~10.0.0.1 10.0.0.3 10.0.0.100 10.0.0.101~~ ~~Assuming WIREGUARD_IPV4_NETWORK is set to 10.0.0.0/24, the IP address for the next device generated _should_ be 10.0.0.2. Before, this IP would have been found with a large SELECT query that tried to find the first non-sequential IP address in the devices table. This required wrapping the SELECT and following INSERT in a single transaction to ensure the IP address "reserved" would be insertable.~~ ~~As you may imagine, this resulted in deadlocks, and prevented us from running much of the device tests with `async: true` enabled.~~ ~~@conectado came up with a simple but elegant solution to this problem using a Mutex to guard an IP allocation cache. It works as follows:~~ ~~1. Maintain an allocation cache: a list of all of the available IP addresses represented as a list of {gap_start, gap_end} tuples. Using the example data above, this would be: ``` [ {10.0.0.2, 10.0.0.2}, {10.0.0.4, 10.0.0.99}, {10.0.0.102, 10.0.0.254} ] ``` 2. When creating a new device, lock the mutex, then select the first available IP address from the allocation cache. Increment the IP address and save this new state as the new allocation cache. 3. If allocation fails (i.e. the cache is empty), rebuild the cache from the DB. This has the nice effect that a stale cache just skips a few IPs and causes a premature cache rebuild, but doesn't otherwise halt creation like a `SEQUENCE` field would. 4. If the rebuilt cache is **also** empty, the IP address pool actually *has* been exhausted, so return `nil` for the IP. The Device schema interprets this as an error and adds an appropriate error to the changeset.~~ **EDIT**: After further research into Postgres gapless sequences and the information theory realities the problem is bound by, I'm not so sure creating a global mutex to maintain strict consistency for sequential IP allocation is the right approach. Going to explore some simpler methods of IP allocation, likely involving random selection inside the CGNAT range. Co-authored-by: Andrew Dryga <andrew@dryga.com> Remove line committed by accident (#1231) Replace all bigint primary keys with UUIDs (#1233)

@conectado

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior Remove unneeded functions, add tests REST API: Controllers and Views (#1148) * Checkpoint controllers and views * Checkpoint Guardian pipeline * Fix tests * Add more JSON views * Create APICase template * Fix partial name * Checkpoint device api tests * Checkpoint tests * Add Rule controller tests Add revoked helpers to ApiTokens Remove unneeded yarn.lock; update package-lock.json Refactors all tests to minimize global state (#1192) Tests were intermittently failing for two reasons: * We were modifying the Application env with `restore_env/3` (a custom function) between tests. This is not thread-safe. * The Conf cache was also being modified between tests. This is a global memory store and is also not thread-safe. This PR aims to mock both of these using Mox in order to provide for a manageable way to test the repurcussions of both Application env and Cache calls. Make default IP range CGNAT; randomize IP numbering (#1200) ~~Allocating IP addresses for device creation is not a trivial endeavor.~~ ~~Before, we were using a window function and full-table lock in order to select a candidate device IP to use when creating a new device. This behavior was designed to mimic DHCP.~~ ~~Consider the following example.~~ ~~Say you currently have a list of devices in the database with the following IPs:~~ ~~10.0.0.1 10.0.0.3 10.0.0.100 10.0.0.101~~ ~~Assuming WIREGUARD_IPV4_NETWORK is set to 10.0.0.0/24, the IP address for the next device generated _should_ be 10.0.0.2. Before, this IP would have been found with a large SELECT query that tried to find the first non-sequential IP address in the devices table. This required wrapping the SELECT and following INSERT in a single transaction to ensure the IP address "reserved" would be insertable.~~ ~~As you may imagine, this resulted in deadlocks, and prevented us from running much of the device tests with `async: true` enabled.~~ ~~@conectado came up with a simple but elegant solution to this problem using a Mutex to guard an IP allocation cache. It works as follows:~~ ~~1. Maintain an allocation cache: a list of all of the available IP addresses represented as a list of {gap_start, gap_end} tuples. Using the example data above, this would be: ``` [ {10.0.0.2, 10.0.0.2}, {10.0.0.4, 10.0.0.99}, {10.0.0.102, 10.0.0.254} ] ``` 2. When creating a new device, lock the mutex, then select the first available IP address from the allocation cache. Increment the IP address and save this new state as the new allocation cache. 3. If allocation fails (i.e. the cache is empty), rebuild the cache from the DB. This has the nice effect that a stale cache just skips a few IPs and causes a premature cache rebuild, but doesn't otherwise halt creation like a `SEQUENCE` field would. 4. If the rebuilt cache is **also** empty, the IP address pool actually *has* been exhausted, so return `nil` for the IP. The Device schema interprets this as an error and adds an appropriate error to the changeset.~~ **EDIT**: After further research into Postgres gapless sequences and the information theory realities the problem is bound by, I'm not so sure creating a global mutex to maintain strict consistency for sequential IP allocation is the right approach. Going to explore some simpler methods of IP allocation, likely involving random selection inside the CGNAT range. Co-authored-by: Andrew Dryga <andrew@dryga.com> Remove line committed by accident (#1231) Replace all bigint primary keys with UUIDs (#1233)

@conectado

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior Remove unneeded functions, add tests REST API: Controllers and Views (#1148) * Checkpoint controllers and views * Checkpoint Guardian pipeline * Fix tests * Add more JSON views * Create APICase template * Fix partial name * Checkpoint device api tests * Checkpoint tests * Add Rule controller tests Add revoked helpers to ApiTokens Remove unneeded yarn.lock; update package-lock.json Refactors all tests to minimize global state (#1192) Tests were intermittently failing for two reasons: * We were modifying the Application env with `restore_env/3` (a custom function) between tests. This is not thread-safe. * The Conf cache was also being modified between tests. This is a global memory store and is also not thread-safe. This PR aims to mock both of these using Mox in order to provide for a manageable way to test the repurcussions of both Application env and Cache calls. Make default IP range CGNAT; randomize IP numbering (#1200) ~~Allocating IP addresses for device creation is not a trivial endeavor.~~ ~~Before, we were using a window function and full-table lock in order to select a candidate device IP to use when creating a new device. This behavior was designed to mimic DHCP.~~ ~~Consider the following example.~~ ~~Say you currently have a list of devices in the database with the following IPs:~~ ~~10.0.0.1 10.0.0.3 10.0.0.100 10.0.0.101~~ ~~Assuming WIREGUARD_IPV4_NETWORK is set to 10.0.0.0/24, the IP address for the next device generated _should_ be 10.0.0.2. Before, this IP would have been found with a large SELECT query that tried to find the first non-sequential IP address in the devices table. This required wrapping the SELECT and following INSERT in a single transaction to ensure the IP address "reserved" would be insertable.~~ ~~As you may imagine, this resulted in deadlocks, and prevented us from running much of the device tests with `async: true` enabled.~~ ~~@conectado came up with a simple but elegant solution to this problem using a Mutex to guard an IP allocation cache. It works as follows:~~ ~~1. Maintain an allocation cache: a list of all of the available IP addresses represented as a list of {gap_start, gap_end} tuples. Using the example data above, this would be: ``` [ {10.0.0.2, 10.0.0.2}, {10.0.0.4, 10.0.0.99}, {10.0.0.102, 10.0.0.254} ] ``` 2. When creating a new device, lock the mutex, then select the first available IP address from the allocation cache. Increment the IP address and save this new state as the new allocation cache. 3. If allocation fails (i.e. the cache is empty), rebuild the cache from the DB. This has the nice effect that a stale cache just skips a few IPs and causes a premature cache rebuild, but doesn't otherwise halt creation like a `SEQUENCE` field would. 4. If the rebuilt cache is **also** empty, the IP address pool actually *has* been exhausted, so return `nil` for the IP. The Device schema interprets this as an error and adds an appropriate error to the changeset.~~ **EDIT**: After further research into Postgres gapless sequences and the information theory realities the problem is bound by, I'm not so sure creating a global mutex to maintain strict consistency for sequential IP allocation is the right approach. Going to explore some simpler methods of IP allocation, likely involving random selection inside the CGNAT range. Co-authored-by: Andrew Dryga <andrew@dryga.com> Remove line committed by accident (#1231) Replace all bigint primary keys with UUIDs (#1233)

@conectado

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior Remove unneeded functions, add tests REST API: Controllers and Views (#1148) * Checkpoint controllers and views * Checkpoint Guardian pipeline * Fix tests * Add more JSON views * Create APICase template * Fix partial name * Checkpoint device api tests * Checkpoint tests * Add Rule controller tests Add revoked helpers to ApiTokens Remove unneeded yarn.lock; update package-lock.json Refactors all tests to minimize global state (#1192) Tests were intermittently failing for two reasons: * We were modifying the Application env with `restore_env/3` (a custom function) between tests. This is not thread-safe. * The Conf cache was also being modified between tests. This is a global memory store and is also not thread-safe. This PR aims to mock both of these using Mox in order to provide for a manageable way to test the repurcussions of both Application env and Cache calls. Make default IP range CGNAT; randomize IP numbering (#1200) ~~Allocating IP addresses for device creation is not a trivial endeavor.~~ ~~Before, we were using a window function and full-table lock in order to select a candidate device IP to use when creating a new device. This behavior was designed to mimic DHCP.~~ ~~Consider the following example.~~ ~~Say you currently have a list of devices in the database with the following IPs:~~ ~~10.0.0.1 10.0.0.3 10.0.0.100 10.0.0.101~~ ~~Assuming WIREGUARD_IPV4_NETWORK is set to 10.0.0.0/24, the IP address for the next device generated _should_ be 10.0.0.2. Before, this IP would have been found with a large SELECT query that tried to find the first non-sequential IP address in the devices table. This required wrapping the SELECT and following INSERT in a single transaction to ensure the IP address "reserved" would be insertable.~~ ~~As you may imagine, this resulted in deadlocks, and prevented us from running much of the device tests with `async: true` enabled.~~ ~~@conectado came up with a simple but elegant solution to this problem using a Mutex to guard an IP allocation cache. It works as follows:~~ ~~1. Maintain an allocation cache: a list of all of the available IP addresses represented as a list of {gap_start, gap_end} tuples. Using the example data above, this would be: ``` [ {10.0.0.2, 10.0.0.2}, {10.0.0.4, 10.0.0.99}, {10.0.0.102, 10.0.0.254} ] ``` 2. When creating a new device, lock the mutex, then select the first available IP address from the allocation cache. Increment the IP address and save this new state as the new allocation cache. 3. If allocation fails (i.e. the cache is empty), rebuild the cache from the DB. This has the nice effect that a stale cache just skips a few IPs and causes a premature cache rebuild, but doesn't otherwise halt creation like a `SEQUENCE` field would. 4. If the rebuilt cache is **also** empty, the IP address pool actually *has* been exhausted, so return `nil` for the IP. The Device schema interprets this as an error and adds an appropriate error to the changeset.~~ **EDIT**: After further research into Postgres gapless sequences and the information theory realities the problem is bound by, I'm not so sure creating a global mutex to maintain strict consistency for sequential IP allocation is the right approach. Going to explore some simpler methods of IP allocation, likely involving random selection inside the CGNAT range. Co-authored-by: Andrew Dryga <andrew@dryga.com> Remove line committed by accident (#1231) Replace all bigint primary keys with UUIDs (#1233)

Add RevokeToken

f94de09

github-actions bot added the kind/feature New feature or request label Nov 26, 2022

jamilbk added 2 commits November 26, 2022 08:01

Checkpoint UUID ApiToken

91c3d37

Passing tests

042ac5e

jamilbk changed the title ~~REST API: Token Revocation~~ REST API: Token migration Nov 28, 2022

jamilbk requested a review from akrousset November 28, 2022 16:20

jamilbk self-assigned this Nov 28, 2022

github restore cache

77d2185

jamilbk marked this pull request as ready for review November 28, 2022 17:29

akrousset reviewed Nov 28, 2022

View reviewed changes

apps/fz_http/priv/repo/migrations/20221126044850_create_api_tokens.exs Outdated Show resolved Hide resolved

akrousset approved these changes Nov 29, 2022

View reviewed changes

jamilbk commented Nov 29, 2022

View reviewed changes

Fix on_delete behavior

68c9fff

akrousset self-requested a review November 29, 2022 00:43

jamilbk merged commit b1616ac into feat/rest-api Nov 29, 2022

jamilbk deleted the feat/rest-api@revoke-token branch November 29, 2022 00:44

jamilbk added a commit that referenced this pull request Nov 29, 2022

REST API: Token migration (#1149)

6bed6c9

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

jamilbk added a commit that referenced this pull request Dec 7, 2022

REST API: Token migration (#1149)

a11de3b

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

jamilbk added a commit that referenced this pull request Dec 7, 2022

REST API: Token migration (#1149)

a0b570b

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

jamilbk added a commit that referenced this pull request Dec 19, 2022

REST API: Token migration (#1149)

d3797df

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

conectado pushed a commit that referenced this pull request Dec 22, 2022

REST API: Token migration (#1149)

626968a

* Add RevokeToken * Checkpoint UUID ApiToken * Passing tests * github restore cache * Fix on_delete behavior

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REST API: Token migration #1149

REST API: Token migration #1149

jamilbk commented Nov 26, 2022 •

edited

jamilbk Nov 29, 2022

jamilbk Nov 29, 2022

akrousset Nov 29, 2022

akrousset Nov 29, 2022

jamilbk Nov 29, 2022

REST API: Token migration #1149

REST API: Token migration #1149

Conversation

jamilbk commented Nov 26, 2022 • edited

jamilbk Nov 29, 2022

Choose a reason for hiding this comment

jamilbk Nov 29, 2022

Choose a reason for hiding this comment

akrousset Nov 29, 2022

Choose a reason for hiding this comment

akrousset Nov 29, 2022

Choose a reason for hiding this comment

jamilbk Nov 29, 2022

Choose a reason for hiding this comment

jamilbk commented Nov 26, 2022 •

edited