Implements Auth provider configuration management APIs #415

bojeil-google · 2018-12-09T08:43:23Z

Implements Auth provider configuration management APIs for configuring SAML and OIDC providers.

listProviderConfigs(
  options: admin.auth.AuthProviderConfigFilter
): Promise<admin.auth.ListProviderConfigResults>;
getProviderConfig(providerId: string): Promise<admin.auth.AuthProviderConfig>
deleteProviderConfig(providerId: string): Promise<void>;
updateProviderConfig(
  providerId: string, updatedConfig: admin.auth.UpdateAuthProviderRequest
): Promise<admin.auth.AuthProviderConfig>;
createProviderConfig(
  config: admin.auth.AuthProviderConfig
): Promise<admin.auth.AuthProviderConfig>;

…g SAML and OIDC providers. ``` listProviderConfigs( options: admin.auth.AuthProviderConfigFilter ): Promise<admin.auth.ListProviderConfigResults>; getProviderConfig(providerId: string): Promise<admin.auth.AuthProviderConfig> deleteProviderConfig(providerId: string): Promise<void>; updateProviderConfig( providerId: string, updatedConfig: admin.auth.UpdateAuthProviderRequest ): Promise<admin.auth.AuthProviderConfig>; createProviderConfig( config: admin.auth.AuthProviderConfig ): Promise<admin.auth.AuthProviderConfig>; ```

hiranya911

Haven't gone into the unit tests yet. But I've pointed out a bunch of potential changes in the rest of the code. Looks mostly good. Lets try to reduce the use of any wherever possible.

hiranya911 · 2019-01-02T23:52:38Z

src/auth/auth-api-request.ts

