Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rb] Add FedCM support to the ruby selenium client #13796

Open
wants to merge 39 commits into
base: trunk
Choose a base branch
from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Apr 10, 2024

User description

Description

As mentioned in #12088

The goal is to add support for the Federal credential management API (FedCM) so users have access to commands that will enable them to automate it

Reference Docs:

https://fedidcg.github.io/FedCM/#automation
https://developer.mozilla.org/en-US/docs/Web/API/FedCM_API

Motivation and Context

On Chrome 108 FedCM has shipped and it's already implemented by several web solutions, by adding support to the client library we guarantee that more users can automate this functionality without work arounds

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests


Description

  • Added support for the Federal Credential Management API (FedCM) to the Ruby Selenium client.
  • Introduced FedCM module with Account and Dialog classes.
  • Implemented HasFedCmDialog driver extension for managing FedCM dialogs.
  • Added methods for FedCM dialog interactions in the bridge.
  • Included integration and unit tests for FedCM functionality.
  • Added type signatures for new FedCM classes and modules.

Changes walkthrough 📝

Relevant files
Enhancement
12 files
webdriver.rb
Add autoload for FedCM module.                                                     

rb/lib/selenium/webdriver.rb

  • Added autoload for FedCM.
+1/-0     
driver.rb
Include HasFedCmDialog in driver extensions.                         

rb/lib/selenium/webdriver/chromium/driver.rb

  • Added HasFedCmDialog to driver extensions.
+1/-0     
common.rb
Require has_fedcm_dialog in common driver extensions.       

rb/lib/selenium/webdriver/common.rb

  • Required has_fedcm_dialog in common driver extensions.
+1/-0     
has_fedcm_dialog.rb
Add HasFedCmDialog module for FedCM dialog management.     

