Skip to content

Implement exchangeToken public api #9039

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: gcip-byociam-web
Choose a base branch
from
Open

Implement exchangeToken public api #9039

wants to merge 6 commits into from

Conversation

mansisampat
Copy link
Contributor

Discussion

  • Implement the Public APIs for exchangeToken

Testing

  • Testing done via demo app
  • Implemented Unit test

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 14, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 41ad562

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions GitHub Actions
Copy link
Contributor

Vertex AI Mock Responses Check ⚠️

A newer major version of the mock responses for Vertex AI unit tests is available. update_vertexai_responses.sh should be updated to clone the latest version of the responses: v13.0

@mansisampat mansisampat force-pushed the sammansi-gcip-byociam-web branch from 40ca4b5 to 27674f1 Compare May 14, 2025 12:51
@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 14, 2025 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (7bcdaf8)Merge (0c08a6f)Diff
    browser195 kB195 kB-117 B (-0.1%)
    cordova168 kB168 kB-146 B (-0.1%)
    main149 kB150 kB+706 B (+0.5%)
    module195 kB195 kB-117 B (-0.1%)
    react-native167 kB168 kB+845 B (+0.5%)

  • @firebase/auth-cordova

    TypeBase (7bcdaf8)Merge (0c08a6f)Diff
    browser168 kB168 kB-146 B (-0.1%)
    module168 kB168 kB-146 B (-0.1%)

  • @firebase/auth-web-extension

    TypeBase (7bcdaf8)Merge (0c08a6f)Diff
    browser145 kB145 kB-119 B (-0.1%)
    main161 kB162 kB+1.30 kB (+0.8%)
    module145 kB145 kB-119 B (-0.1%)

  • @firebase/auth/internal

    TypeBase (7bcdaf8)Merge (0c08a6f)Diff
    browser206 kB206 kB-117 B (-0.1%)
    main175 kB176 kB+1.19 kB (+0.7%)
    module206 kB206 kB-117 B (-0.1%)

  • bundle

    TypeBase (7bcdaf8)Merge (0c08a6f)Diff
    auth (Anonymous)79.5 kB78.2 kB-1.25 kB (-1.6%)
    auth (EmailAndPassword)89.6 kB88.4 kB-1.23 kB (-1.4%)
    auth (GoogleFBTwitterGitHubPopup)111 kB110 kB-1.26 kB (-1.1%)
    auth (GooglePopup)104 kB102 kB-1.26 kB (-1.2%)
    auth (GoogleRedirect)104 kB102 kB-1.26 kB (-1.2%)
    auth (Phone)97.0 kB95.8 kB-1.21 kB (-1.2%)

  • firebase

    TypeBase (7bcdaf8)Merge (0c08a6f)Diff
    firebase-auth-compat.js147 kB146 kB-985 B (-0.7%)
    firebase-auth-cordova.js144 kB144 kB-6 B (-0.0%)
    firebase-auth-web-extension.js127 kB127 kB+35 B (+0.0%)
    firebase-auth.js164 kB164 kB+82 B (+0.0%)
    firebase-compat.js804 kB803 kB-1.01 kB (-0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/WjjHqV8BFr.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 14, 2025 •
edited
Loading

Size Analysis Report 1

This report is too large (375,540 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/r2YCA3qJJn.html

@mansisampat mansisampat force-pushed the sammansi-gcip-byociam-web branch from 7516e0e to f8410db Compare May 14, 2025 16:33
@mansisampat
GCIP BYO-CIAM Initialize Auth Changes (#8995)
235713a 
* Implement InitializeAuth changes for BYO-CIAM Firebase Auth

* Removing white space

* Addressing review comments

* Adding Unit test for initializeAuth with TenantConfig dependency

* Running yarn run demo

* Update doc
@mansisampat
Throw Operation Not Allowed for Invalid Auth Endpoint (#9013)
80deb06 
* Throw not supported exception in _performApiRequest
@mansisampat
Throw Operation Not Allowed for Invalid Auth Endpoint (#9013) (#9019)
28f00fa
@mansisampat
Throw Operation Not Allowed for Invalid Auth Endpoint (#9013) (#9019)
7f23bf4
@mansisampat
Implement exchangeToken public api
0f3970a
@mansisampat
Using expiry_in returned by backend
41ad562
@mansisampat mansisampat force-pushed the sammansi-gcip-byociam-web branch from fb70a71 to 41ad562 Compare May 19, 2025 06:08
NhienLam
NhienLam reviewed May 20, 2025
View reviewed changes
packages/auth/src/core/strategies/exhange_token.ts
* for an OidcToken i.e. Outbound Access Token.
*
* @remarks
* This method is implemented only for {@link DefaultConfig.REGIONAL_API_HOST}
Copy link
Contributor

Choose a reason for hiding this comment

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

{@link DefaultConfig.REGIONAL_API_HOST} here and in other comments are not generated in the above .md files.

Perhaps, can we use backtick around DefaultConfig.REGIONAL_API_HOST instead of link?

docs-devsite/auth.md

Asynchronously exchanges an OIDC provider's Authorization code or Id Token for an OidcToken i.e. Outbound Access Token.

This method is imeplemented only for and requires [TenantConfig](./auth.tenantconfig.md#tenantconfig_interface) to be configured in the [Auth](./auth.auth.md#auth_interface) instance used.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: implemented

Also, there's an extra whitespace between "for and" because {@link DefaultConfig.REGIONAL_API_HOST} isn't generated.

packages/auth/src/api/authentication/exchange_token.ts
@@ -0,0 +1,46 @@
/**
* @license
* Copyright 2020 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

2025

Same for other new files.

packages/auth/src/api/index.test.ts
@@ -595,4 +603,34 @@ describe('api/_performApiRequest', () => {
.and.not.have.property('tenantId');
});
});

context('throws Operation not allowed exception', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test a happy case for _performRegionalApiRequest to verify it sets the correct request, method, HTTP Headers, etc.

@NhienLam
Copy link
Contributor

Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NhienLam NhienLam NhienLam left review comments

@lisajian lisajian Awaiting requested review from lisajian lisajian is a code owner

@Xiaoshouzi-gh Xiaoshouzi-gh Awaiting requested review from Xiaoshouzi-gh Xiaoshouzi-gh is a code owner

@sam-gc sam-gc Awaiting requested review from sam-gc sam-gc is a code owner

@pashanka pashanka Awaiting requested review from pashanka pashanka is a code owner

@hsubox76 hsubox76 Awaiting requested review from hsubox76

@erikeldridge erikeldridge Awaiting requested review from erikeldridge

@visumickey visumickey Awaiting requested review from visumickey

@avolkovi avolkovi Awaiting requested review from avolkovi

@yoyomyo yoyomyo Awaiting requested review from yoyomyo

@zwu52 zwu52 Awaiting requested review from zwu52

@maneesht maneesht Awaiting requested review from maneesht

@tonyjhuang tonyjhuang Awaiting requested review from tonyjhuang

@aashishpatil-g aashishpatil-g Awaiting requested review from aashishpatil-g

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mansisampat @google-oss-bot @NhienLam