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.

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

feat(auth): Added code flow support for OIDC flow. #1220

Merged
merged 17 commits into from
May 25, 2021
Merged

feat(auth): Added code flow support for OIDC flow. #1220

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d6681cc
OIDC codeflow support
xil222 Apr 6, 2021
3a925a5
improve configs to simulate the real cases
xil222 Apr 6, 2021
4f2e7bc
update for changes in signiture
xil222 Apr 6, 2021
3aacc80
Merge branch 'xinxinxin-codeflow' of https://github.com/firebase/fire…
xil222 Apr 6, 2021
4b86338
resolve comments
xil222 Apr 15, 2021
e565db3
improve validator logic
xil222 Apr 15, 2021
0a36501
remove unnecessary logic
xil222 Apr 19, 2021
92202e6
Merge branch 'master' into xinxinxin-codeflow
xil222 Apr 19, 2021
01f0c34
add tests and fix errors
xil222 Apr 20, 2021
14034d2
Merge branch 'xinxinxin-codeflow' of https://github.com/firebase/fire…
xil222 Apr 20, 2021
4fb8597
add auth-api-request rests
xil222 Apr 23, 2021
82ea5bb
Merge branch 'master' into xinxinxin-codeflow
xil222 Apr 23, 2021
b87a4ce
Merge branch 'master' into xinxinxin-codeflow
xil222 Apr 27, 2021
5dd52df
Merge branch 'master' into xinxinxin-codeflow
xil222 May 4, 2021
0a0cd0d
Merge branch 'master' into xinxinxin-codeflow
xil222 May 11, 2021
e824a41
Merge branch 'master' into xinxinxin-codeflow
xil222 May 19, 2021
2db147e
Merge branch 'master' into xinxinxin-codeflow
xil222 May 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions src/auth/auth-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -810,13 +810,11 @@ export class OIDCConfig implements OIDCAuthProviderConfig {
});

const idToken = options.responseType.idToken;
if (typeof idToken !== 'undefined') {
if (!validator.isBoolean(idToken)) {
throw new FirebaseAuthError(
AuthClientErrorCode.INVALID_ARGUMENT,
'"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.',
);
}
if (typeof idToken !== 'undefined' && !validator.isBoolean(idToken)) {
throw new FirebaseAuthError(
AuthClientErrorCode.INVALID_ARGUMENT,
'"OIDCAuthProviderConfig.responseType.idToken" must be a boolean.',
);
}

const code = options.responseType.code;
Expand All @@ -829,7 +827,7 @@ export class OIDCConfig implements OIDCAuthProviderConfig {
}

// If code flow is enabled, client secret must be provided.
xil222 marked this conversation as resolved.
Show resolved Hide resolved
if (typeof options.clientSecret === 'undefined') {
if (code === true && typeof options.clientSecret === 'undefined') {
xil222 marked this conversation as resolved.
Show resolved Hide resolved
throw new FirebaseAuthError(
AuthClientErrorCode.MISSING_OAUTH_CLIENT_SECRET,
'The OAuth configuration client secret is required to enable OIDC code flow.',
Expand Down Expand Up @@ -897,8 +895,8 @@ export class OIDCConfig implements OIDCAuthProviderConfig {
providerId: this.providerId,
issuer: this.issuer,
clientId: this.clientId,
clientSecret: this.clientSecret,
responseType: this.responseType,
clientSecret: deepCopy(this.clientSecret),
responseType: deepCopy(this.responseType),
};
}
}
26 changes: 23 additions & 3 deletions test/unit/auth/auth-api-request.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3503,12 +3503,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
enabled: true,
clientId: 'CLIENT_ID',
issuer: 'https://oidc.com/issuer',
clientSecret: 'CLIENT_SECRET',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am concerned about legacy customers and would recommend keeping a test without clientSecret and responseType. Can you add new tests instead of modifying the existing ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add tests for createOAuthIdpConfig and updateOAuthIdpConfig

responseType: {
code: true,
},
};
const expectedRequest = {
displayName: 'OIDC_DISPLAY_NAME',
enabled: true,
clientId: 'CLIENT_ID',
issuer: 'https://oidc.com/issuer',
clientSecret: 'CLIENT_SECRET',
responseType: {
code: true,
},
};
const expectedResult = utils.responseFrom(deepExtend({
name: `projects/project1/oauthIdpConfigs/${providerId}`,
Expand Down Expand Up @@ -3594,12 +3602,20 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
enabled: true,
clientId: 'CLIENT_ID',
issuer: 'https://oidc.com/issuer',
clientSecret: 'CLIENT_SECRET',
responseType: {
code: true,
},
};
const expectedRequest = {
displayName: 'OIDC_DISPLAY_NAME',
enabled: true,
clientId: 'CLIENT_ID',
issuer: 'https://oidc.com/issuer',
clientSecret: 'CLIENT_SECRET',
responseType: {
code: true,
},
};
const expectedResult = utils.responseFrom(deepExtend({
name: `projects/project_id/oauthIdpConfigs/${providerId}`,
Expand All @@ -3611,10 +3627,14 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
enabled: false,
clientId: 'NEW_CLIENT_ID',
issuer: 'https://oidc.com/issuer2',
clientSecret: 'CLIENT_SECRET',
responseType: {
code: true,
},
}));

it('should be fulfilled given full parameters', () => {
const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId';
const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code';
const stub = sinon.stub(HttpClient.prototype, 'send').resolves(expectedResult);
stubs.push(stub);

Expand Down Expand Up @@ -3708,7 +3728,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
});

it('should be rejected when the backend returns a response missing name', () => {
const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId';
const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code';
const expectedError = new FirebaseAuthError(
AuthClientErrorCode.INTERNAL_ERROR,
'INTERNAL ASSERT FAILED: Unable to update OIDC configuration',
Expand All @@ -3728,7 +3748,7 @@ AUTH_REQUEST_HANDLER_TESTS.forEach((handler) => {
});

it('should be rejected when the backend returns an error', () => {
const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId';
const expectedPath = path + '?updateMask=enabled,displayName,issuer,clientId,clientSecret,responseType.code';
const expectedServerError = utils.errorFrom({
error: {
message: 'INVALID_CONFIG',
Expand Down
12 changes: 8 additions & 4 deletions test/unit/auth/auth-config.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,10 +788,14 @@ describe('OIDCConfig', () => {
});

it('should set readonly property expected responseType', () => {
const expectedResponseType = {
code: true,
};
expect(config.responseType).to.deep.equal(expectedResponseType);
expect(config.responseType).to.deep.equal({ code: true });
});

it('should not throw on no responseType and clientSecret', () => {
const testResponse = deepCopy(serverResponse);
delete testResponse.clientSecret;
delete testResponse.responseType;
expect(() => new OIDCConfig(testResponse)).not.to.throw();
});

it('should throw on missing issuer', () => {
Expand Down