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

Ctf improvments #295

Merged
merged 12 commits into from
Apr 13, 2017
Merged

Ctf improvments #295

merged 12 commits into from
Apr 13, 2017

Conversation

J12934
Copy link
Member

@J12934 J12934 commented Apr 13, 2017

Hi,
we have hosted a couple of CTF events in the last month and have made a few improvements to the challenge notification we would like to merge back to you ;)

The improvements include:

  • Ability to resend a challenge solved notification, in case the user closed the notification before copying the key. The re sending is triggered by clicking on the challenge solved notification on the scoreboard.
  • Ability to copy the key via a copy to clipboard button (using the already imported ngclipboard package)
  • Ability to hide the ctf key in the challenge solved notification via a config options as they can be distracting to user not in a ctf environment. I added a ctf config file to demo this

I have also removed the fallback to the config application name to better be able to hide the application name during ctf events to try to prevent cheating a bit better.

Thank you for reading this and for making this awesome application!

@bkimminich
Copy link
Member

Wow, that is awesome! Thanks a lot! I'll merge it into develop and do some testing, maybe a bit of re-wording the descriptions and then it'll be part of v2.26! 👍

@bkimminich
Copy link
Member

bkimminich commented Apr 13, 2017

I think it makes sense to only offer the re-send feature if notifications and CTF-mode are enabled, right? Otherwise it might confuse people. Following @wurstbrot's explicit and self-explaining naming scheme in the config, we could also call the property e.g. showCtfFlagsInNotifications instead of ctfEnabled?

showChallengeSolvedNotifications: true
showCtfFlagsInNotifications: true
# = Show notifications with flags and copy/paste-button.
# Also show repeat-notification feature via scoreboard.
showChallengeSolvedNotifications: true
showCtfFlagsInNotifications: false
# = Show notifications but hide flags.
# Hide repeat-notification feature from scoreboard.
showChallengeSolvedNotifications: false
showCtfFlagsInNotifications: true|false
# = Hide notifications and re-send feature.

Do you maybe want to do those changes before I merge?

@J12934
Copy link
Member Author

J12934 commented Apr 13, 2017

Yeah that makes sense.
I'll look into implementing this this afternoon.

@J12934
Copy link
Member Author

J12934 commented Apr 13, 2017

I added the functionality.
But having the ctf mode disabled by default means it is also disabled in the e2e test which leads to failing the test case i wrote for this feature.

Do you have an idea how to test features which are disabled by default in a nice way?

@bkimminich
Copy link
Member

Hm, I didn't realize CTF would be disabled by default... The corresponding test could enable it before it runs, I guess. Just enabling it by default would also be okay as a workaround.

@bkimminich bkimminich merged commit 72d2cb1 into juice-shop:develop Apr 13, 2017
@bkimminich
Copy link
Member

Fixed the issue with that e2e test (it checks now wether CTF is on or off and uses a correspondig it() definition) and reduced the ctf.yml to only overriding the necessary setting.

Thanks again to @J12934 and @kkottke!!! 🥇

@J12934 J12934 deleted the ctf-improvments branch April 14, 2017 07:25
@J12934 J12934 restored the ctf-improvments branch April 14, 2017 07:25
bkimminich added a commit that referenced this pull request Apr 20, 2017
* v2.26.0-SNAPSHOT

* Make config callback optional

* Show thumbnail in search results