+        request.pageSize > MAX_LIST_PROVIDER_CONFIGURATION_PAGE_SIZE) {
+      throw new FirebaseAuthError(
+        AuthClientErrorCode.INVALID_ARGUMENT,
+        `Required "maxResults" must be a positive non-zero number that does not exceed ` +


May be "positive integer that does not...". Positive implies non-zero.

hiranya911 · 2019-01-02T23:54:53Z

src/auth/auth-api-request.ts

  constructor(app: FirebaseApp) {
    this.httpClient = new AuthorizedHttpClient(app);
    this.authUrlBuilder = new AuthResourceUrlBuilder(utils.getProjectId(app), 'v1');
+    this.projectConfigUrlBuilder = new AuthResourceUrlBuilder(utils.getProjectId(app), 'v2beta1');


Nit: Refactor getProjectId(app) into a constant.

const projectId = utils.getProjectId(app); this.authUrlBuilder = ... this.projectConfigUrlBuilder = ...

hiranya911 · 2019-01-02T23:57:59Z

src/auth/auth-api-request.ts

+   *
+   * @param {number=} maxResults The page size, 100 if undefined. This is also the maximum
+   *     allowed limit.
+   * @param {string=} pageToken The next page token. If not specified, returns OIDC configurations starting


Remove "starting"

src/auth/auth-api-request.ts

hiranya911 · 2019-01-03T00:05:54Z

src/auth/auth-api-request.ts

+   * @param {string} providerId The provider identifier of the configuration to lookup.
+   * @return {Promise<object>} A promise that resolves with the provider configuration information.
+   */
+  public getOAuthIdpConfig(providerId: string): Promise<object> {


Can this be declared to return a more specific type than object?

test/integration/auth.spec.ts

test/resources/mocks.ts

src/utils/index.ts

src/auth/auth-api-request.ts

hiranya911

Looks pretty good. I've gone through both impl and tests, and made several comments.

I also think we probably should have done this in 2 PRs (one for OIDC and one for SAML). Lets work on making smaller PRs in the future.

hiranya911 · 2019-01-15T18:39:04Z

src/auth/auth.ts

+   *     meeting the filter requirements.
+   */
+  public listProviderConfigs(options: AuthProviderConfigFilter): Promise<ListProviderConfigResults> {
+    const processResponse = (response: any, providerConfigs: AuthProviderConfig[]) => {


Declare return type.

src/auth/auth.ts

hiranya911 · 2019-01-15T18:46:12Z

src/utils/index.ts

+  for (const key in obj) {
+    if (obj.hasOwnProperty(key) && typeof obj[key] !== 'undefined') {
+      let maskList: string[] = [];
+      maskList = generateUpdateMask(obj[key]);


const maskList = generateUpdateMask(...)

hiranya911 · 2019-01-15T18:48:00Z

src/auth/auth-api-request.ts

 export class FirebaseAuthRequestHandler {
  private httpClient: AuthorizedHttpClient;
  private authUrlBuilder: AuthResourceUrlBuilder;
+  private projectConfigUrlBuilder: AuthResourceUrlBuilder;


All these can be readonly.

hiranya911 · 2019-01-15T18:54:13Z

src/auth/auth-api-request.ts

+      pageToken,
+    };
+    // Remove next page token if not provided.
+    if (typeof request.pageToken === 'undefined') {


You can work around this by putting an explicit any on request. Or even:

const request: {pageSize: number, pageToken?: string} = ...;

is not too much of a hassle. But I'm ok if you choose to leave this as is for this PR.

hiranya911 · 2019-01-15T19:35:42Z

test/unit/auth/auth.spec.ts

+        // Stub listOAuthIdpConfigs to return expected response.
+        const listConfigsStub = sinon
+          .stub(FirebaseAuthRequestHandler.prototype, 'listOAuthIdpConfigs')
+          .returns(Promise.resolve(listConfigsResponse));


.resolves(listConfigsResponse). Here and elsewhere.

hiranya911 · 2019-01-15T19:36:22Z

test/unit/auth/auth.spec.ts

+        // Stub listOAuthIdpConfigs to throw a backend error.
+        const listConfigsStub = sinon
+          .stub(FirebaseAuthRequestHandler.prototype, 'listOAuthIdpConfigs')
+          .returns(Promise.reject(expectedError));


.rejects(expectedError). Here and elsewhere.

hiranya911 · 2019-01-15T19:36:59Z

test/unit/auth/auth.spec.ts

+          throw new Error('Unexpected success');
+        })
+        .catch((error) => {
+          expect(error).to.have.property('code', 'auth/argument-error');


Can't we do .should.eventually.be.rejected.and.have.property here?

hiranya911 · 2019-01-15T19:37:40Z

test/unit/auth/auth.spec.ts

+        providerConfigs: [],
+      };
+
+      it('should resolve on listInboundSamlConfigs request success with configs in response', () => {


should resolve on success with configs in response

hiranya911 · 2019-01-15T19:50:52Z

test/unit/auth/auth-api-request.spec.ts

+  describe('getOAuthIdpConfig()', () => {
+    const providerId = 'oidc.provider';
+    const path = `/v2beta1/projects/project_id/oauthIdpConfigs/${providerId}`;
+    const method = 'GET';


Nit: Calling this getMethod or httpGet would make the rest of the code more readable. Or even using the string literal as callParams(path, 'GET', {}) is not a bad idea.

Renamed to expectedHttpMethod here and all the new proposed APIs We need to assert this value in all the suite test. It is better not to hardcode it.

bojeil-google

Appreciate the review.
Regarding the PR being too long, as mentioned in another comment, the way we have been doing things, it is unlikely we can split into multiple PRs since partial features would be available in master before launchcals are approved.
What I can do going forward is split into smaller commits and request reviews one part at a time.

bojeil-google · 2019-01-15T21:24:51Z

src/auth/auth.ts

+   *     meeting the filter requirements.
+   */
+  public listProviderConfigs(options: AuthProviderConfigFilter): Promise<ListProviderConfigResults> {
+    const processResponse = (response: any, providerConfigs: AuthProviderConfig[]) => {


src/auth/auth.ts

bojeil-google · 2019-01-15T21:30:49Z

src/auth/auth-api-request.ts

+      throw new FirebaseAuthError(
+        AuthClientErrorCode.INVALID_ARGUMENT,
+        `Required "maxResults" must be a positive integer that does not exceed ` +
+        `the allowed ${MAX_LIST_PROVIDER_CONFIGURATION_PAGE_SIZE}.`,


bojeil-google · 2019-01-15T21:31:53Z

src/auth/auth-api-request.ts

 export class FirebaseAuthRequestHandler {
  private httpClient: AuthorizedHttpClient;
  private authUrlBuilder: AuthResourceUrlBuilder;
+  private projectConfigUrlBuilder: AuthResourceUrlBuilder;


src/auth/auth-api-request.ts

bojeil-google · 2019-01-16T01:42:23Z

test/unit/auth/auth.spec.ts

+          throw new Error('Unexpected success');
+        })
+        .catch((error) => {
+          expect(error).to.have.property('code', 'auth/argument-error');


bojeil-google · 2019-01-16T01:44:26Z

test/unit/auth/auth.spec.ts

+        providerConfigs: [],
+      };
+
+      it('should resolve on listOAuthIdpConfigs request success with configs in response', () => {


bojeil-google · 2019-01-16T01:55:54Z

test/unit/auth/auth.spec.ts

+        // Stub listOAuthIdpConfigs to return expected response.
+        const listConfigsStub = sinon
+          .stub(FirebaseAuthRequestHandler.prototype, 'listOAuthIdpConfigs')
+          .returns(Promise.resolve(listConfigsResponse));


bojeil-google · 2019-01-16T02:02:09Z

test/unit/auth/auth.spec.ts

+        // Stub listOAuthIdpConfigs to throw a backend error.
+        const listConfigsStub = sinon
+          .stub(FirebaseAuthRequestHandler.prototype, 'listOAuthIdpConfigs')
+          .returns(Promise.reject(expectedError));


bojeil-google · 2019-01-16T02:02:43Z

test/unit/auth/auth.spec.ts

+        providerConfigs: [],
+      };
+
+      it('should resolve on listInboundSamlConfigs request success with configs in response', () => {


hiranya911

Thanks for making the changes. LGTM 👍

hiranya911 · 2019-01-17T20:08:17Z

As for managing large changes, this is what I would recommend.

Create a branch for the new feature and enable branch protection for it (you can ask me or another Firebase org admin to do this for you).

git checkout -b auth-id-providers master
// enable protection for auth-id-providers

Then create a series of pull requests against the new branch. Branch protection will make sure that PRs have to be reviewed and CI checks must pass before they can be merged.

git checkout -b impl-part1 auth-idp-providers
// create PR, review and merge

git checkout -b impl-part2 auth-idp-providers
// create PR, review and merge

When the entire feature is implemented, we can merge the new branch to master. This can be done in a PR from the new branch to the master.

bojeil-google assigned hiranya911 Dec 11, 2018

Merge branch 'master' into provider-config

7d40d4a

hiranya911 suggested changes Jan 3, 2019

View reviewed changes

bojeil-google added 3 commits January 14, 2019 21:45

Addresses review comments.

e4574d0

Fixes missing type casting error in unit test.

26f7529

Fixes broken tests.

3a4970e

hiranya911 suggested changes Jan 15, 2019

View reviewed changes

Addresses additional comments.

73d4949

bojeil-google commented Jan 16, 2019

View reviewed changes

hiranya911 approved these changes Jan 17, 2019

View reviewed changes

hiranya911 assigned bojeil-google and unassigned hiranya911 Feb 10, 2019

bojeil-google added 3 commits April 11, 2019 18:47

Merge branch 'master' into provider-config

2683ee5

Added changelog note.

60ee5f0

Fixed changelog note.

eb66230

bojeil-google merged commit 9f770b4 into master Apr 16, 2019

hiranya911 deleted the provider-config branch May 18, 2021 18:45

Implements Auth provider configuration management APIs #415

Implements Auth provider configuration management APIs #415

Uh oh!

Conversation

bojeil-google commented Dec 9, 2018

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bojeil-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!