rb/lib/selenium/webdriver/common/driver_extensions/has_fedcm_dialog.rb

  • Added HasFedCmDialog module with methods for FedCM dialog management.
  • +36/-0   
    fedcm.rb
    Introduce FedCM module with autoload.                                       

    rb/lib/selenium/webdriver/fedcm.rb

    • Added FedCM module with autoload for Account and Dialog.
    +10/-0   
    account.rb
    Add Account class for FedCM accounts.                                       

    rb/lib/selenium/webdriver/fedcm/account.rb

    • Added Account class to represent FedCM accounts.
    +31/-0   
    dialog.rb
    Add Dialog class for FedCM interactions.                                 

    rb/lib/selenium/webdriver/fedcm/dialog.rb

    • Added Dialog class with methods for FedCM dialog interactions.
    +52/-0   
    bridge.rb
    Add FedCM dialog management methods in bridge.                     

    rb/lib/selenium/webdriver/remote/bridge.rb

    • Added methods for FedCM dialog management in the bridge.
    +51/-12 
    commands.rb
    Add FedCM commands to bridge.                                                       

    rb/lib/selenium/webdriver/remote/bridge/commands.rb

    • Added FedCM related commands to the bridge.
    +18/-2   
    account.rbs
    Add type signatures for FedCM Account.                                     

    rb/sig/lib/selenium/webdriver/fedcm/account.rbs

    • Added type signatures for FedCM Account class.
    +51/-0   
    dialog.rbs
    Add type signatures for FedCM Dialog.                                       

    rb/sig/lib/selenium/webdriver/fedcm/dialog.rbs

    • Added type signatures for FedCM Dialog class.
    +31/-0   
    has_fed_cm_dialog.rbs
    Add type signatures for HasFedCmDialog.                                   

    rb/sig/selenium/web_driver/driver_extensions/has_fed_cm_dialog.rbs

    • Added type signatures for HasFedCmDialog module.
    +15/-0   
    Tests
    3 files
    dialog_spec.rb
    Add integration tests for FedCM dialog.                                   

    rb/spec/integration/selenium/webdriver/fedcm/dialog_spec.rb

    • Added integration tests for FedCM dialog interactions.
    +89/-0   
    account_spec.rb
    Add unit tests for FedCM Account.                                               

    rb/spec/unit/selenium/webdriver/fedcm/account_spec.rb

    • Added unit tests for FedCM Account class.
    +45/-0   
    dialog_spec.rb
    Add unit tests for FedCM Dialog.                                                 

    rb/spec/unit/selenium/webdriver/fedcm/dialog_spec.rb

    • Added unit tests for FedCM Dialog class.
    +67/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @aguspe aguspe marked this pull request as ready for review June 9, 2024 14:04
    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR introduces a new feature with multiple new classes and methods across several files. The changes are well-structured and modular, but the complexity and interdependencies of the new functionality require a thorough review to ensure correctness and integration with existing systems.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The methods set_fedcm_delay and reset_fedcm_cooldown are defined twice in the commands.rb file. This could lead to unexpected behavior or syntax errors depending on the Ruby interpreter's handling of method redefinitions.

    Error Handling: The methods in dialog.rb and account.rb do not appear to handle exceptions beyond the scope of existing error checks. Additional error handling might be necessary to manage unexpected or invalid responses from the FedCM API.

    🔒 Security concerns

    No

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Eliminate duplicate method definitions

    Remove duplicate method definitions to avoid confusion and potential bugs.

    rb/sig/selenium/web_driver/fed_cm/dialog.rbs [36-42]

    -def title: -> untyped
     def title: -> untyped
     def type: -> untyped
    -def type: -> untyped
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Removing duplicate method definitions is crucial to prevent confusion and potential runtime errors. This is a significant improvement for maintainability.

    8
    Remove redundant definitions of set_fedcm_delay and reset_fedcm_cooldown commands

    The set_fedcm_delay and reset_fedcm_cooldown commands are defined twice in the COMMANDS
    hash. This redundancy should be removed to avoid potential confusion or errors.

    rb/lib/selenium/webdriver/remote/bridge.rb [170-173]

    -set_fedcm_delay: [:post, 'session/:session_id/fedcm/setdelayenabled'],
    -reset_fedcm_cooldown: [:post, 'session/:session_id/fedcm/resetcooldown'],
     set_fedcm_delay: [:post, 'session/:session_id/fedcm/setdelayenabled'],
     reset_fedcm_cooldown: [:post, 'session/:session_id/fedcm/resetcooldown']
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Removing duplicate command definitions avoids confusion and potential errors in command execution, improving maintainability.

    7
    Best practice
    Replace untyped with specific types for instance variables and attributes

    Consider using specific types instead of untyped for the instance variables and
    attributes. This will improve type safety and make the code more maintainable.

    rb/sig/lib/selenium/webdriver/fedcm/account.rbs [6-22]

    -@account_id: untyped
    -@email: untyped
    -@name: untyped
    -@given_name: untyped
    -@picture_url: untyped
    -@idp_config_url: untyped
    -@login_state: untyped
    -@terms_of_service_url: untyped
    -@privacy_policy_url: untyped
    +@account_id: String
    +@email: String
    +@name: String
    +@given_name: String
    +@picture_url: String
    +@idp_config_url: String
    +@login_state: String
    +@terms_of_service_url: String
    +@privacy_policy_url: String
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Specifying types instead of using untyped enhances type safety and maintainability. However, the exact types should be confirmed to match the data expected.

    7
    Specify return types for methods to enhance type safety and readability

    Add return types for the methods to improve type safety and code readability.

    rb/sig/lib/selenium/webdriver/fedcm/dialog.rbs [8-29]

    -def cancel: () -> untyped
    -def select_account: (untyped index) -> untyped
    -def type: () -> untyped
    -def title: () -> untyped
    -def subtitle: () -> untyped
    -def accounts: () -> untyped
    +def cancel: () -> void
    +def select_account: (Integer index) -> void
    +def type: () -> String
    +def title: () -> String
    +def subtitle: () -> String?
    +def accounts: () -> Array[Account]
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding specific return types improves type safety and readability of the code. The suggestion is correct and aligns with best practices in typed Ruby.

    7
    Enhancement
    Simplify the transformation and nil-checking of the name variable using the safe navigation operator

    The name variable is being transformed and checked for nil in the same line. This can be
    simplified by using the safe navigation operator (&.) to avoid potential nil errors and
    make the code more readable.

    rb/lib/selenium/webdriver/remote/bridge.rb [109-110]

    -name = @capabilities.browser_name
    -name ? name.tr(' -', '_').downcase.to_sym : 'unknown'
    +name = @capabilities.browser_name&.tr(' -', '_')&.downcase&.to_sym || :unknown
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using the safe navigation operator simplifies the code and prevents potential nil errors, enhancing readability and safety.

    6
    Possible issue
    Add an initialization method for @fedcm_dialog

    Consider adding a method to initialize @fedcm_dialog to ensure it is properly set up
    before use.

    rb/sig/selenium/web_driver/driver_extensions/has_fed_cm_dialog.rbs [5]

     @fedcm_dialog: untyped
     
    +def initialize_fedcm_dialog: () -> void
    +  @fedcm_dialog = SomeDefaultValue
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding an initialization method can ensure that @fedcm_dialog is properly set up, which is good for code robustness. However, the necessity and implementation details should be carefully considered.

    6
    Performance
    Add a check to ensure the dialog is present before calling wait_for_fedcm_dialog to avoid unnecessary waiting

    The wait_for_fedcm_dialog method is called without checking if the dialog is actually
    present, which may cause unnecessary waiting. Adding a check before calling this method
    can improve performance.

    rb/spec/integration/selenium/webdriver/fedcm/dialog_spec.rb [54]

    -driver.wait_for_fedcm_dialog
    +driver.wait_for_fedcm_dialog if dialog_present?
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a presence check before waiting for the dialog can improve test performance by avoiding unnecessary waits, but the suggestion lacks the actual implementation of dialog_present?.

    5

    @diemol diemol requested a review from titusfortner June 10, 2024 10:33
    Copy link
    Member

    @p0deje p0deje left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This definitely looks like a great start, thank you for working on this!

    Can we add more tests to verify all the endpoints such as delays and cooldowns?

    @@ -0,0 +1,10 @@
    # frozen_string_literal: true
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Can you move this file to common folder?

    end

    def fedcm_dialog
    @fedcm_dialog ||= Selenium::WebDriver::FedCM::Dialog.new(@bridge)
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    You can avoid Selenium::WebDriver:: here since it's the code is already within this module.

    end

    def wait_for_fedcm_dialog(timeout: 5, interval: 0.2, message: nil, ignore: nil)
    wait = Selenium::WebDriver::Wait.new(timeout: timeout, interval: interval, message: message, ignore: ignore)
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    You can avoid Selenium::WebDriver:: here since it's the code is already within this module.

    @@ -0,0 +1,31 @@
    module Selenium
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Can you move this directory to common/fedcm folder?

    @@ -1,20 +0,0 @@
    module Selenium
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I think you removed it by accident, can you pleas restore?

    module WebDriver
    module FedCM
    describe Dialog, exclusive: {browser: :chrome} do
    let(:url) { 'https://fedcm-rp-demo.glitch.me/' }
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Unfortunately, our tests should be atomic and not depend on external websites. Is it possible to replicate a demo similar to how we test basic authentication (

    when '/basicAuth'
    authorize(env)
    ) - purely locally without external network calls?

    expect(account.privacy_policy_url).to eq('privacy-policy-url')
    end

    it 'has a constant LOGIN_STATE_SIGNIN' do
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I don't think it is important to test constant values, we are just testing implementation detail.

    module WebDriver
    module FedCM
    describe Dialog do
    let(:bridge) { instance_double(Selenium::WebDriver::Remote::Bridge) }
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    You can avoid Selenium::WebDriver:: here since it's the code is already within this module.

    module Selenium
    module WebDriver
    module FedCM
    describe Dialog, exclusive: {browser: :chrome} do
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Let's switch from exclusive to only - this will ensure tests are run in other browsers but expected to fail.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    3 participants