feat(scan): add ability to run broken access control test with required options #270
Conversation
abright25
changed the title
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
abright25
changed the title
There was a problem hiding this comment.
Why is the file name plural? It seems the main entry is not collection.
There was a problem hiding this comment.
We will add more configurable tests in the future. BAC is not the only one test with options.
There was a problem hiding this comment.
Neither BrokenAccessControlTest nor BrokenAccessControlOptions are not the main entry in this file. The only entry is Test, not even Tests. So, the file should correspond to that.
| mockedCi, | ||
| mockedConfiguration | ||
| ) | ||
| reset<ApiClient | Configuration>(mockedApiClient, mockedConfiguration) |
There was a problem hiding this comment.
Now the tests is gonna behave incorrectly, CI mock is not being reset.
| mappedTests: string[], | ||
| testMetadata: Record<string, unknown> | ||
| ) { | ||
| if (!test.options?.auth) { |
There was a problem hiding this comment.
Lets TS enforce correctness instead of runtime checks
| typeof auth !== 'string' && | ||
| (!Array.isArray(auth) || auth.length !== 2) |
| continue; | ||
| } | ||
|
|
||
| if (test.name === 'broken_access_control') { |
There was a problem hiding this comment.
This check is technically redundant today, compiler knows the only single test with options
| if (!config.tests) { | ||
| return { ...config }; | ||
| } | ||
|
|
||
| const { mappedTests, testMetadata } = this.mapTests(config.tests); | ||
| const { tests: originalTests, ...restConfig } = config; | ||
|
|
||
| if (Object.keys(testMetadata).length > 0) { | ||
| const result: Record<string, unknown> = { | ||
| ...restConfig, | ||
| tests: mappedTests, | ||
| testMetadata | ||
| }; | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| return { ...config }; |
There was a problem hiding this comment.
The intent is unclear from both the name and implementation. Rename the method to reflect its role and avoid imperative style.
| if (!config.tests) { | |
| return { ...config }; | |
| } | |
| const { mappedTests, testMetadata } = this.mapTests(config.tests); | |
| const { tests: originalTests, ...restConfig } = config; | |
| if (Object.keys(testMetadata).length > 0) { | |
| const result: Record<string, unknown> = { | |
| ...restConfig, | |
| tests: mappedTests, | |
| testMetadata | |
| }; | |
| return result; | |
| } | |
| return { ...config }; | |
| const mapped = config.tests.map(test => this.mapTest(test)); | |
| const tests = mapped.map(t => t.name); | |
| const testMetadata = mapped.reduce<TestMetadata | undefined>( | |
| (acc, { metadata }) => | |
| metadata | |
| ? { ...acc, ...metadata } | |
| : acc, | |
| undefined | |
| ); | |
| return { | |
| ...config, | |
| tests, | |
| ...(testMetadata && { testMetadata }), | |
| }; |
| expect(result).toEqual({ id }); | ||
| }); | ||
|
|
||
| it('should throw error when broken_access_control test has no auth option', async () => { |
There was a problem hiding this comment.
This and tests below are no longer relevant due to https://github.com/NeuraLegion/sectester-js/pull/270/changes#r2692255661
| } | ||
|
|
||
| if (seenTestConfigurations.has(testName) || simpleTests.has(testName)) { | ||
| throw new Error(`Duplicate test configuration found: ${testName}`); |
There was a problem hiding this comment.
Error messages should include CTA
| throw new Error(`Duplicate test configuration found: ${testName}`); | |
| throw new Error(`Please remove a duplicate for the ${testName} test`); |
2 participants
No description provided.