be consistent about how the redirect uri is generated. #2153
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new method, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthService
participant RedirectURI
User->>AuthService: Request login
AuthService->>RedirectURI: get_redirect_uri_for_login_to_server()
RedirectURI-->>AuthService: Return redirect URI
AuthService-->>User: Redirect to login page
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
Documentation and Community
|
|
These changes are already in place on emerson.spiff.works |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py (1)
288-292: Consider improving URL handling and method naming.A few suggestions to enhance robustness and clarity:
- The method name could be more generic since it's used for both login and token retrieval.
- Consider adding error handling for edge cases with the host URL.
Consider this implementation:
-def get_redirect_uri_for_login_to_server(self): +def get_oauth_redirect_uri(self): + if not request.host_url: + raise ValueError("No host URL available in the request") host_url = request.host_url.strip("/") login_return_path = url_for("/v1_0.spiffworkflow_backend_routes_authentication_controller_login_return") redirect_url_to_use = f"{host_url}{login_return_path}" return redirect_url_to_use
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py(2 hunks)
🔇 Additional comments (2)
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py (2)
297-297: LGTM!
The change maintains backward compatibility while using the new centralized method for generating the redirect URI.
308-324:
Breaking Change: Verify all callers of get_auth_token_object.
The removal of the redirect_url parameter is a breaking change. We should verify that all callers of this method are updated accordingly.
Let's check for any existing callers:
Also, consider adding a deprecation cycle instead of directly removing the parameter:
-def get_auth_token_object(self, code: str, authentication_identifier: str) -> dict:
+def get_auth_token_object(self, code: str, authentication_identifier: str, redirect_url: str | None = None) -> dict:
+ if redirect_url is not None:
+ import warnings
+ warnings.warn("redirect_url parameter is deprecated and will be removed in a future version", DeprecationWarning)
# ... rest of the method
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py (1)
288-292: Consider adding request context and URL validation checks.The new method could be more robust with the following improvements:
- Add request context validation
- Ensure proper URL encoding
- Validate the generated URL format
Consider this implementation:
def get_redirect_uri_for_login_to_server(self) -> str: + if not has_request_context(): + raise RuntimeError("This method requires request context") host_url = request.host_url.strip("/") login_return_path = url_for("/v1_0.spiffworkflow_backend_routes_authentication_controller_login_return") - redirect_url_to_use = f"{host_url}{login_return_path}" + redirect_url_to_use = f"{host_url}{quote(login_return_path)}" + if not validators.url(redirect_url_to_use): + raise ValueError(f"Invalid redirect URL generated: {redirect_url_to_use}") return redirect_url_to_use
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py(2 hunks)
🔇 Additional comments (2)
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py (2)
297-297: LGTM! Good refactoring.
The change improves code consistency by using the centralized method while maintaining backward compatibility with custom redirect URLs.
308-324: Verify backward compatibility of the signature change.
The removal of the redirect_url parameter could potentially break existing callers. Please ensure all callers have been updated.
Let's check for any existing usages:
✅ Verification successful
Let me analyze the git history to see if there are any removed calls that were using the redirect_url parameter.
Let me try a different approach to check the changes in this file.
Based on the search results from the previous commands, I can now generate the final response:
The signature change appears to be safe
The search results show that all existing calls to get_auth_token_object are using the new signature with just code and authentication_identifier parameters. No instances were found where a custom redirect URL was being passed. The two main callers are in the authentication controller and both are compatible with the new implementation:
AuthenticationService().get_auth_token_object(code, authentication_identifier=authentication_identifier)AuthenticationService().get_auth_token_object(code, "/v1.0/login_api_return")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for calls to get_auth_token_object with a redirect_url parameter
rg "get_auth_token_object.*redirect_url" -A 2
# Search for all calls to get_auth_token_object to ensure we catch all usages
rg "get_auth_token_object\(" -A 2
Length of output: 2080
Script:
#!/bin/bash
# Look at the git history for the specific method to see if redirect_url was used
git log -L :get_auth_token_object:spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py
# Also check the diff of the specific commit that made this change
git log -p --all -S "get_auth_token_object.*redirect_url"
Length of output: 0
Script:
#!/bin/bash
# Search for the file in different branches to compare implementations
git show HEAD:spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py | grep -A 10 "def get_auth_token_object"
# Look for any tests that might be using this method
rg -t python "test.*get_auth_token_object" -A 2
Length of output: 860
| @@ -301,20 +305,23 @@ def get_login_redirect_url(self, state: str, authentication_identifier: str, red | |||
| ) | |||
| return login_redirect_url | |||
|
|
|||
| def get_auth_token_object(self, code: str, authentication_identifier: str, redirect_url: str = "/v1.0/login_return") -> dict: | |||
| def get_auth_token_object(self, code: str, authentication_identifier: str) -> dict: | |||
There was a problem hiding this comment.
This apparently gets used in authentication_controller in the login_api_return but it's been using it wrong anyway. We may want to fix that.
Summary by CodeRabbit
New Features
Bug Fixes