REST API #1155
Merged
REST API #1155
Conversation
This file contains 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
jamilbk
force-pushed
the
feat/rest-api
branch
from
2d9c2b7
to
6bed6c9
Compare
AndrewDryga
reviewed
Dec 6, 2022
AndrewDryga
reviewed
Dec 6, 2022
jamilbk
force-pushed
the
feat/rest-api
branch
from
6bed6c9
to
a0b570b
Compare
AndrewDryga
reviewed
Dec 11, 2022
AndrewDryga
reviewed
Dec 11, 2022
apps/fz_http/lib/fz_http_web/live/setting_live/unprivileged/account_live.ex
Outdated
Show resolved
Hide resolved
jamilbk
force-pushed
the
feat/rest-api
branch
3 times, most recently
from
d0fc3a3
to
791b180
Compare
This was referenced Dec 22, 2022
jamilbk
force-pushed
the
feat/rest-api
branch
from
bfc0ff5
to
d7c7e4b
Compare
jamilbk
added a commit
that referenced
this pull request
Dec 26, 2022
* Adds views for managing ApiTokens * ApiTokens have a max count of 10 per user * JWT is generated on the fly when "viewing" an ApiToken. The JWT is never stored in the DB There are still a few things left for the REST API. Will update #1155 with the TODOs. https://user-images.githubusercontent.com/167144/209414501-1d7ebdba-15fa-4d6f-9072-4ed6bf7dd1b5.mov
jamilbk
force-pushed
the
feat/rest-api
branch
2 times, most recently
from
1f7a9bd
to
4947f66
Compare
* 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)
jamilbk
force-pushed
the
feat/rest-api
branch
from
4947f66
to
f4b4fbc
Compare
|
@AndrewDryga This is ready for final review before going into The remaining items in the TODO will be handled in separate PRs. |
jamilbk
force-pushed
the
feat/rest-api
branch
from
f4b4fbc
to
56b516a
Compare
Co-authored-by: Andrew Dryga <andrew@dryga.com> Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
AndrewDryga
reviewed
Dec 27, 2022
| What a time to be a developer, right? | ||
| """ | ||
| # credo:disable-for-next-line Credo.Check.Readability.MaxLineLength | ||
| @ip_regex ~r/((^\s*((([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5]))\s*$)|(^\s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\s*$))/ |
There was a problem hiding this comment.
There is a nice way to write long regexes especially if they repeatedly use the same patterns (like IP and CIDR would be almost the same, plus address segments are the same):
iex(1)> seg = "[0-9]*" "[0-9]*" iex(2)> Regex.compile!(seg)
~r/[0-9]*/
iex(3)> Regex.compile!(seg <> seg)
~r/[0-9]*[0-9]*/
So you can define module attributes like @ipv4_address_segment and @ipv6_address_segment and then reuse them multiple time in ip/cidr regexes reducing their complexity greately.
There was a problem hiding this comment.
^ this is just FYI, it's a lot of work to split this monster now and make sure it's valid
AndrewDryga
reviewed
Dec 27, 2022
| def create_user_api_token(%FzHttp.Users.User{} = user, attrs) do | ||
| changeset = | ||
| attrs | ||
| |> Enum.into(%{user_id: user.id}) |
There was a problem hiding this comment.
We can just pass it to the changeset and use put_assoc there. This will make the changeset code more explicit like we did here in the context. It looks even nicer because we have a changeset function per action, so their argument is different and the update changeset won't even accept the user anymore as the user can't be changed.
There was a problem hiding this comment.
Later having attributes like this one will be handy so we can use them for auth checks, eg. if auth subject has permission to manage user tokens.
AndrewDryga
reviewed
Dec 27, 2022
Comment on lines
+45
to
+46
| # At the moment it's not a big concern. Fixing it would require locking against INSERTs or DELETEs | ||
| # while counts are happening. |
There was a problem hiding this comment.
If we leave a TODO I think it's better to leave a better design suggestion if somebody decided to tackle this.
Locking the entire ApiTokens table would be really bad if we have a large environment or cloud panel for multiple clients, instead, we can lock the user.id record in users table preventing all user changes as a side effect but then the lock won't affect other users using the table.
Suggested change
| # At the moment it's not a big concern. Fixing it would require locking against INSERTs or DELETEs | |
| # while counts are happening. | |
| # At the moment it's not a big concern. Fixing it would require locking against user record in `users` table | |
| # while counts are happening. |
This removes the ETS-backed cache entirely. Caches are tricky to scale horizontally and are likely not needed at this point in our product roadmap. We can revisit at a later point when performance becomes an issue. Fixes #1240
Closed
AndrewDryga
previously approved these changes
Dec 27, 2022
AndrewDryga
reviewed
Dec 28, 2022
| field :openid_connect_providers, :map | ||
| field :saml_identity_providers, :map | ||
| field :openid_connect_providers, :map, read_after_writes: true | ||
| field :saml_identity_providers, :map, read_after_writes: true |
There was a problem hiding this comment.
Why do we want to read them after writing? Won't the result be the same?
jamilbk
force-pushed
the
feat/rest-api
branch
from
2c02ce8
to
d702b1d
Compare
AndrewDryga
previously approved these changes
Dec 28, 2022
This was referenced Dec 31, 2022
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.
TODO
sitesfields toconfigurationsso they can be updated from APIcachex#1240 -- it introduces possible race conditions for API requestsopenid_connect_providersandsaml_identity_providerssubfields #1249