[stackexchange obs discord pepy docker] Improve our approach for testing auth (part 1)

[jNullj]

This PR refs #9493

As mentioned in the issue, this is a first attempt to improve auth testing with stackexchange as a study case and hopefully a first step. I tried to stick with making everything testable from within the same file while reusing the same code. If testAuth is very reusable it might be better to move it into test-helpers.js to save a few lines of code, i suspect we might be able to use it for most tests saving a few lines of code per test.

I think this could be improved if i had a way to generate the dummyResponse automatically from the oneapi object (should be possible in theory, there are some unmaintained modules that could do that, should i make my own to only cover what is needed by shields?)

[github-actions[bot]]

	Messages
:book:	:sparkles: Thanks for your contribution to Shields, @jNullj!

Generated by :no_entry_sign: dangerJS against a2b838c4575d3a8edef19f8c01f0e3555c063d42

[jNullj]

Here is an example for auth test of stackexchange as a case study for the rest of the project. If it looks good I can try and apply this pattern over the project in chunks

[chris48s]

Sorry I haven't managed to look at this one yet. I will make sure to follow this one up over the weekend.

[jNullj]

I did everything we talked about so far in the latest commits but i do have a third option to consider. I noticed we have the authorizedOrigins property. I could potentially use that to make one loop that finds all classes that can be tested for auth in one go, this would be a spec file at the root of the services called something like authTest.spec.js and it could do the following:

1. Go over all services and find everything that has authorizedOrigins and OpenAPI example
2. Run the authTest function for each of those

The only catch is that I will have to find a way to craft dummy responses automatically. But that would remove the need of manually going over all services to add tests, and also cover every future service added automatically assuming it has an OpenAPI example.

Do you think that's over-complicating or should i give that a try? Would that fit well with the project style and code philosophy?

[chris48s]

Nice. Latest iteration of this looks cool.

but i do have a third option to consider....

What you've suggested would work, but I also think the latest iteration of this PR looks great. Feels like the direction you are already heading is a big improvement. As you note, making a valid example of a fake response in a generic way could be non-trivial for some service families. Maybe for some services families where we've just got a lot of badges that all call the exact same API endpoint and report a different value, we could define a single example response shared by all test cases. Maybe it is good to think of this as a pattern that is available to us when it makes sense, rather than one to apply in all situations.

[jNullj]

Oh nvm, i will add another fix tomorrow

[chris48s]

Thanks for looking at all the additional auth methods. I think the approach of converting our own source code to text and attempting to parse javascript with regex is not a good solution to this though. It is going to create a situation where changes to the AuthHelper can have quite non-obvious knock-on effects.

Broadly, there are 2 ways we can handle this:

1. Pass in params when we call testAuth that specify
   • the auth method and
   • custom header name, if applicable
2. Refactor auth helper so that we store all the auth info in properties on the service class.

Option 1 is kind of less nice because there will be a little bit of duplication between the service class and the tests. We specify the auth method twice, and in some cases we might specify the header name twice too. Option 2 is perhaps the most ideal solution, but it is also way more of a lift and a big scope creep from the original intent of this. We risk straying far from the original point by embarking on that now.

I'd be in favour of option 1, at least to start with.

[jNullj]

@chris48s While the current regex-based approach works for testing, it might not be ideal for long-term maintainability. Could we explore alternative methods like class properties for improved robustness and clarity? Would love to hear if you got other ideas from your experience.

edit: we posted nearly at the same time, so i just got the above response after posting.

I think option 1 would work best to test out this new approach for auth testing without making too much changes to the codebase.

[jNullj]

Will fix this tomorrow

 .query(queryObject => queryObject.key === fakeSecret)

[jNullj]

Ok, the last thing i want to do is add en example for each auth method in thie PR im only missing ApiKeyHeader and JwtAuth

[jNullj]

Fixed a few things and made sure we got at least 1 example for each authentication method. Testing seems to fail due to unrelated issue. Maybe we are missing pepy secret for api access on github?

[chris48s]

Thanks. The latest iteration of this looks really good. Only one minor comment

[chris48s]

Cool. This looks great.

Just FYI, if you want to follow up more of these https://github.com/badges/shields/issues/9493#issuecomment-1904056131 we can skip running the service tests on these PRs.

Putting the service names in square brackets runs the service tests - the ones in files called .tester.js. We don't run all of these on every PR because running them all is quite slow and they can be flaky.

The tests in files called .spec.js are always run, even if the .spec.js files live in /services. We run all of these on every PR because they're quick and reliable.

So if we're not actually modifying the service code, it is ok to skip the service tests.