Update/intialize the access token with elixir starter kit #12
Conversation
WalkthroughThe changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KindeClientSDK
participant API
Client->>KindeClientSDK: init_with_tokens(conn, access_token, refresh_token)
KindeClientSDK-->>Client: Store tokens in assigns
Client->>KindeClientSDK: authenticated_request(conn)
KindeClientSDK->>KindeClientSDK: get_kinde_client(conn)
KindeClientSDK-->>Client: Retrieve access token
KindeClientSDK->>API: Make request with Bearer token
API-->>KindeClientSDK: Response
KindeClientSDK-->>Client: Return response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/kinde_sdk/kinde_client_sdk.ex (1 hunks)
Additional comments not posted (1)
lib/kinde_sdk/kinde_client_sdk.ex (1)
1013-1015: LGTM!The
get_kinde_client/1function efficiently retrieves the client from the connection's assigns.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/kinde_sdk/kinde_client_sdk.ex (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/kinde_sdk/kinde_client_sdk.ex
|
approved |
lib/kinde_sdk/kinde_client_sdk.ex
Outdated
| end | ||
|
|
||
|
|
||
| def authenticated_request(conn, method, url, params \\ %{}) do |
There was a problem hiding this comment.
I'm not sure what this method is supposed to do?
It takes a method, url and params but doesn't do anything with it, and just returns headers?
There was a problem hiding this comment.
Its been resolved and pushed.
|
To be honest, I'm not sure how this PR has to be used. In the light of #11, let's say I have the access and refresh tokens. conn =
KindeClientSDK.init_with_tokens(
conn,
get_session(conn, :kinde_access_token),
get_session(conn, :kinde_refresh_token)
)
dbg(KindeClientSDK.authenticated?(conn))returns false?? How do I get that to return true? Or can you show me some code how to use |
|
Just to be clear, if the |
|
Thanks for your commends @tcoopman, are you able to address these please @Hammadkhan0034 ? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/kinde_sdk/kinde_client_sdk.ex (1 hunks)
Additional comments not posted (3)
lib/kinde_sdk/kinde_client_sdk.ex (3)
1001-1022: Review ofinit_with_tokens/3: Enhanced token initialization.The function
init_with_tokenscorrectly initializes the connection with access and refresh tokens. The error handling for nil tokens is implemented as suggested in previous comments, ensuring robustness. The documentation is clear and provides all necessary details about the function's purpose and usage.The changes are approved as they meet the requirements for functionality and documentation.
1024-1026: Review ofget_kinde_client/1: Efficient token retrieval.The function
get_kinde_clientretrieves the token map from the connection efficiently. The implementation is straightforward and uses the Elixir'sassignsmap correctly to fetch the stored client.The changes are approved as they correctly implement the token retrieval functionality.
1028-1037: Review ofauthenticated_request/4: Proper use of access tokens in requests.The function
authenticated_requesthas been updated to utilize theget_kinde_clientfunction for retrieving the client and its access token. The error handling for missing client or access token is appropriate, preventing potential runtime errors. The construction of the authorization header using the access token is correctly implemented.The changes are approved as they enhance the security and functionality of making authenticated HTTP requests.
Hammadkhan0034
force-pushed
the
persisted_tokens
branch
from
c0cb0e4 to
08c569e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- lib/kinde_sdk/kinde_client_sdk.ex (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- lib/kinde_sdk/kinde_client_sdk.ex
|
Yes if the valid refresh token is available in cache, you can use it to obtain new token, if both cache and refresh token are unavailable or expires then user will need re-authentication. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
lib/kinde_sdk/kinde_client_sdk.ex (1)
1001-1040: Enhance overall consistency and completeness of new functions.The new functions (
init_with_tokens/3,get_kinde_client/1, andauthenticated_request/1) add valuable token management and authentication capabilities to the SDK. However, there are some inconsistencies in documentation and error handling that should be addressed:
init_with_tokens/3has detailed documentation, while the other functions lack it.- Error handling approaches differ between functions.
- The
authenticated_request/1function is incomplete compared to its intended functionality.To improve the overall quality and consistency of the SDK:
- Ensure all public functions have clear and consistent documentation.
- Standardize error handling approaches across functions.
- Complete the implementation of
authenticated_request/1as suggested in the previous comment.- Consider adding unit tests for these new functions to ensure they work as expected and to prevent regressions in future updates.
By addressing these points, you'll significantly improve the usability and reliability of the SDK for developers integrating with Kinde.
| def get_kinde_client(conn) do | ||
| conn.assigns[:kinde_client] | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and error handling to get_kinde_client/1.
The function lacks documentation and error handling, which could be improved for better usability and robustness.
Consider updating the function as follows:
@doc """
Retrieves the Kinde client from the connection's assigns.
## Parameters
- `conn`: The connection containing the Kinde client.
## Returns
- The Kinde client if found, or nil if not present.
## Examples
client = get_kinde_client(conn)
"""
def get_kinde_client(conn) do
conn.assigns[:kinde_client]
endThis change adds clear documentation for the function. Note that the function returns nil if the client is not found in the assigns, which is the default behavior of accessing a map with []. If you want to handle this case differently, you could add explicit error handling:
def get_kinde_client(conn) do
case conn.assigns[:kinde_client] do
nil -> raise "Kinde client not found in connection"
client -> client
end
endChoose the approach that best fits your error handling strategy.
| def authenticated_request(conn) do | ||
| client = get_kinde_client(conn) | ||
|
|
||
| if is_nil(client) or is_nil(client.access_token) do | ||
| raise "Kinde Client or access token is missing" | ||
| end | ||
|
|
||
| access_token = client.access_token | ||
| headers = [{"Authorization", "Bearer #{access_token}"}] | ||
| end |
There was a problem hiding this comment.
Incomplete implementation of authenticated_request/1.
The current implementation of authenticated_request/1 has several issues:
- It lacks documentation.
- It doesn't use the
method,url, andparamsparameters mentioned in a previous comment. - It creates an authorization header but doesn't actually make a request.
To address these issues, consider the following improvements:
- Add documentation to explain the function's purpose and parameters.
- Implement the actual request using the
method,url, andparams. - Return the response from the authenticated request.
Here's a suggested implementation:
@doc """
Makes an authenticated request using the Kinde client's access token.
## Parameters
- `conn`: The connection containing the Kinde client.
- `method`: The HTTP method for the request (e.g., :get, :post).
- `url`: The URL for the request.
- `params`: Optional parameters for the request (default: %{}).
## Returns
- The response from the authenticated request.
## Raises
- RuntimeError if the Kinde client or access token is missing.
"""
def authenticated_request(conn, method, url, params \\ %{}) do
client = get_kinde_client(conn)
if is_nil(client) or is_nil(client.access_token) do
raise "Kinde Client or access token is missing"
end
headers = [{"Authorization", "Bearer #{client.access_token}"}]
# Make the actual request using a HTTP client library (e.g., HTTPoison)
case HTTPoison.request(method, url, Jason.encode!(params), headers) do
{:ok, response} -> response
{:error, error} -> raise "Request failed: #{inspect(error)}"
end
endThis implementation completes the function by making the actual authenticated request and handling the response.
|
|
||
| @doc """ | ||
| Initializes the connection with access and refresh tokens. | ||
|
|
||
| ## Parameters | ||
| - `conn`: The connection to assign the tokens to. | ||
| - `access_token`: The access token to be stored. | ||
| - `refresh_token`: The refresh token to be stored. | ||
|
|
||
| ## Returns | ||
| - The updated connection with assigned tokens. | ||
| """ | ||
| def init_with_tokens(conn, access_token, refresh_token) do | ||
| if is_nil(access_token) or is_nil(refresh_token) do | ||
| raise ArgumentError, "Tokens cannot be nil" | ||
| end | ||
|
|
||
| client = %{ | ||
| access_token: access_token, | ||
| refresh_token: refresh_token | ||
| } | ||
|
|
||
| Conn.assign(conn, :kinde_client, client) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider additional validation for empty string tokens.
The function correctly checks for nil tokens, but it might be beneficial to also check for empty strings. This would prevent potential issues with empty tokens being treated as valid.
Consider updating the validation as follows:
- if is_nil(access_token) or is_nil(refresh_token) do
- raise ArgumentError, "Tokens cannot be nil"
+ if is_nil(access_token) or access_token == "" or is_nil(refresh_token) or refresh_token == "" do
+ raise ArgumentError, "Tokens cannot be nil or empty"
endThis change ensures that both nil and empty string tokens are caught, improving the robustness of the function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @doc """ | |
| Initializes the connection with access and refresh tokens. | |
| ## Parameters | |
| - `conn`: The connection to assign the tokens to. | |
| - `access_token`: The access token to be stored. | |
| - `refresh_token`: The refresh token to be stored. | |
| ## Returns | |
| - The updated connection with assigned tokens. | |
| """ | |
| def init_with_tokens(conn, access_token, refresh_token) do | |
| if is_nil(access_token) or is_nil(refresh_token) do | |
| raise ArgumentError, "Tokens cannot be nil" | |
| end | |
| client = %{ | |
| access_token: access_token, | |
| refresh_token: refresh_token | |
| } | |
| Conn.assign(conn, :kinde_client, client) | |
| end | |
| @doc """ | |
| Initializes the connection with access and refresh tokens. | |
| ## Parameters | |
| - `conn`: The connection to assign the tokens to. | |
| - `access_token`: The access token to be stored. | |
| - `refresh_token`: The refresh token to be stored. | |
| ## Returns | |
| - The updated connection with assigned tokens. | |
| """ | |
| def init_with_tokens(conn, access_token, refresh_token) do | |
| if is_nil(access_token) or access_token == "" or is_nil(refresh_token) or refresh_token == "" do | |
| raise ArgumentError, "Tokens cannot be nil or empty" | |
| end | |
| client = %{ | |
| access_token: access_token, | |
| refresh_token: refresh_token | |
| } | |
| Conn.assign(conn, :kinde_client, client) | |
| end |
Explain your changes
This Pull request introduces a more structured approach to handling authentication tokens within the application by adding three new functions: init_with_tokens/3, get_kinde_client/1, and authenticated_request/4. These changes improve how access and refresh tokens are managed within the connection (conn).