test: setup conformance test suite #60

raynerljm · 2023-06-08T10:09:25Z

Problem

As part of ensuring security and quality for sgID, we would like to ensure that our SDKs are conformant to the OIDC specifications. OIDC provides a conformance testing tool to check whether a relying party/server adheres to the standards laid out.

So far, we have been testing our TypeScript and Python SDK manually before releases as a sanity check (Tests are outlined and documented here. However, in an effort to improve efficiency and maintainability (especially if we decide to develop more SDKs), we would like automate these tests.

Related to #44

Solution

The demo-rp directory contains the demo app that will be interacting with the conformance tests.

This app is based off the Next.js CSR example app under examples/nextjs-csr.
Additionally, this demo app overrides the endpoints to point to the conformance tests instead of the sgID transaction server (the overriding can be found under test/conformance/demo-rp/src/lib/sgidClient.ts).

The suite directory contains the scripts that interact with the conformance test server to start the test plan and test modules, and interact with the demo app through API calls.

test.py is the main test runner that defines the config of the conformance test plan and runs each test module
conformance.py (modified based off this tutorial) is a wrapper over the API calls to the conformance test server and also includes the logic to interact with the locally hosted demo app
modules.py specifies the test modules (in the oidcc-client-test-plan test plan) to run and their expected final statuses and results

Limitations of the test suite

Some of the test modules (i.e. oidcc-client-test-signing-key-rotation) require the authorization flow to be ran twice which is currently not supported with the way the test suite is built - Therefore the test modules are commented out
- This could be resolved by customizing the script for certain test modules but I've not embarked on this unless we decide that it's worth the effort to invest more into this test suite
Some of the test modules test for features that we do not support (i.e. Webfinger, open id config from discovery, Aggregated/Distributed claims) and are thus commented out to reduce the flakiness of the test suite (as the behaviour is less predictable for these modules)

Screenshots

Tests

You can run the conformance suite by following the instructions under test/conformance/README.md

The tests will be run manually when needed and will not be integrated into our CI/CD pipeline because of the following reasons (which are also outlined in the README)

The tests take up to a few minutes to run which could be disruptive to our releases

The tests depend on the OIDC conformance test servers and we do not want the availability of those to influence our ability to make releases

The tests are flaky as they depend on making requests to the OIDC conformance test servers which could also be disruptive to our releases

New environment variables:

CONFORMANCE_TOKEN : permanent token from OpenID certification to interact with the conformance suite server

mantariksh

small initial comments before I read through the rest

test/unit/mocks/constants.ts

test/conformance/demo-rp/package.json

test/conformance/demo-rp/public/skyline.png

mantariksh · 2023-06-13T06:07:04Z

test/conformance/README.md

+     ```
+     to setup a Python virtual env
+   - Run `cat .env.example > .env` and repalce the `CONFORMANCE_TOKEN` with your [conformance testing permanent token](https://www.certification.openid.net/tokens.html) to setup your environment variables
+   - Run `pip install -r requirements.txt` to install the requirements


have we considered running these tests as part of our CI/CD pipeline? if we've decided against it, might be good to document why, probably in the PR description or in this README

thanks for the comment - documented in the README here aebdb25

mantariksh

could you say more about why you chose the NextJS app as the demo RP? I'm concerned about us maintaining an entire full-stack NextJS app for a test, especially in a year's time when NextJS (probably) will no longer be in fashion

test/conformance/README.md

mantariksh

overall, as I said before, tbh I'm still not convinced that the payoff-to-maintainability ratio is worth it for this, for a few reasons:

the conformance tests are not doing anything special that we can't do ourselves in unit tests, e.g. testing for invalid sub, invalid signature. the fact that they are published by the OpenID Foundation does not make them magical.
the conformance tests are opinionated (e.g. enforcing discovery, which is optional as per spec), and as a result we have to determine which tests are okay to fail based on the spec anyway. we might as well just write our own unit tests by referring to the spec itself.
the conformance tests require a lot of code to maintain, including a whole full stack web app, test driver code and monkey-patching of our own SDK, as seen in this PR. if the tests break in a year's time, it will be hard for us to figure out what's wrong.
the tests are extremely costly in terms of time, as they take 6.5 minutes to run. our CI currently takes 30-40 seconds.

will leave the final decision to jiehao!

test/conformance/demo-rp/package.json

mantariksh · 2023-06-13T06:55:03Z

test/conformance/README.md

+1. In the `test/conformance/demo-rp` directory, run `npm run dev`
+1. In a new terminal in the `test/conformance/suite` directory
+   - If you have not activate your Python venv, run `source .venv/bin/activate`
+   - Run `python test.py`


I ran this and got the following:

Failed modules: 3 / 25 ['oidcc-client-test-discovery-openid-config', 'oidcc-client-test-signing-key-rotation-just-before-signing', 'oidcc-client-test-signing-key-rotation']

is that expected?

sorry that i wasn't terribly clear about the expected behaviour. in this change a1dab4f, I've added code-level comments to the modules.py file with the name, description, and expected behaviour (+ comments) for each test module

I've also commented out the test modules that are

not supported by sgID (i.e. Webfinger, open id config from discovery, Aggregated/Distributed claims)

not currently supported with the way the test suite is ran (i.e.oidcc-client-test-signing-key-rotation)

I've updated the PR description with the above-mentioned limitations of the test suite

so right now all the tests should be passing

mantariksh · 2023-06-13T07:01:31Z

test/conformance/demo-rp/src/pages/api/userinfo.ts

+  }
+
+  // Request user info with access token
+  const { data } = await sgidClient.userinfo({


does the decryption always fail because the test payload isn't encrypted? might be good to reflect that in a comment

the OIDC server returns an empty object for the userinfo payload

as a result, the decryptPayload function will not be called during execution of the test suite.

here is a test that proves that it is not actually called

modified SDK code:

server logs:

thanks for the comment though! I'll make a note of this in the README + make a code level comment about this behaviour

added the comments in the README and code level here aebdb25

additionally removed images from conformance testing demo app to remove bloat additionally fixed typos in the conformance testing README

raynerljm · 2023-06-20T02:53:05Z

@mantariksh

could you say more about why you chose the NextJS app as the demo RP? I'm concerned about us maintaining an entire full-stack NextJS app for a test, especially in a year's time when NextJS (probably) will no longer be in fashion

i chose the Next.js app (more specifically the CSR one) as the demo RP for the following reasons

i wanted to utilize one of the example apps in the examples directory. this is so that if we had to update the example apps (due to a change in the SDK), the additional effort required to update the demo RP for the conformance test would be minimal
among the example apps, the Next.js apps can mimic the full E2E RP flow (FE to BE) from a single server (compared to the express example which requires spinning up an additional server to serve the frontend files as well.

this is why the implementation of the demo RP is essentially just the Next.js CSR example app but with a modified lib/sgidClient.ts file to interact with the conformance test server instead.

to facilitate the discussion, do you have any suggestions for a demo RP that could potentially be easier to maintain?

additionally add comments to README and `userinfo` endpoint about how the decryption of the payload will not be attempted

mantariksh · 2023-06-20T03:22:26Z

to facilitate the discussion, do you have any suggestions for a demo RP that could potentially be easier to maintain?

another couple of options (not mutually exclusive):

Go framework-free for the frontend. Use vanilla JS and just have one login button which does a fetch to get the auth URL. So we don't have to maintain a whole React app for the tests.
Use Express for the backend and have an endpoint to serve static files.

agree that NextJS does more stuff for you, but imo it'll be harder to maintain in future as NextJS and React keep upgrading. Express may upgrade too but if we just use it for a backend + static files, we'd have very little surface area which might be affected by breaking changes.

kwajiehao

lgtm, left some nit comments

test/conformance/README.md

test/conformance/demo-rp/src/lib/sgidClient.ts

test/conformance/demo-rp/src/pages/api/auth-url.ts

test/conformance/demo-rp/src/pages/api/callback.ts

test/conformance/suite/modules.py

kwajiehao · 2023-06-28T08:24:57Z

overall, as I said before, tbh I'm still not convinced that the payoff-to-maintainability ratio is worth it for this, for a few reasons:

the conformance tests are not doing anything special that we can't do ourselves in unit tests, e.g. testing for invalid sub, invalid signature. the fact that they are published by the OpenID Foundation does not make them magical.

the conformance tests are opinionated (e.g. enforcing discovery, which is optional as per spec), and as a result we have to determine which tests are okay to fail based on the spec anyway. we might as well just write our own unit tests by referring to the spec itself.

the conformance tests require a lot of code to maintain, including a whole full stack web app, test driver code and monkey-patching of our own SDK, as seen in this PR. if the tests break in a year's time, it will be hard for us to figure out what's wrong.

the tests are extremely costly in terms of time, as they take 6.5 minutes to run. our CI currently takes 30-40 seconds.

Personally I think that it's worth it to maintain these conformance tests. The main reason for doing so is to offload the burden of maintaining the test cases to OpenID Foundation. Quick response to the points you've raised:

We can certainly write our own unit tests but these are harder to maintain because we need to do them for each language we have a SDK in. By deferring to the OpenID Foundation test server, we just need to provide a monkey patch for the sgID client.
As you've pointed out, we don't actually need to update the test app - we just need to make sure that the sgID client can run the test cases. As long as we freeze our dependencies for the test app, there should be no breakage due to dependency updates and any failed tests will be cases that we should rightly investigate.
Since we don't release often, a few minutes for tests is not a heavy cost to pay

Also keen to hear your thoughts @raynerljm !

Additionally renamed `callback.ts` to `redirect.ts` to follow the Next.js CSR example. Also renamed `result` to `expected-result`. Updated README to explain how the test suite works

raynerljm · 2023-06-30T08:45:11Z

i was previously more on the fence on whether we should merge these tests as I wasn't sure whether it was wise to bog down the sgid-client repository with all the conformance-suite related code.

however, after refactoring the suite code into its own repository and leaving the monkey patch code in the relevant SDK, I believe its fine to continue with the merge + with the maintenance of the conformance suite. in the event that we do find that the cost is not worth the benefit, we can just stop using the suite and leave the repository alone instead of having to delete this portion in every single SDK.

kwajiehao · 2023-07-03T03:08:46Z

test/conformance/README.md

+   - Run `pip install -r requirements.txt` to install the requirements
+   - Run `cat .env.example > .env` and replace the `CONFORMANCE_TOKEN` with your [conformance testing permanent token](https://www.certification.openid.net/tokens.html) to setup your environment variables
+5. Additional notes
+   - Please ensure that the value of `test_plan_config.alias` in `test/conformance/suite/test.py` is the same as the last path parameter in `hostname` in `test/conformance/demo-rp/src/lib/sgidClient.ts` (e.g. `sgid-sdk-conformance-test`)


This refers to a file which no longer exists - test/conformance/suite/test.py

thanks for the catch! 🎉 - updated in 71be529

kwajiehao

lgtm

instructions on the new repo are a bit confusing because they reference files and directories in another repo (for e.g. test/conformance/demo-rp) but we can deal with that later

left a nit comment on docs

raynerljm marked this pull request as draft June 8, 2023 10:09

raynerljm force-pushed the test/oidc branch from 920249b to d95846a Compare June 8, 2023 10:12

feat: setup conformance test suite

1d6fe88

raynerljm force-pushed the test/oidc branch from d95846a to 1d6fe88 Compare June 9, 2023 01:58

raynerljm changed the title ~~(WIP) test: setup conformance test suite~~ test: setup conformance test suite Jun 9, 2023

raynerljm requested a review from kwajiehao June 9, 2023 02:17

raynerljm marked this pull request as ready for review June 9, 2023 02:17

mantariksh reviewed Jun 13, 2023

View reviewed changes

raynerljm added 2 commits June 14, 2023 10:12

refactor: move jest config files into test/unit

f008af6

fix: point example and demo apps to sdk in npm

07384b4

additionally removed images from conformance testing demo app to remove bloat additionally fixed typos in the conformance testing README

docs: update README with reasons not to include in CI/CD pipeline

aebdb25

additionally add comments to README and `userinfo` endpoint about how the decryption of the payload will not be attempted

fix: update test modules with documentation

a1dab4f

kwajiehao approved these changes Jun 28, 2023

View reviewed changes

raynerljm added 2 commits June 30, 2023 12:04

doc: update README and add code-level comments

7c69dc3

Additionally renamed `callback.ts` to `redirect.ts` to follow the Next.js CSR example. Also renamed `result` to `expected-result`. Updated README to explain how the test suite works

refactor: refactor the conformance test suite into its own repository

0246a91

kwajiehao reviewed Jul 3, 2023

View reviewed changes

kwajiehao approved these changes Jul 3, 2023

View reviewed changes

kwajiehao linked an issue Jul 3, 2023 that may be closed by this pull request

Investigate OpenID foundation's RP conformance testing process #44

Closed

docs: update conformance suite README directory name

71be529

raynerljm merged commit 2c53240 into develop Jul 3, 2023

raynerljm deleted the test/oidc branch July 3, 2023 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: setup conformance test suite #60

test: setup conformance test suite #60

raynerljm commented Jun 8, 2023 •

edited

Loading

mantariksh left a comment

mantariksh Jun 13, 2023

raynerljm Jun 20, 2023

mantariksh left a comment

mantariksh left a comment

mantariksh Jun 13, 2023

raynerljm Jun 20, 2023

mantariksh Jun 13, 2023

raynerljm Jun 20, 2023

raynerljm Jun 20, 2023

raynerljm commented Jun 20, 2023

mantariksh commented Jun 20, 2023

kwajiehao left a comment

kwajiehao commented Jun 28, 2023 •

edited

Loading

raynerljm commented Jun 30, 2023

kwajiehao Jul 3, 2023

raynerljm Jul 3, 2023

kwajiehao left a comment •

edited

Loading

test: setup conformance test suite #60

test: setup conformance test suite #60

Conversation

raynerljm commented Jun 8, 2023 • edited Loading

Problem

Solution

Limitations of the test suite

Screenshots

Tests

mantariksh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantariksh left a comment

Choose a reason for hiding this comment

mantariksh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raynerljm commented Jun 20, 2023

mantariksh commented Jun 20, 2023

kwajiehao left a comment

Choose a reason for hiding this comment

kwajiehao commented Jun 28, 2023 • edited Loading

raynerljm commented Jun 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwajiehao left a comment • edited Loading

Choose a reason for hiding this comment

raynerljm commented Jun 8, 2023 •

edited

Loading

kwajiehao commented Jun 28, 2023 •

edited

Loading

kwajiehao left a comment •

edited

Loading