Complete rewrite of the library #50
Conversation
|
@AndrewDryga thank you, there is a lot to review but I don't think this is in a state to review yet. There are references to Firezone and modules that don't exist. Can you please restore the references to DockYard, remove the references to Firezone? As I noted adding Firezone as a collaborator makes sense but not some of the changes as they're here |
|
@bcardarella I reverted all the changes and updated README.md to reflect new function interfaces. |
The library was storing a state in a GenServer which is causing various issues: 1. When configuration is changed we needed to restart the process having a small OIDC downtime during the process; 2. There were no way to run async tests because GenServer was querying discovery document URL's on the background, which means we wasn't able to use Bypass in integration tests 3. Storing a lot of configuration on the library side would cause a lot of issues once we have multi-tenancy and a ton of different configuration, the GenServer will be a bottleneck.
|
@AndrewDryga awesome, thanks! @xn can we try this branch out on dockyard admin and confirm? I'll also do a code review |
|
@AndrewDryga I think you've addressed all of the code review issues. I want to wait on our own internal test of this branch replacing the previous version of the lib before merging. Thanks! |
AndrewDryga
changed the title
| @@ -120,14 +98,17 @@ get("/session/authorization-uri", SessionController, :authorization_uri) | |||
| # session_controller.ex | |||
| # you could also take the `provider` as a query param to pass into the function | |||
| def authorization_uri(conn, _params) do | |||
| json(conn, %{uri: OpenIDConnect.authorization_uri(:google)}) | |||
| google_config = Application.fetch_env!(:my_app, :google_oidc_config) | |||
| json(conn, %{uri: OpenIDConnect.authorization_uri(google_config)}) | |||
There was a problem hiding this comment.
authorization_uri/1 returns an :ok tuple.
We want something like:
def authorization_uri(conn, _params) do
google_config = Application.fetch_env!(:my_app, :google_oidc_config)
{:ok, uri} = OpenIDConnect.authorization_uri(google_config)
json(conn, %{uri: uri})
end
|
@AndrewDryga This is great stuff! Really appreciate the work! One small documentation fix and it looks good to me. |
|
@AndrewDryga if you can make the requested documentation change ☝️ we can get this merged |
|
Ah sorry guys, I did not notice the notification about that doc change comment. Thank you! Glad to see it merged. |
|
Should I open a different PR for the doc change? |
|
No worries, I just opened #51 |
|
Thank you so much for all of your hard and excellent work! |
|
@AndrewDryga no worries, thank you for the PR 🎉 |
|
|
||
| defp build_document(document_json) do | ||
| keys = Map.keys(document_json) | ||
| required_keys = ["jwks_uri", "authorization_endpoint", "token_endpoint"] |
There was a problem hiding this comment.
A bit late to the party but should this also include "userinfo_endpoint" given that it uses Map.fetch! on this key as well? OR should it not use fetch! given that it doesn't seem to be required by the RFC? 🤔
| token_endpoint: Map.fetch!(document_json, "token_endpoint"), | ||
| userinfo_endpoint: Map.fetch!(document_json, "userinfo_endpoint"), | ||
| response_types_supported: | ||
| Map.get(document_json, "response_types_supported") |
There was a problem hiding this comment.
also should this default to []? Map.get(document_json, "response_types_supported", []). Otherwise if it's nil it will fail on Enum.map I think? 🤔
This PR addresses several issues we faced while using the official version:
I know this is a lot to review, it has breaking changes (hence the major version bump), but this code is battle tested at https://github.com/firezone/firezone on thousands of instances.
This also resolves most of the PR's and issues opened, I went through them and made a list so that once PR is merged they are closed automatically:
Closes #45
Closes #39
Closes #36
Closes #49
Closes #44
Closes #42
Closes #37
Closes #28
Closes #40
Closes #35
Closes #31
Closes #29
Closes #24