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

[js] Add Federated Credential Management support #15008

Merged
merged 10 commits into from
Jan 6, 2025

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jan 2, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Add Federated Credential Management support

Motivation and Context

Add Federated Credential Management support as described in https://www.w3.org/TR/fedcm/#automation

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 Federated Credential Management (FedCM) API support.

  • Introduced new commands for FedCM operations.

  • Implemented FedCM dialog and account handling classes.

  • Added comprehensive tests for FedCM functionality.


Changes walkthrough 📝

Relevant files
Enhancement
5 files
command.js
Added new FedCM-related commands.                                               
+11/-0   
account.js
Implemented `Account` class for FedCM account handling.   
+78/-0   
dialog.js
Implemented `Dialog` class for FedCM dialog operations.   
+76/-0   
http.js
Added HTTP command mappings for FedCM operations.               
+10/-0   
webdriver.js
Integrated FedCM dialog handling into WebDriver.                 
+13/-0   
Tests
3 files
fileserver.js
Added FedCM test page to file server.                                       
+1/-1     
fedcm_test.js
Added tests for FedCM dialog and account handling.             
+183/-0 
fedcm_async_js.html
Added example HTML page for FedCM testing.                             
+40/-0   
Configuration changes
1 files
BUILD.bazel
Updated Bazel build configuration to include FedCM files.
+1/-0     

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link
Contributor

qodo-merge-pro bot commented Jan 2, 2025

PR Reviewer Guide 🔍

(Review updated until commit 1e6c0a2)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 Security concerns

Sensitive information exposure:
The FedCM implementation handles sensitive user account information including emails and account IDs. While the implementation follows the W3C spec, care should be taken to ensure this information is properly protected in transit and not logged or exposed unnecessarily.

⚡ Recommended focus areas for review

Missing Error Handling

The subtitle() and type() methods don't handle potential errors or invalid states, unlike other methods in the class. Consider adding proper error handling and validation.

subtitle() {
  return this._driver.execute(new command.Command(command.Name.GET_FEDCM_TITLE))
}

type() {
  return this._driver.execute(new command.Command(command.Name.GET_FEDCM_DIALOG_TYPE))
}
Test Coverage

The test suite doesn't verify error cases or edge conditions, such as invalid account indices or network failures. Consider adding negative test cases.

describe('Federated Credential Management Test', function () {
  it('credential mangement dialog should appear', async function () {
    await driver.get(Pages.fedcm)

    let triggerButton = await driver.findElement(By.id('triggerButton'))
    await triggerButton.click()

    let dialog

    await driver.wait(
      async () => {
        try {
          dialog = await driver.getFederalCredentialManagementDialog()
          return (await dialog.type()) === 'AccountChooser'
        } catch (error) {
          return false
        }
      },
      10000,
      'Expected dialog type to be "AccountChooser"',
      2000,
    )

    assert.equal(await dialog.type(), 'AccountChooser')
    let title = await dialog.title()
    assert.equal(title.includes('Sign in to'), true)
  })

  it('can dismiss dialog', async function () {
    await driver.get(Pages.fedcm)

    let triggerButton = await driver.findElement(By.id('triggerButton'))
    await triggerButton.click()

    let dialog = await driver.getFederalCredentialManagementDialog()

    await driver.wait(
      async () => {
        try {
          return (await dialog.type()) === 'AccountChooser'
        } catch (error) {
          return false
        }
      },
      10000,
      'Expected dialog type to be "AccountChooser"',
      2000,
    )

    assert.equal(await dialog.type(), 'AccountChooser')
    let title = await dialog.title()
    assert.equal(title.includes('Sign in to'), true)

    await dialog.dismiss()

    try {
      await dialog.type()
      assert.fail('Above command should throw error')
    } catch (error) {
      assert.equal(error.message.includes('no such alert'), true)
    }
  })

  it('can select account', async function () {
    await driver.get(Pages.fedcm)

    let triggerButton = await driver.findElement(By.id('triggerButton'))
    await triggerButton.click()

    let dialog = await driver.getFederalCredentialManagementDialog()

    await driver.wait(
      async () => {
        try {
          return (await dialog.type()) === 'AccountChooser'
        } catch (error) {
          return false
        }
      },
      10000,
      'Expected dialog type to be "AccountChooser"',
      2000,
    )

    assert.equal(await dialog.type(), 'AccountChooser')
    let title = await dialog.title()
    assert.equal(title.includes('Sign in to'), true)

    await dialog.selectAccount(1)
  })

  it('can get account list', async function () {
    await driver.get(Pages.fedcm)

    let triggerButton = await driver.findElement(By.id('triggerButton'))
    await triggerButton.click()

    let dialog = await driver.getFederalCredentialManagementDialog()

    await driver.wait(
      async () => {
        try {
          return (await dialog.type()) === 'AccountChooser'
        } catch (error) {
          return false
        }
      },
      10000,
      'Expected dialog type to be "AccountChooser"',
      2000,
    )

    assert.equal(await dialog.type(), 'AccountChooser')
    let title = await dialog.title()
    assert.equal(title.includes('Sign in to'), true)

    const accounts = await dialog.accounts()

    assert.equal(accounts.length, 2)

    const account1 = accounts[0]
    const account2 = accounts[1]

    assert.strictEqual(account1.name, 'John Doe')
    assert.strictEqual(account1.email, '[email protected]')
    assert.strictEqual(account1.accountId, '1234')
    assert.strictEqual(account1.givenName, 'John')
    assert(account1.idpConfigUrl.includes('/fedcm/config.json'), true)
    assert.strictEqual(account1.pictureUrl, 'https://idp.example/profile/123')
    assert.strictEqual(account1.loginState, 'SignUp')
    assert.strictEqual(account1.termsOfServiceUrl, 'https://rp.example/terms_of_service.html')
    assert.strictEqual(account1.privacyPolicyUrl, 'https://rp.example/privacy_policy.html')

    assert.strictEqual(account2.name, 'Aisha Ahmad')
    assert.strictEqual(account2.email, '[email protected]')
    assert.strictEqual(account2.accountId, '5678')
    assert.strictEqual(account2.givenName, 'Aisha')
    assert(account2.idpConfigUrl.includes('/fedcm/config.json'), true)
    assert.strictEqual(account2.pictureUrl, 'https://idp.example/profile/567')
    assert.strictEqual(account2.loginState, 'SignUp')
    assert.strictEqual(account2.termsOfServiceUrl, 'https://rp.example/terms_of_service.html')
    assert.strictEqual(account2.privacyPolicyUrl, 'https://rp.example/privacy_policy.html')
  })
})

