Feat/enhanced login #789
Feat/enhanced login #789
Conversation
|
""" WalkthroughSupport for AWS Cognito managed login UI branding is added. New methods are introduced to the Changes
Sequence Diagram(s)sequenceDiagram
participant ManagedLoginStack
participant PersistentStack
participant UserPool
participant UserPoolClient
participant Branding JSON
participant Image Files
ManagedLoginStack->>Branding JSON: Load style settings for staff and provider
ManagedLoginStack->>PersistentStack: Request branding assets preparation
PersistentStack->>UserPool: prepare_assets_for_managed_login_ui()
UserPool->>Image Files: Read and encode images to base64
UserPool->>PersistentStack: Return branding assets
ManagedLoginStack->>UserPoolClient: Create CfnManagedLoginBranding with assets and settings
ManagedLoginStack->>UserPoolClient: Add dependency for resource creation order
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (6)
backend/compact-connect/common_constructs/user_pool.py (3)
229-246: Well-structured method for applying managed login styles.The method correctly creates a CfnManagedLoginBranding resource and sets up the proper dependency relationship with the user pool client. This ensures that the client resource is created before the branding is applied.
I notice that the resource ID is hardcoded as 'MyCfnManagedLoginBranding', which could cause conflicts if multiple login branding resources are created within the same construct scope.
Consider making the identifier more specific:
- login_branding = CfnManagedLoginBranding(self, 'MyCfnManagedLoginBranding', + login_branding = CfnManagedLoginBranding(self, f'{user_pool_client.node.id}ManagedLoginBranding',
247-281: Well-designed asset preparation method.The method handles different types of assets appropriately (favicon, logo, background) and has a good optional parameter design for the background file path. The implementation follows AWS documentation guidelines as noted in the comment.
Two suggestions for improvement:
- Consider adding error handling for missing files:
def prepare_assets_for_managed_login_ui( self, ico_filepath: str, logo_filepath: str, background_file_path: str | None = None ): + # Validate file existence + for filepath in [ico_filepath, logo_filepath]: + if not os.path.exists(filepath): + raise ValueError(f"Required file not found: {filepath}") + + if background_file_path and not os.path.exists(background_file_path): + raise ValueError(f"Background file not found: {background_file_path}") # options found: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-cognito-managedloginbranding-assettype.html#cfn-cognito-managedloginbranding-assettype-category
- The method would benefit from a docstring explaining the purpose and parameters.
283-287: Simple and effective utility method.This method correctly handles binary file reading and base64 encoding.
Consider adding error handling for file operations.
def convert_img_to_base_64(self, file_path: str): + """ + Converts an image file to a base64 encoded string. + + Args: + file_path: Path to the image file to convert + + Returns: + Base64 encoded string representation of the image file + + Raises: + FileNotFoundError: If the file does not exist + IOError: If there is an issue reading the file + """ with open(file_path, 'rb') as binary_file: binary_file_data = binary_file.read() base64_encoded_data = base64.b64encode(binary_file_data) return base64_encoded_data.decode('utf-8')backend/compact-connect/stacks/persistent_stack/provider_users.py (1)
76-88: Well-structured implementation of managed login styling.The implementation follows a clean approach:
- Load styling settings from JSON file
- Prepare the branding assets
- Apply styles and assets to the user pool client
The code leverages the new methods added to the UserPool class effectively.
Two considerations for further improvement:
- Consider adding error handling for the file loading operation:
- with open('resources/provider_managed_login_style_settings.json') as f: - branding_settings = json.load(f) + try: + with open('resources/provider_managed_login_style_settings.json') as f: + branding_settings = json.load(f) + except (FileNotFoundError, json.JSONDecodeError) as e: + self.node.add_warning(f"Could not load branding settings: {str(e)}. Using default branding.") + branding_settings = {}
- Consider making the asset paths configurable rather than hardcoded:
branding_assets = self.prepare_assets_for_managed_login_ui( - ico_filepath='resources/assets/favicon.ico', - logo_filepath='resources/assets/compact-connect-logo.png' + ico_filepath=self.node.try_get_context('branding_favicon') or 'resources/assets/favicon.ico', + logo_filepath=self.node.try_get_context('branding_logo') or 'resources/assets/compact-connect-logo.png' )backend/compact-connect/stacks/persistent_stack/staff_users.py (1)
74-87: Effective implementation of managed login styling for staff users.The implementation properly:
- Loads styling settings from a staff-specific JSON file
- Prepares branding assets including a background image
- Applies styles and assets to the staff UI client
The implementation is consistent with the approach used for provider users.
Two considerations for improvement:
- The file loading code is duplicated between this class and ProviderUsers. Consider extracting a utility method in the parent UserPool class:
# In UserPool class: + def load_branding_settings(self, file_path): + """Load branding settings from a JSON file with error handling""" + try: + with open(file_path) as f: + return json.load(f) + except (FileNotFoundError, json.JSONDecodeError) as e: + self.node.add_warning(f"Could not load branding settings from {file_path}: {str(e)}. Using default branding.") + return {} # Then in StaffUsers: - with open('resources/staff_managed_login_style_settings.json') as f: - branding_settings = json.load(f) + branding_settings = self.load_branding_settings('resources/staff_managed_login_style_settings.json')
- Consider making the asset paths configurable to improve flexibility:
branding_assets = self.prepare_assets_for_managed_login_ui( - ico_filepath='resources/assets/favicon.ico', - logo_filepath='resources/assets/compact-connect-logo.png', - background_file_path='resources/assets/staff-background.png' + ico_filepath=self.node.try_get_context('staff_branding_favicon') or 'resources/assets/favicon.ico', + logo_filepath=self.node.try_get_context('staff_branding_logo') or 'resources/assets/compact-connect-logo.png', + background_file_path=self.node.try_get_context('staff_branding_background') or 'resources/assets/staff-background.png' )backend/compact-connect/resources/provider_managed_login_style_settings.json (1)
1-449: Comprehensive styling configuration for the managed login UI.The JSON configuration provides detailed styling for all UI components in both light and dark modes, adhering to what appears to be a consistent design system.
A few observations:
Color values are specified without the "#" prefix (e.g., "ffffffff" instead of "#ffffffff"). This is likely required by AWS Cognito's API, but it would be helpful to add a comment in the related code explaining this requirement.
Some color values appear to include alpha channels (8 characters instead of 6), but others might not have fully specified alpha values. Ensure all color specifications follow the same format as required by AWS Cognito.
Consider adding a comment or documentation about how to update or modify these styles in the future, including any tools or design system references that were used to generate these values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
backend/compact-connect/resources/assets/compact-connect-logo.pngis excluded by!**/*.pngbackend/compact-connect/resources/assets/favicon.icois excluded by!**/*.icobackend/compact-connect/resources/assets/staff-background.pngis excluded by!**/*.png
📒 Files selected for processing (4)
backend/compact-connect/common_constructs/user_pool.py(5 hunks)backend/compact-connect/resources/provider_managed_login_style_settings.json(1 hunks)backend/compact-connect/stacks/persistent_stack/provider_users.py(2 hunks)backend/compact-connect/stacks/persistent_stack/staff_users.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/compact-connect/stacks/persistent_stack/staff_users.py (1)
backend/compact-connect/common_constructs/user_pool.py (2)
prepare_assets_for_managed_login_ui(247-281)add_managed_login_styles(229-245)
backend/compact-connect/stacks/persistent_stack/provider_users.py (1)
backend/compact-connect/common_constructs/user_pool.py (2)
prepare_assets_for_managed_login_ui(247-281)add_managed_login_styles(229-245)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (4)
backend/compact-connect/common_constructs/user_pool.py (3)
1-1: Appropriate import added for base64 encoding.The addition of the base64 module is necessary for the new image encoding functionality.
10-10: Required Cognito imports added.Adding CfnManagedLoginBranding and making UserPoolClient explicit are necessary for the new managed login UI styling functionality.
Also applies to: 25-25
130-131: Good parameter design with optional UI scopes.Making the ui_scopes parameter optional with a default value of None is a good practice, ensuring backward compatibility.
backend/compact-connect/stacks/persistent_stack/provider_users.py (1)
3-4: Appropriate import added for JSON handling.The import of the json module is necessary for loading style settings from the configuration file.
backend/compact-connect/resources/staff_managed_login_style_settings.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/compact-connect/stacks/persistent_stack/provider_users.py (1)
3-4: Remove unused import.The
jsonimport is no longer used after moving the managed login styling logic to the ManagedLoginStack.-import json🧰 Tools
🪛 Ruff (0.8.2)
3-3:
jsonimported but unusedRemove unused import:
json(F401)
backend/compact-connect/stacks/managed_login_stack.py (4)
16-26: Constructor has unused parameters.The
environment_nameandenvironment_contextparameters are declared but not used in the implementation.If these parameters are not needed, consider removing them or documenting why they're included for future use:
def __init__( self, scope: Construct, construct_id: str, *, persistent_stack: PersistentStack, - environment_name: str, - environment_context: dict, **kwargs, ) -> None: super().__init__(scope, construct_id, **kwargs)If they're kept for future use or API consistency, add a comment explaining this.
🧰 Tools
🪛 Ruff (0.8.2)
22-22: Unused method argument:
environment_name(ARG002)
23-23: Unused method argument:
environment_context(ARG002)
34-60: Consider making file paths more robust.The implementation uses relative file paths which may be fragile if the working directory changes during execution.
Consider using a more robust approach to locate resource files, such as:
- with open('resources/staff_managed_login_style_settings.json') as f: + import os + # Add this where appropriate, e.g., as a class method or in __init__ + def get_resource_path(self, filename): + """Get absolute path to a resource file.""" + base_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + return os.path.join(base_dir, 'resources', filename) + + # Then use it in your methods + with open(self.get_resource_path('staff_managed_login_style_settings.json')) as f:🧰 Tools
🪛 Ruff (0.8.2)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Trailing whitespace
Remove trailing whitespace
(W291)
58-58: Blank line contains whitespace
Remove whitespace from blank line
(W293)
34-87: Consider reducing code duplication.The methods
_create_managed_login_for_staff_usersand_create_managed_login_for_provider_usershave significant duplication.Extract common logic into a helper method:
def _create_managed_login(self, user_type: str, user_pool_group, style_settings_file: str, assets_config: dict): """ Create managed login branding for a user pool Args: user_type: String identifier ('Staff' or 'Provider') user_pool_group: The user pool group (staff_users or provider_users) style_settings_file: Path to style settings JSON assets_config: Dictionary of asset file paths """ # Load the style settings with open(style_settings_file) as f: branding_settings = json.load(f) # Prepare the assets branding_assets = user_pool_group.prepare_assets_for_managed_login_ui(**assets_config) # Create the managed login branding login_branding = CfnManagedLoginBranding( self, f'{user_type}ManagedLoginBranding', user_pool_id=user_pool_group.user_pool_id, assets=branding_assets, client_id=user_pool_group.ui_client.user_pool_client_id, return_merged_resources=False, settings=branding_settings, use_cognito_provided_values=False ) # Add dependency on the UI client login_branding.add_dependency(user_pool_group.ui_client.node.default_child) return login_brandingThen call it from your initialization:
def __init__(self, ...): # ... self._create_managed_login( 'Staff', persistent_stack.staff_users, 'resources/staff_managed_login_style_settings.json', { 'ico_filepath': 'resources/assets/favicon.ico', 'logo_filepath': 'resources/assets/compact-connect-logo.png', 'background_file_path': 'resources/assets/staff-background.png' } ) self._create_managed_login( 'Provider', persistent_stack.provider_users, 'resources/provider_managed_login_style_settings.json', { 'ico_filepath': 'resources/assets/favicon.ico', 'logo_filepath': 'resources/assets/compact-connect-logo.png' } )🧰 Tools
🪛 Ruff (0.8.2)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Trailing whitespace
Remove trailing whitespace
(W291)
58-58: Blank line contains whitespace
Remove whitespace from blank line
(W293)
67-67: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: Blank line contains whitespace
Remove whitespace from blank line
(W293)
76-76: Trailing whitespace
Remove trailing whitespace
(W291)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Remove trailing whitespace.There's trailing whitespace on lines 49 and 76.
- self, + self,Also applies to: 76-76
🧰 Tools
🪛 Ruff (0.8.2)
49-49: Trailing whitespace
Remove trailing whitespace
(W291)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/common_constructs/user_pool.py(6 hunks)backend/compact-connect/pipeline/backend_stage.py(2 hunks)backend/compact-connect/stacks/managed_login_stack.py(1 hunks)backend/compact-connect/stacks/persistent_stack/provider_users.py(2 hunks)backend/compact-connect/stacks/persistent_stack/staff_users.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/compact-connect/stacks/persistent_stack/staff_users.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/common_constructs/user_pool.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/compact-connect/pipeline/backend_stage.py (1)
backend/compact-connect/stacks/managed_login_stack.py (1)
ManagedLoginStack(8-87)
🪛 Ruff (0.8.2)
backend/compact-connect/stacks/managed_login_stack.py
11-11: Blank line contains whitespace
Remove whitespace from blank line
(W293)
22-22: Unused method argument: environment_name
(ARG002)
23-23: Unused method argument: environment_context
(ARG002)
30-30: Blank line contains whitespace
Remove whitespace from blank line
(W293)
39-39: Blank line contains whitespace
Remove whitespace from blank line
(W293)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Trailing whitespace
Remove trailing whitespace
(W291)
58-58: Blank line contains whitespace
Remove whitespace from blank line
(W293)
67-67: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: Blank line contains whitespace
Remove whitespace from blank line
(W293)
76-76: Trailing whitespace
Remove trailing whitespace
(W291)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/compact-connect/stacks/persistent_stack/provider_users.py
3-3: json imported but unused
Remove unused import: json
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (6)
backend/compact-connect/pipeline/backend_stage.py (2)
9-9: Added import for the new ManagedLoginStack.The import is correctly placed with other stack imports.
39-47: Correctly integrates the new ManagedLoginStack.The stack is properly instantiated between the PersistentStack and IngestStack, with all necessary parameters passed. This follows the same pattern as other stacks in the file.
backend/compact-connect/stacks/persistent_stack/provider_users.py (1)
76-78: Clear comment explaining architectural change.Good documentation explaining that managed login styling has been moved to reduce CloudFormation template size.
backend/compact-connect/stacks/managed_login_stack.py (3)
8-15: Well-documented new stack class.The class documentation clearly explains the purpose of this new stack and its benefits in relation to CloudFormation template size limits.
🧰 Tools
🪛 Ruff (0.8.2)
11-11: Blank line contains whitespace
Remove whitespace from blank line
(W293)
53-57: CfnManagedLoginBranding configuration looks good.The managed login branding is configured correctly with user pool ID, client ID, assets, and settings. The explicit setting of
return_merged_resources=Falseanduse_cognito_provided_values=Falseensures consistent behavior.
60-60: Good dependency management.Adding an explicit dependency on the UI client ensures proper deployment order, preventing potential race conditions during CloudFormation deployment.
Also applies to: 87-87
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/compact-connect/stacks/managed_login_stack.py (4)
11-11: Remove whitespace from blank line.This blank line contains whitespace which should be removed.
- +🧰 Tools
🪛 Ruff (0.8.2)
11-11: Blank line contains whitespace
Remove whitespace from blank line
(W293)
40-48: Consider parameterizing file paths.The file paths for assets are hardcoded. Consider making them configurable via environment variables or configuration files to improve flexibility.
51-60: Remove unused variable assignment.The variable
staff_login_brandingis assigned but never used.- staff_login_branding = CfnManagedLoginBranding( + CfnManagedLoginBranding(🧰 Tools
🪛 Ruff (0.8.2)
51-51: Local variable
staff_login_brandingis assigned to but never usedRemove assignment to unused variable
staff_login_branding(F841)
75-84: Remove unused variable assignment.The variable
provider_login_brandingis assigned but never used.- provider_login_branding = CfnManagedLoginBranding( + CfnManagedLoginBranding(🧰 Tools
🪛 Ruff (0.8.2)
75-75: Local variable
provider_login_brandingis assigned to but never usedRemove assignment to unused variable
provider_login_branding(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/stacks/managed_login_stack.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/compact-connect/stacks/managed_login_stack.py
11-11: Blank line contains whitespace
Remove whitespace from blank line
(W293)
51-51: Local variable staff_login_branding is assigned to but never used
Remove assignment to unused variable staff_login_branding
(F841)
75-75: Local variable provider_login_branding is assigned to but never used
Remove assignment to unused variable provider_login_branding
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/stacks/managed_login_stack.py (3)
8-19: Well-structured stack with clear documentation!The creation of a separate stack for managing Cognito branding assets is a smart approach to avoid CloudFormation template size limits. I particularly appreciate the documentation that explains how to obtain the style settings JSON data.
🧰 Tools
🪛 Ruff (0.8.2)
11-11: Blank line contains whitespace
Remove whitespace from blank line
(W293)
31-35: LGTM!The initialization code cleanly separates the creation of managed login branding for different user groups.
69-72: Note the absence of background image for provider users.Unlike the staff users configuration, the provider users don't have a background image. If this is intentional, it might be worth adding a comment explaining this difference.
There was a problem hiding this comment.
Looks good, just deployed it and verified everything in my sandbox. This is ready for your review @jlkravitz
Requirements List
Description List
Testing List
backend/compact-connect/tests/unit/test_api.pyCloses #719
Summary by CodeRabbit
New Features
Chores