Skip to content
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

Improve our approach for testing auth (part 1) #9681

Merged
merged 29 commits into from
Feb 21, 2024

Conversation

jNullj
Copy link
Collaborator

@jNullj jNullj commented Oct 23, 2023

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?)

Change auth tests to include all shields of the base class.
The code is formated to be used in more general cases and increases code reuseability.
@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2023

Messages
📖 ✨ Thanks for your contribution to Shields, @jNullj!

Generated by 🚫 dangerJS against a2b838c

@jNullj
Copy link
Collaborator Author

jNullj commented Jan 6, 2024

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
image

@jNullj jNullj marked this pull request as ready for review January 6, 2024 17:37
@jNullj jNullj requested a review from chris48s January 6, 2024 17:37
@jNullj jNullj added core Server, BaseService, GitHub auth, Shared helpers javascript [dependabot only] Pull requests that update Javascript packages labels Jan 6, 2024
@chris48s
Copy link
Member

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

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I've left some comments. I think having seen this implementation, I'm now actually leaning towards the second option I mentioned in #9493 (comment) but it is really useful to see what you've done here.

services/stackexchange/stackexchange-base.spec.js Outdated Show resolved Hide resolved
services/stackexchange/stackexchange-base.spec.js Outdated Show resolved Hide resolved
services/stackexchange/stackexchange-base.spec.js Outdated Show resolved Hide resolved
We already test all existing classes, no need for a dummy
Add getBadgeExampleCall to extract the first OpenAPI example then reformat it for service invoke function.
Add the testAuth function which tests auth of a service (badge) using a provided dummy response.
@jNullj
Copy link
Collaborator Author

jNullj commented Jan 20, 2024

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?


cleanUpNockAfterEach()

const config = { private: { stackapps_api_key: 'fake-key' } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to the stackexchange badges. If this is going to be a completely generic helper we can use for all services, we'll need to pass this into testAuth as a param. I'd suggest we make the config object the first or second param to testAuth(). This one is small, so lets address this one before merging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So i went a bit overboard and just made everything generic and support all auth methods.
So now i have a function to generate a fake config object based on the service class.
See ffc7800


const scope = nock(authOrigin)
.get(/.*/)
.query(queryObject => queryObject.key === 'fake-key')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is a bit specific to this service. This will work for services that pass auth in the query string, but won't for services where we pass auth in a header, for example. We're probably going to need a few different variants of this function for different auth methods (basic auth, header, query string, etc). I don't think that will be too hard - there's a few different ways we can do this.

I think I'd be fine just merging this how it is and saying at the moment it only works for querystring auth. Then we work out how to generalise this as we expand more services to use this helper. I usually find it easier to think about this stuff once I have a concrete problem I am trying to apply it to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See ffc7800, its all more generic now and should handle most service classes

services/test-helpers.js Show resolved Hide resolved
@chris48s
Copy link
Member

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.

Add all auth methods used to testAuth to be generic and used by all services.
Add helper functions to make testAuth more readable
@jNullj
Copy link
Collaborator Author

jNullj commented Feb 10, 2024

Oh nvm, i will add another fix tomorrow

@jNullj jNullj changed the title [stackexchange] Improve our approach for testing auth (part 1) [stackexchange,obs,discord] Improve our approach for testing auth (part 1) Feb 11, 2024
@chris48s chris48s changed the title [stackexchange,obs,discord] Improve our approach for testing auth (part 1) [stackexchange obs discord] Improve our approach for testing auth (part 1) Feb 13, 2024
@chris48s
Copy link
Member

chris48s commented Feb 13, 2024

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
Copy link
Collaborator Author

jNullj commented Feb 13, 2024

@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.

Use apiHeaderKey & bearerHeaderKey as function params rather then extracting them with regex from function strings.

Those options are now part of an options object param joined with the contentType that replaces header.

header was originaly added for setting content type of the reply, so it makes more sense to directly set the content type
@jNullj
Copy link
Collaborator Author

jNullj commented Feb 13, 2024

Will fix this tomorrow

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

Before this commit the QueryStringAuth would only work for the key of stackexchange.
This commit makes the testAuth function generic and allows passing user and pass keys.
@jNullj
Copy link
Collaborator Author

jNullj commented Feb 16, 2024

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

Might set wrong header for jwt login request.
This commit fixes that.
Some services might have more then one authOrigin.
This commit makes sure we test for redundent authOrigins as well as support requests to them if needed.
Prior to this change, JwtAuth testing would lead to erros due to the absence of a specified login endpoint,
Nock would be dumplicated for both login and non login hosts and indicate a missing request.

This commit enforces the requirement for a new jwtLoginEndpoint argument when testing JwtAuth.
The argument seperates the endpoint nock scope from the behavior of the request nock.
@jNullj jNullj self-assigned this Feb 17, 2024
@jNullj jNullj changed the title [stackexchange obs discord] Improve our approach for testing auth (part 1) [stackexchange obs discord pepy docker] Improve our approach for testing auth (part 1) Feb 17, 2024
@jNullj
Copy link
Collaborator Author

jNullj commented Feb 17, 2024

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
Copy link
Member

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

Comment on lines 112 to 114
if (!fakeauthorizedOrigins || typeof fakeauthorizedOrigins !== 'object') {
throw new TypeError('Invalid fakeauthorizedOrigins: Must be an array.')
}
Copy link
Member

@chris48s chris48s Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check and exception text don't seem to match here

@chris48s chris48s changed the title [stackexchange obs discord pepy docker] Improve our approach for testing auth (part 1) Improve our approach for testing auth (part 1) Feb 21, 2024
@chris48s
Copy link
Member

Cool. This looks great.

Just FYI, if you want to follow up more of these #9493 (comment) 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.

@chris48s chris48s added this pull request to the merge queue Feb 21, 2024
Merged via the queue into badges:master with commit 8ab9dfa Feb 21, 2024
22 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers javascript [dependabot only] Pull requests that update Javascript packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants