-
-
Notifications
You must be signed in to change notification settings - Fork 371
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
Introduce separate configuration for challenges #1083
Conversation
645f0c9
to
04ee7c2
Compare
This is now configurable, so we can define different documentation for different environments.
ceb5b58
to
3ca0af1
Compare
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.
Review in progress, first looks: pretty good! Thank you @nbaars !
Spotbugs spits out a few errors at https://github.com/OWASP/wrongsecrets/actions/runs/6909060649/job/18799652205?pr=1083: can you please have a look good sir? |
} | ||
|
||
@Override | ||
public int getTotalReceivedPoints() { | ||
return solvedChallenges.stream() | ||
.map(challenge -> challenge.difficulty() * (100 + (challenge.difficulty() - 1) * 25)) | ||
.map( |
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.
should we maybe just cache this value? it's just an int right?
src/main/java/org/owasp/wrongsecrets/challenges/ChallengesController.java
Show resolved
Hide resolved
So Ive just been running the tests locally. Positives
Negatives/questions |
@@ -48,15 +48,6 @@ jobs: | |||
with: |
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.
Does this whole ui-test section need removing?
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.
Yes, it is now part of the normal tests. You can run it as a normal unit-test with different Spring Boot configurations per Cypress test if necessary
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.
The tests are still there they are now part of the normal tests
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.
Oh yeah sorry, I was saying that so far you have just deleted a step in the ui-test section. I think the whole "ui-test:" object needs removing
A challenge has a name and a short name, which can be used in URLs etc. Using `shortName` instead of `url` makes more sense.
Next on last challenge should be disabled.
Link now points to the right URL and added a Cypress test to verify clicking the link.
Since we only need the contents in the CTF controller we can make this a bit more lazy.
The screenshots are saved, you can also run the Cypress test directly from Intellij. |
Re #1083 (comment): can we maybe have the "howto" added to the contributing.md ;) ? |
Yeah I think something to explain this to new users and we will be away! Apart from those answered concerns I think everything is an improvement. It feels loads cleaner!! |
@nbaars can you please have a look at the remaining issues at https://github.com/OWASP/wrongsecrets/actions/runs/6948950794/job/18906041764?pr=1083 so we can merge and fix more bugs in separate PRs? |
Created #1105 for future improvements once this is merged |
{ | ||
"reporterEnabled": "spec, mochawesome", | ||
"mochawesomeReporterOptions": { | ||
"reportDir": "cypress/reports/mochawesome", |
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 don't seem to be getting this mochawsome report when I run the tests. Should a report appear this in this path after each run?
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.
Weirdly I am also still not seeing screenshots on a failure
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.
@@ -4,14 +4,15 @@ This project uses [Cypress](https://www.cypress.io/) to run UI tests for the pro | |||
|
|||
## How to run the tests |
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 think a new section in here about how to use the visual debugger is important. Unless this is possible with maven. So we have one method of using it quickly and easily through unit tests and one way that is a little harder to run but way easier to debug visually. If youd like im happy to add this.
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 you add this to #1105 please :-)?
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.
Great improvements here, thanks a lot! 😁
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.
Thank you @nbaars for another big step forward! Now we can parallelize development of new challenges and do other improvements!
This PR split the configuration of a challenge into two parts:
Each definition can link to 1 or more challenges. This makes it possible to have different implementations and documentation for each environment. For example:
This example shows how we can define for different environments different hints. This also makes it possible to have a different
Challenge
class per environment.The configuration for the application is also part of the yaml configuration. For example, the environments, difficulty levels are now defined in the yaml file:
Adding a new difficulty level is as easy as adding a new entry in this yaml section. No code changes are necessary to add a new difficulty level. Same applies for categories and environments it only needs a config update.
The UI now treats every challenge the same. No more specific "challenge" code leaks into the main part of the application.
The order of the challenges is now defined by the configuration file, if you want to change the order you can simply change the order in the
yaml
file. Also if you want to runWrongSecrets
with a couple of challenges only need to change the yaml file.** Performance **
Starting time locally is now:
The memory consumption is around 90 Mb
Closes #502