Copy link
Contributor

qodo-merge-pro bot commented Jan 2, 2025

PR Code Suggestions ✨

Latest suggestions up to 1e6c0a2
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add async/await handling and proper result extraction for asynchronous method

The subtitle() method is missing the 'async' keyword and doesn't handle the result
object correctly. This could lead to runtime errors when accessing the subtitle
property.

javascript/node/selenium-webdriver/lib/fedcm/dialog.js [32-34]

-subtitle() {
-  return this._driver.execute(new command.Command(command.Name.GET_FEDCM_TITLE))
+async subtitle() {
+  const result = await this._driver.execute(new command.Command(command.Name.GET_FEDCM_TITLE))
+  return result.subtitle
 }
  • Apply this suggestion
Suggestion importance[1-10]: 9

Why: The missing async/await handling could cause runtime errors and Promise-related issues. The suggestion also correctly adds result object handling for the subtitle property, which is crucial for proper functionality.

9
Add async/await handling for asynchronous command execution

The type() method is missing the 'async' keyword which is required for executing
commands. This will cause Promise-related errors.

javascript/node/selenium-webdriver/lib/fedcm/dialog.js [36-38]

-type() {
-  return this._driver.execute(new command.Command(command.Name.GET_FEDCM_DIALOG_TYPE))
+async type() {
+  const result = await this._driver.execute(new command.Command(command.Name.GET_FEDCM_DIALOG_TYPE))
+  return result
 }
  • Apply this suggestion
Suggestion importance[1-10]: 9

Why: Missing async/await in the type() method would cause Promise-related errors when executing commands. This is a critical fix for proper asynchronous operation.

9
General
Maintain consistent command name casing convention across the codebase

The command name 'CLICK_DIALOG_BUTTON' is inconsistently cased compared to other
commands (using lowercase 'dialogbutton'). This could cause matching issues with the
command map.

javascript/node/selenium-webdriver/lib/command.js [198]

-CLICK_DIALOG_BUTTON: 'clickdialogbutton',
+CLICK_DIALOG_BUTTON: 'clickDialogButton',
  • Apply this suggestion
Suggestion importance[1-10]: 5

Why: While not affecting functionality, inconsistent command name casing reduces code maintainability and readability. The suggestion helps maintain a consistent naming convention.

5

Previous suggestions

✅ Suggestions up to commit 42ebe3e
CategorySuggestion                                                                                                                                    Score
Possible issue
Add async/await and proper result handling to prevent potential Promise-related issues

The subtitle() method is missing the 'async' keyword and doesn't return the subtitle
property from the result. This could cause unexpected behavior when calling the
method.

javascript/node/selenium-webdriver/lib/fedcm/dialog.js [36-38]

-subtitle() {
-  return this._driver.execute(new command.Command(command.Name.GET_FEDCM_TITLE))
+async subtitle() {
+  const result = await this._driver.execute(new command.Command(command.Name.GET_FEDCM_TITLE))
+  return result.subtitle
 }
Suggestion importance[1-10]: 8

Why: The suggestion addresses a critical issue with Promise handling that could lead to runtime errors. Adding async/await and proper result extraction is essential for the method to work correctly.

8
Add async/await pattern to ensure proper asynchronous execution

The type() method is missing the 'async' keyword which is required for proper
Promise handling when executing commands.

javascript/node/selenium-webdriver/lib/fedcm/dialog.js [40-42]

-type() {
-  return this._driver.execute(new command.Command(command.Name.GET_FEDCM_DIALOG_TYPE))
+async type() {
+  const result = await this._driver.execute(new command.Command(command.Name.GET_FEDCM_DIALOG_TYPE))
+  return result.type
 }
Suggestion importance[1-10]: 8

Why: The suggestion fixes a critical issue with asynchronous execution that could cause the method to return a Promise instead of the actual dialog type.

8
✅ Add proper async handling and result processing for account list retrieval
Suggestion Impact:The commit implemented async handling of accounts() method and improved result processing, though with a different implementation that creates Account objects from the results

code diff:

-  accounts() {
-    const result = this._driver.execute(new command.Command(command.Name.GET_ACCOUNTS))
+  async accounts() {
+    const result = await this._driver.execute(new command.Command(command.Name.GET_ACCOUNTS))
 
-    return result
+    const accountArray = []
+
+    result.forEach((account) => {
+      const acc = new Account(
+        account.accountId,
+        account.email,
+        account.name,
+        account.givenName,
+        account.pictureUrl,
+        account.idpConfigUrl,
+        account.loginState,
+        account.termsOfServiceUrl,
+        account.privacyPolicyUrl,
+      )
+      accountArray.push(acc)
+    })
+
+    return accountArray
   }

The accounts() method is missing the 'async' keyword and doesn't properly handle the
accounts array from the result.

javascript/node/selenium-webdriver/lib/fedcm/dialog.js [44-48]

-accounts() {
-  const result = this._driver.execute(new command.Command(command.Name.GET_ACCOUNTS))
-  return result
+async accounts() {
+  const result = await this._driver.execute(new command.Command(command.Name.GET_ACCOUNTS))
+  return result.accounts || []
 }
Suggestion importance[1-10]: 8

Why: The suggestion addresses both async handling and proper result processing, preventing potential undefined errors when no accounts are present.

8
✅ Remove duplicate command mapping to prevent potential conflicts
Suggestion Impact:The commit removed one instance of the SET_DELAY_ENABLED command mapping, effectively addressing the duplication issue

code diff:

-  [cmd.Name.SET_DELAY_ENABLED, post(`/session/:sessionId/fedcm/setdelayenabled`)],

Remove the duplicate SET_DELAY_ENABLED command mapping which could cause unexpected
behavior.

javascript/node/selenium-webdriver/lib/http.js [324-329]

 [cmd.Name.SET_DELAY_ENABLED, post(`/session/:sessionId/fedcm/setdelayenabled`)],
 [cmd.Name.CANCEL_DIALOG, post(`/session/:sessionId/fedcm/canceldialog`)],
 [cmd.Name.SELECT_ACCOUNT, post(`/session/:sessionId/fedcm/selectaccount`)],
 [cmd.Name.GET_FEDCM_TITLE, get(`/session/:sessionId/fedcm/gettitle`)],
 [cmd.Name.GET_FEDCM_DIALOG_TYPE, get('/session/:sessionId/fedcm/getdialogtype')],
-[cmd.Name.SET_DELAY_ENABLED, post(`/session/:sessionId/fedcm/setdelayenabled`)],
Suggestion importance[1-10]: 7

Why: The suggestion removes a duplicate command mapping that could cause inconsistent behavior in the command execution system.

7

Copy link
Member

@harsha509 harsha509 left a comment

Choose a reason for hiding this comment

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

we also need to add

"lib/fedcm/*.js", in here

@pujagani pujagani marked this pull request as draft January 6, 2025 07:56
@pujagani pujagani marked this pull request as ready for review January 6, 2025 08:24
Copy link
Contributor

qodo-merge-pro bot commented Jan 6, 2025

Persistent review updated to latest commit 1e6c0a2

@pujagani pujagani merged commit 88d3996 into SeleniumHQ:trunk Jan 6, 2025
34 checks passed
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.

2 participants