(OLD) refactor: Refactor and reorganize files and exports #39
Conversation
raynerljm
requested review from
PrawiraGenestonlia,
mantariksh,
kwajiehao and
kschiew
raynerljm
force-pushed
the
refactor/post-pkce
branch
from
452cfd1 to
6f27d56
Compare
| redirectUri = this.getFirstRedirectUri(), | ||
| codeChallenge, | ||
| }: AuthorizationUrlParams): AuthorizationUrlReturn { | ||
| if (this.apiVersion !== 2) { |
There was a problem hiding this comment.
See comments on #35 for this
| const expected = new URL(MOCK_AUTH_ENDPOINT_V2) | ||
| expect(actual.host).toBe(expected.host) | ||
| expect(actual.pathname).toBe(expected.pathname) | ||
| expect(actual.searchParams.get('client_id')).toBe(MOCK_CLIENT_ID) |
There was a problem hiding this comment.
nit: currently, there are many repetitive lines of expects across the tests. instead of writing attribute by attribute, what do you think of using either a string or object to compare (especially considering most values are checked against the default values)?
this allows you to construct a base, expected search params object which you can modify as needed to get the expected result
| expect(client).toBeDefined() | ||
| }) | ||
|
|
||
| it('should initialise correctly when a PKCS8 private key is provided', () => { |
There was a problem hiding this comment.
we can add a test case that checks for whether new lines are stripped from the private key
| */ | ||
| export const codeVerifierAndChallengePattern = /^[A-Za-z\d\-._~]{43,128}$/ | ||
| export async function decryptPayload( |
There was a problem hiding this comment.
Can we also have unit tests for decryptPayload?
| }) | ||
| }) | ||
|
|
||
| it('should generate authorisation URL correctly when state and codeChallenge is provided', () => { |
There was a problem hiding this comment.
Actually, we don't need state to be mandatory since we are mandating PKCE
| expect(actualNumSearchParams).toBe(8) | ||
| }) | ||
|
|
||
| it('should throw when no redirectUri is provided', () => { |
There was a problem hiding this comment.
We should have a negative test case for when no code challenge is provided
| * Regex pattern that the code verifier and code challenge in the PKCE flow should match according to the PKCE RFC | ||
| * https://www.rfc-editor.org/rfc/rfc7636 | ||
| */ | ||
| export const codeVerifierAndChallengePattern = /^[A-Za-z\d\-._~]{43,128}$/ |
There was a problem hiding this comment.
is this used purely in test code? if so, we should put it together with the test code instead of bundling it together with SDK code. it's good to be deliberate about what we export, as we have to support it forever or make a breaking change to remove it
There was a problem hiding this comment.
mm that's a very good point - shifted into the test code in this PR
raynerljm
force-pushed
the
feat/pkce-breaking
branch
from
ef32dbd to
d16757e
Compare
raynerljm
changed the title
|
Closed because most of these changes were implemented in #35 . |
(OLD): Some of these changes were implemented in #35 . I will make a new PR specifically for test-related changes subsequently
Problem
General refactoring of files and unit tests
Closes issue #38 https://github.com/orgs/datagovsg/projects/43/views/1?pane=issue&itemId=26650732
Depends on #35
Solution
Refactor the
SgidClientclass into its ownsgidClient.tsfileconstants.tsfilegeneratePkcePair) intogenerators.tsfile)decryptPayloadfunction into theutils.tsfile and expose it publiclyindex.tsjust as a tunnel to export the main exposed functions (i.e.sgidClient.ts,generators.ts, andutils.tsTests
All unit tests are still passing after the refactor