persisting tokens in the session #11
Conversation
WalkthroughThe recent changes enhance authentication and session management in the Changes
Poem
Warning Rate limit exceeded@Hammadkhan0034 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
Outside diff range, codebase verification and nitpick comments (2)
lib/demo_elixir_phoenix_web/router.ex (2)
Line range hint
22-37: Review route organization for clarity and maintainability.The changes organize routes into authenticated and non-authenticated scopes, which enhances clarity. Ensure that all routes are correctly categorized and that no routes requiring authentication are accidentally placed in the non-authenticated scope.
Consider documenting the purpose of each scope for future maintainability.
13-15: Add tests forCheckAuthplug to handle edge casesThe
CheckAuthplug checks for tokens in the session and redirects unauthenticated users. However, it does not explicitly handle edge cases like expired tokens or network issues during SDK initialization. Additionally, there are no automated tests found for this plug. Consider adding tests to verify its behavior under various scenarios.
- File:
lib/demo_elixir_phoenix_web/plugs/check_auth.exAnalysis chain
Ensure
CheckAuthplug is reliable and efficient.The
:authenticatedpipeline uses theCheckAuthplug to enforce authentication. Ensure that the plug efficiently handles all edge cases, such as expired tokens or network issues during SDK initialization.Consider adding tests to verify that the
CheckAuthplug correctly redirects unauthenticated requests and allows authenticated ones.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `CheckAuth` plug correctly handles authentication. # Test: Ensure that the plug is redirecting unauthenticated requests. rg --type elixir -A 5 'plug DemoElixirPhoenixWeb.Plugs.CheckAuth'Length of output: 402
Script:
#!/bin/bash # Locate the implementation of the CheckAuth plug. fd 'CheckAuth.ex' # Search for test files that might verify the behavior of the CheckAuth plug. fd 'check_auth_test.exs' # If the file is found, extract the implementation of the CheckAuth plug. ast-grep --lang elixir --pattern 'defmodule DemoElixirPhoenixWeb.Plugs.CheckAuth do $$$ end'Length of output: 1648
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- lib/demo_elixir_phoenix_web/controllers/page_controller.ex (2 hunks)
- lib/demo_elixir_phoenix_web/plugs/check_auth.ex (1 hunks)
- lib/demo_elixir_phoenix_web/router.ex (2 hunks)
Additional comments not posted (2)
lib/demo_elixir_phoenix_web/controllers/page_controller.ex (2)
31-32: Ensure secure handling of session tokens.The
callbackfunction storesaccess_tokenandrefresh_tokenin the session. Ensure these tokens are securely handled to prevent session hijacking or token leakage.Consider setting appropriate session options, such as
secureandhttp_only, to enhance security.
80-81: Ensure tokens are fully cleared on logout.The
log_outfunction deletesaccess_tokenandrefresh_tokenfrom the session. Verify that the session is fully cleared to prevent unauthorized access.Consider adding tests to ensure that logging out effectively invalidates the session.
|
@Hammadkhan0034 so I'm trying this PR, but I'm not sure I get it working correctly. When I login (successfully) I get all the user details, but as soon as I click on permissions or user details or refresh, the user is always nil. Am I doing something wrong? |
@tcoopman as per Andre message |
|
I'll try again and I'll set-up a reproduction repository if I don't get it to work. Thanks |
|
I am not able to get refresh token after login with client credentials grant type. Can anyone let me how can I get it and what I am doing wrong. |
|
@mhtouseef9 Refresh tokens are generally issued in flows where the client is acting on behalf of a user (such as the :authorization_code flow), allowing the application to refresh the user's access token without requiring them to log in again. In the case of client_credentials, since the access token typically has a short lifespan, the recommended approach is to request a new access token when the old one expires. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
lib/demo_elixir_phoenix_web/plugs/check_auth.ex (4)
1-4: Add module documentation and improve Logger usage.Consider these improvements:
- Add
@moduledocto describe the plug's purpose and usage- Replace
import Loggerwithrequire Loggeras it's the preferred way to use Logger in Elixirdefmodule DemoElixirPhoenixWeb.Plugs.CheckAuth do import Plug.Conn - import Logger + require Logger + @moduledoc """ + A plug that verifies the presence of authentication tokens in the session. + + This plug checks for both access_token and refresh_token in the session + and initializes the KindeClientSDK with these tokens. If tokens are missing + or invalid, it redirects to the login page. + """ alias KindeClientSDK
16-24: Fix formatting and improve pattern matching.The authentication logic can be more explicit and better documented.
+ @doc """ + Authenticates the connection when both tokens are present. + + Returns the connection if authenticated, otherwise redirects to login. + """ + @spec authenticate(Plug.Conn.t(), map(), binary(), binary()) :: Plug.Conn.t() - def authenticate(conn, client ,access_token, refresh_token) when is_binary(access_token) and is_binary(refresh_token) do - case client do - %{grant_type: :authorization_code_flow_pkce, auth_status: :authenticated} -> - conn - _ -> - Logger.warn("Unauthorized, redirecting to login") - redirect_to_login(conn) - end + def authenticate(conn, %{grant_type: :authorization_code_flow_pkce, auth_status: :authenticated}, access_token, refresh_token) + when is_binary(access_token) and is_binary(refresh_token) do + conn + end + + def authenticate(conn, client, access_token, refresh_token) + when is_binary(access_token) and is_binary(refresh_token) do + Logger.warn("Unauthorized client: #{inspect(client)}, redirecting to login") + redirect_to_login(conn) end
26-29: Improve error logging and add documentation.The fallback case should provide more detailed logging about the missing tokens.
+ @doc """ + Fallback authentication handler when tokens are missing or invalid. + + Always redirects to login page. + """ + @spec authenticate(Plug.Conn.t(), map(), term(), term()) :: Plug.Conn.t() def authenticate(conn, client, _, _) do - Logger.warn("Missing tokens, redirecting to login") + Logger.warn("Invalid or missing tokens for client: #{inspect(client)}, redirecting to login") redirect_to_login(conn) end
31-35: Add type spec to the redirect helper.The implementation is correct, but would benefit from a type specification.
+ @spec redirect_to_login(Plug.Conn.t()) :: Plug.Conn.t() defp redirect_to_login(conn) do conn |> Phoenix.Controller.redirect(to: "/log-in") |> halt() endlib/demo_elixir_phoenix_web/templates/page/index.html.eex (1)
23-23: Consider adding a note about session state.For better UX, consider adding an italicized note similar to other routes to indicate whether this action will clear all session data and tokens.
- <li><a href="/log-out">Logout</a></li> + <li><a href="/log-out">Logout</a> <i>(clears all session tokens)</i></li>lib/demo_elixir_phoenix_web/controllers/page_controller.ex (3)
106-116: Extract repeated string sanitization into a helper functionThe
String.replace("\"", "")pattern is repeated multiple times. Consider extracting this into a helper function for better maintainability.+ defp sanitize_env_var(value) do + value |> String.replace("\"", "") + end - Application.get_env(:kinde_sdk, :domain) |> String.replace("\"", ""), + sanitize_env_var(Application.get_env(:kinde_sdk, :domain)),
123-123: Improve user feedback in PKCE registration responseRendering nil as response doesn't provide clear feedback about the success/failure of the registration process. Consider returning a meaningful message or status.
- render(conn, "index.html", response: nil) + render(conn, "index.html", response: %{status: "success", message: "PKCE registration completed"})
137-137: Remove or improve debug statementThe IO.inspect call with a non-descriptive label appears to be debugging code. Either remove it for production or improve it with proper logging.
- {conn, client} = KindeClientSDK.get_token(conn) |> IO.inspect(label: "11225544") + {conn, client} = KindeClientSDK.get_token(conn)Or if logging is needed:
- {conn, client} = KindeClientSDK.get_token(conn) |> IO.inspect(label: "11225544") + {conn, client} = KindeClientSDK.get_token(conn) + Logger.debug("Token endpoint response: #{inspect(client)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
lib/demo_elixir_phoenix_web/controllers/page_controller.ex(3 hunks)lib/demo_elixir_phoenix_web/plugs/check_auth.ex(1 hunks)lib/demo_elixir_phoenix_web/templates/page/index.html.eex(1 hunks)
🔇 Additional comments (4)
lib/demo_elixir_phoenix_web/plugs/check_auth.ex (2)
6-6: LGTM!
Standard plug initialization implementation.
8-14: Enhance token validation security.
Consider adding these security improvements:
- Validate token format before using them
- Check token expiration
- Implement rate limiting for failed attempts
Let's verify if token validation is handled elsewhere:
lib/demo_elixir_phoenix_web/templates/page/index.html.eex (1)
23-23: LGTM! The logout link is properly integrated.
The logout link is consistently placed within the routes list and follows the established pattern.
lib/demo_elixir_phoenix_web/controllers/page_controller.ex (1)
80-82: Verify complete session cleanup during logout
While the code removes access and refresh tokens, verify if there are other authentication-related session keys that should be cleaned up (e.g., user info, claims, etc.).
✅ Verification successful
Based on the search results, I can generate the final response as we have a clear view of all session-related operations in the codebase. The results show that only access_token and refresh_token are being managed through put_session and delete_session operations, and there are no other authentication-related session keys being used.
Session cleanup during logout is complete
The current implementation correctly handles all authentication-related session data. The only session keys used for authentication are access_token and refresh_token, and both are properly cleaned up during logout.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other session-related keys that might need cleanup
rg -A 2 "put_session|delete_session" --type elixir
Length of output: 1265
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
lib/demo_elixir_phoenix_web/controllers/page_controller.ex (3)
80-82: Enhance logout securityConsider enhancing the logout process:
- Clear all session data instead of just tokens
- Add logging for security audit purposes
- conn = delete_session(conn, :access_token) - conn = delete_session(conn, :refresh_token) + conn = + conn + |> configure_session(drop: true) # Clear entire session + |> Logger.info("User logged out successfully")
96-124: Refactor PKCE registration flow for better maintainabilityThe current implementation has several areas for improvement:
- Config handling should be extracted to a helper module
- Add error handling for invalid config values
- Improve response handling
Consider creating a dedicated module for config handling:
defmodule DemoElixirPhoenixWeb.AuthConfig do def get_config(key) do Application.get_env(:kinde_sdk, key) |> case do nil -> {:error, "Missing config for #{key}"} value -> {:ok, String.replace(value, "\"", "")} end end endThen refactor the initialization:
- {conn, client} = - KindeClientSDK.init( - conn, - Application.get_env(:kinde_sdk, :domain) |> String.replace("\"", ""), - # ... more config values - ) + with {:ok, domain} <- AuthConfig.get_config(:domain), + {:ok, callback_url} <- AuthConfig.get_config(:pkce_callback_url), + {:ok, client_id} <- AuthConfig.get_config(:backend_client_id), + {:ok, client_secret} <- AuthConfig.get_config(:client_secret), + {:ok, logout_url} <- AuthConfig.get_config(:pkce_logout_url) do + {conn, client} = + KindeClientSDK.init(conn, domain, callback_url, client_id, + client_secret, grant_type, logout_url) + # ... rest of the flow + else + {:error, msg} -> + conn + |> put_flash(:error, "Authentication configuration error: #{msg}") + |> redirect(to: Routes.page_path(conn, :start)) + end
138-138: Replace debug statement with proper loggingUsing
IO.inspectfor debugging in production code is not recommended. Consider using proper logging instead.- {conn, client} = KindeClientSDK.get_token(conn) |> IO.inspect(label: "11225544") + {conn, client} = KindeClientSDK.get_token(conn) + Logger.debug("Token endpoint response", client: inspect(client))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/demo_elixir_phoenix_web/controllers/page_controller.ex(3 hunks)lib/demo_elixir_phoenix_web/plugs/check_auth.ex(1 hunks)
🔇 Additional comments (7)
lib/demo_elixir_phoenix_web/controllers/page_controller.ex (3)
31-33:
Add validation and secure token storage in callback
The current implementation stores tokens without validation or encryption:
- Add nil checks before storing tokens
- Consider encrypting tokens before storage
- Add error handling for invalid tokens
- conn = put_session(conn, :access_token, client.access_token)
- conn = put_session(conn, :refresh_token, client.refresh_token)
+ case {client.access_token, client.refresh_token} do
+ {access_token, refresh_token} when not is_nil(access_token) and not is_nil(refresh_token) ->
+ conn
+ |> put_session(:access_token, encrypt_token(access_token))
+ |> put_session(:refresh_token, encrypt_token(refresh_token))
+ _ ->
+ conn
+ |> put_flash(:error, "Invalid authentication tokens")
+ |> redirect(to: Routes.page_path(conn, :start))
+ end131-133:
Fix unsafe token handling in PKCE callback
The current implementation has unsafe token handling:
- The nil coalescing (
&&) could store nil values - No validation of the response structure
- No error handling for invalid tokens
- conn = put_session(conn, :access_token, res && res[:access_token])
- conn = put_session(conn, :refresh_token, res && res[:refresh_token])
+ with {:ok, tokens} <- validate_tokens(res) do
+ conn
+ |> put_session(:access_token, encrypt_token(tokens.access_token))
+ |> put_session(:refresh_token, encrypt_token(tokens.refresh_token))
+ else
+ {:error, :invalid_tokens} ->
+ conn
+ |> put_flash(:error, "Invalid authentication response")
+ |> redirect(to: Routes.page_path(conn, :start))
+ end
# Add this helper function
+ defp validate_tokens(%{access_token: at, refresh_token: rt})
+ when is_binary(at) and is_binary(rt),
+ do: {:ok, %{access_token: at, refresh_token: rt}}
+ defp validate_tokens(_), do: {:error, :invalid_tokens}Likely invalid or redundant comment.
Line range hint 31-133: Verify token persistence and security
Let's verify the token handling implementation across the codebase:
- Token persistence across requests
- Security of token storage
- Proper error handling
lib/demo_elixir_phoenix_web/plugs/check_auth.ex (4)
28-36: Add type specifications and documentation to authenticate/4
For better code clarity and maintainability, consider adding @doc and @spec annotations to the authenticate/4 function. This helps others understand the function's purpose and its expected inputs and outputs.
38-41: Add type specifications and documentation to the fallback authenticate/4 clause
Similarly, provide @doc and @spec annotations for this clause of the authenticate/4 function to enhance code readability.
18-23: Handle potential errors from KindeClientSDK.get_token/1
The current implementation assumes that KindeClientSDK.get_token/1 will always return a successful result or a tuple matching {conn, %KindeClientSDK{}}. Consider handling possible errors to ensure robustness.
15-16: Ensure tokens are securely managed and exist
Verify that the access_token and refresh_token retrieved from the session are valid and secure. Storing tokens in the session should be done with caution to prevent security vulnerabilities.
|
I have updated the code. Plz try and let me know if any change required |
Explain your changes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation