-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
SSO Siteminder Tests #147
SSO Siteminder Tests #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a few comments but overall looks good!
jobs: | ||
matrix-prep: | ||
name: Generate matrix | ||
runs-on: ubuntu-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a specific runner version
to prevent unexpected errors as the latest version is upgraded on GitHub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
-e CLUSTER=${{ github.event.inputs.cluster }} \ | ||
-v $(pwd)/results:/app/results \ | ||
--add-host=${{ needs.matrix-prep.outputs.hostname }}:${{ matrix.ip }} \ | ||
$(docker build -q .) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks great but in case we want to run this dispatcher more than one time and building the image is time-consuming, consider cache
the docker layers
to reduce the build time for the next time. e.g.
https://github.com/bcgov/sso-switchover-agent/blob/main/.github/workflows/pre-commit.yml#L29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep that's good idea to have caching. Added it.
|
||
- `SITEMINDER_TESTS_DATACENTERS` stores a json payload in below format | ||
|
||
```json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this could be formatted in little nicer?
```json
{
"<ENVIRONMENT>": {
"datacenters": [
{ "name": "<DATACENTER>", "ip": "<DATACENTER-IP>" },
{ "name": "<DATACENTER>", "ip": "<DATACENTER-IP>" }
],
"hostname": "<HOSTNAME>"
}
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@junminahn The github action secret is stored in form of a JSON string but not actual JSON. So it is ideal to store it in its original format.
siteminder-tests/util.js
Outdated
await page.type('#password', idp_password); | ||
|
||
await page.keyboard.press('Enter'); | ||
var isIDIR = test_name.indexOf('IDIR'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- let's use 'const' over 'var'
- the variable name
isIDIR
is good but the statement is not matching to the name; so it could be
const isIDIR = test_name.indexOf('IDIR') > -1;
if (!isIDIR) {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
siteminder-tests/util.js
Outdated
const decodedXML = decodeBase64(cleanSamlResponse); | ||
const jsonResult = await parseStringSync(decodedXML); | ||
const assertion = _.get(jsonResult, 'Response.ns2:Assertion.0'); | ||
//const subject = _.get(assertion, 'ns2:Subject.0.ns2:NameID.0._'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove unused comments unless you want to use them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
uses: actions/upload-artifact@v2 | ||
with: | ||
name: ${{ github.event.inputs.cluster }}-${{ github.event.inputs.environment }}-${{ matrix.name }}-results | ||
path: ${{ github.workspace }}/siteminder-tests/results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to generate a results
folder in the codebase or is github.workspace
something other than the root of the project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thegentlemanphysicist yes the tests would generate a results
folder under siteminder-tests
directory.
@@ -0,0 +1,72 @@ | |||
# IDIR - Environment Agnostic | |||
IDIR_USERNAME= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the teams test IDIR? or will it be a personal idir?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thegentlemanphysicist yes, that's going to be teams test IDIR.
Fixes:
https://trello.com/c/iyOavXXy/1441-codereview-siteminder-test-script-to-include-saml-payload-validation
https://trello.com/c/XU97SNPi/1442-siteminder-test-script-automation