Bugfix/dup port createpayload#58
Conversation
phalestrivir
left a comment
phalestrivir
left a comment
There was a problem hiding this comment.
Looks good, although I noticed there is another bug that we may want to fix here. See my comments below
| import { state } from '../index'; | ||
| import * as AdminOps from './AdminOps'; | ||
|
|
||
| describe('Test AdminOps.getAccessTokeUrl', () => { |
There was a problem hiding this comment.
Fix the typo here. Also, while you're at it, I would structure this test file as follows to be consistent with the rest of Frodo:
describe('AdminOps', () => {
describe('getAccessTokenUrl()', () => {
...
});
});The reason for this structure is in case we end up adding other tests in the future for other functions within AdminOps, then they can be grouped under the AdminOps tests
There was a problem hiding this comment.
Good call - fixed in my first fixup commit, currently commit 012e1af.
| test('0: default to port 80', async () => { | ||
| state.setHost("http://trivirsample.com") | ||
| const payload = AuthenticateOps.createPayload('serviceAcctID', state.getHost()); | ||
| expect(payload.aud).toBe("http://trivirsample.com:80//oauth2/access_token") |
There was a problem hiding this comment.
You may have exposed another bug here that we should probably fix. You notice that it has the // before oauth2, making this an invalid endpoint.
This doesn't happen if you set the host to "http://trivirsample.com/am", which would result in "http://trivirsample.com:80/am/oauth2/access_token". The user is supposed to include the /am part anyways, so this isn't usually an issue, but if they don't include that /am part at the end I'm thinking it should be "http://trivirsample.com:80/oauth2/access_token" and not "http://trivirsample.com:80//oauth2/access_token".
You should probably add a few more test cases as well to test having endpoints at the end of the host url, such as:
There was a problem hiding this comment.
Wow, good eyes! My bad on the '//' item. Resolved in fixup commit: f184eaa7.
|
The other thing I forgot to mention is make sure to rebase this branch against main, and change the PR to point to main instead of trivir. The main branch is what this should be based on since that branch matches Rockcarver's main branch. Feel free to squash your commits as well. When you are all done, also run |
akynaston
force-pushed
the
bugfix/dup-port-createpayload
branch
from
85fcd93 to
f184eaa
Compare
… already present. The createPayload function had a bug that would add the port even if it was already there. This fix and test ensures if the port is already present, the port will be left as is in the resulting JWT. If someone provided the port as shown here, then createPayload would add it again, making the JWT audience invalid. Example configured line, including port 443. C:\Users\akynaston>frodo conn list https://openam-trivir-fairfax.forgeblocks.com:443/am Any unique substring or alias of a saved host can be used as the value for host parameter in all commands C:\Users\akynaston>
…ng, no bugs found.
…load if already present.
akynaston
force-pushed
the
bugfix/dup-port-createpayload
branch
from
f184eaa to
f7e5137
Compare
|
Ready for your review Preston, I've made the changes you requested, and rebased on main. |
|
I'll be sure to squash all 4 commits down to 1 when I create my pr on the rock-carver version. |
2 participants
This update has a test for each of the locations we add on a :443 or :80 to the url:
AdminOps.ts - getAccessTokenUrl
AuthenticateOps.ts - createPayload
The AdminOpts.ts instance did not have any bugs that I could find; so I just added a test, and exported the function being tested. The AuthenticateOps.ts did have a bug in createPayload - if the port was already provided, it was added again. This fix avoids that issue and proves the fix with tests.