* Replace theme in index.html on server start
(fixes #293)

* Delete __vulns.json
(idea has not seen any adoption)

* Increase file size for upload-size test
(to prevent occasional failure on some machines)

* Add unit tests
- for app.configuration of Twitter/Facebook URLs
- and for presence of cryptocurrency addresses

* Extend client side unit tests

* Add walkthrough on http://location-href.com to web links

* Handle $http promises inside services
as preparation for Angular 1.6 migration (see #294)

* Adopt User API tests to new promise handling

* Fix authentication in User API tests

* Ignore node security advisory 154 for sanitize-html
(https://nodesecurity.io/advisories/154)

* New Crowdin translations (#286)

* Romanian
* Spanish
* Burmese

* Add Burmese language to dropdown :mm:

* Ctf improvments (#295)

* Added clipboard button

* Implemented replay notification of solved challenges.

(cherry picked from commit e72245a)

* Made the repeat notification more obvious.

(cherry picked from commit a62656f)

* Made the copy button text translatable

* Fixing coding style to follow standard guidelines

* Made the check if the challange was solved more obvious

* Added e2e test for the challange repeat functionality

* Added option to show/ hide the ctf flags in the notifications

* Refactored code to follow standard code style

* Fixed unexpected request in client side test

* Diabled challange repeat functionality if ctf is disabled

* Remove unnecessary repetition of config properties

* Make notification-repeat test aware of CTF mode

* Update documentation regarding CTF extensions

* Beautify UI for CTF extension
- smaller copy-button
- moved repeat-info to a  tooltip

* Add Testhexen blog post

* Pull out media links into REFERENCES.md

* Reorder README sections

* Changed Docker Baseimage to node-alpine

* Add `git` to Docker image
(+17MB, required for some Bower dependencies)

* Install `git` before switching to `WORKDIR`

* Run `apk update` before attempting to add `git`

* Fix README/REFERENCES wording and syntax

* New Crowdin translations (#296)

* New translations en.json (Romanian)

* New translations en.json (Romanian)

* New translations en.json (Romanian)

* New translations en.json (Romanian)

* New translations en.json (Spanish)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Romanian)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Burmese)

* New translations en.json (Japanese)

* New translations en.json (Czech)

* New translations en.json (Dutch)

* New translations en.json (Romanian)

* New translations en.json (Hungarian)

* New translations en.json (Danish)

* New translations en.json (Indonesian)

* New translations en.json (Turkish)

* New translations en.json (Spanish)

* New translations en.json (Burmese)

* New translations en.json (French)

* New translations en.json (Norwegian)

* New translations en.json (German)

* New translations en.json (Arabic)

* New translations en.json (Swedish)

* New translations en.json (Portuguese)

* New translations en.json (Klingon)

* New translations en.json (Russian)

* New translations en.json (Italian)

* New translations en.json (Estonian)

* New translations en.json (Greek)

* New translations en.json (Japanese)

* New translations en.json (Lithuanian)

* New translations en.json (Latvian)

* New translations en.json (Finnish)

* New translations en.json (Polish)

* New translations en.json (Chinese Simplified)

* New translations en.json (Czech)

* New translations en.json (Dutch)

* New translations en.json (Romanian)

* New translations en.json (Hungarian)

* New translations en.json (Danish)

* New translations en.json (Indonesian)

* New translations en.json (Burmese)

* New translations en.json (Spanish)

* New translations en.json (Turkish)

* New translations en.json (Norwegian)

* New translations en.json (German)

* New translations en.json (French)

* New translations en.json (Arabic)

* New translations en.json (Swedish)

* New translations en.json (Portuguese)

* New translations en.json (Klingon)

* New translations en.json (Russian)

* New translations en.json (Italian)

* New translations en.json (Estonian)

* New translations en.json (Greek)

* New translations en.json (Japanese)

* New translations en.json (Lithuanian)

* New translations en.json (Latvian)

* New translations en.json (Finnish)

* New translations en.json (Polish)

* New translations en.json (Chinese Simplified)

* Rebase with develop

* New translations en.json (German)

* New translations en.json (German)

* New translations en.json (Turkish)

* Update contributors list

* v2.26.0

* Add language flags for non-english links
@lock
Copy link

lock bot commented Nov 4, 2019

This thread has been automatically locked because it has not had recent activity after it was closed. 🔒 Please open a new issue for regressions or related bugs.

@lock lock bot locked and limited conversation to collaborators Nov 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants