pwncollege / dojo

Infrastructure powering the pwn.college dojo
https://pwn.college
BSD 2-Clause "Simplified" License
301 stars 100 forks source link

Support Module level visibility #545

Closed robwaz closed 2 months ago

robwaz commented 2 months ago

Adds support for module level visibility configuration.

ConnorNelson commented 2 months ago

Otherwise, it looks correct to me (I haven't manually tested).

This makes me think I was unnecessarily complex with these Visibility tables, and they should honestly just be columns, but I appreciate you matching the style! Probably (hopefully) some day this will be refactored into columns.

ConnorNelson commented 2 months ago

Actually, edge case consideration!

What happens to the challenge when you define a module start, and a challenge start? Does the challenge take the max(), min(), or its own-specified value. If own-specified value, does this mean we can start a challenge that isn't visually available; should that be possible?

robwaz commented 2 months ago

To the point of the "what if". I haven't tested it, but applies visibility via the existing visibility functionality which takes a series checks in order: dojo, course, challenge and applies the widest visibility found.

This means it is possible to still support the use-case of a visible dojo with some challenges (or resources) hidden. It is not possible to create a hidden dojo with challenges visible/accessible. (Aside from challenges that are imported, but that's unrelated to this PR.)

https://github.com/pwncollege/dojo/blob/7ae88a6f9301bcf6ce205c2b9e598844e47e0f5c/dojo_plugin/utils/dojo.py#L257 https://github.com/pwncollege/dojo/blob/7ae88a6f9301bcf6ce205c2b9e598844e47e0f5c/dojo_plugin/utils/dojo.py#L293

robwaz commented 2 months ago

The thing I do no know, is why this particular CI failure is happening. It's failing to run practice mode in CI, but my local instance doesn't appear to have any issues and the error did not jump out at me reviewing the CI integration test logs.

robwaz commented 2 months ago

Otherwise, it looks correct to me (I haven't manually tested).

This makes me think I was unnecessarily complex with these Visibility tables, and they should honestly just be columns, but I appreciate you matching the style! Probably (hopefully) some day this will be refactored into columns.

I don't think that was the case. This dynamic cascading property setup is more clear to me than if each concept (dojo, module, challenge) had their own column. Suddenly the logic quite a bit messier with more edge-cases like challenges visible (API accessible I imagine) inside modules or dojos that aren't. It's a lot of state to manage for no real win.

ConnorNelson commented 2 months ago

https://github.com/pwncollege/dojo/issues/546

I think this is likely not caused by your changes...

robwaz commented 2 months ago

Let's find out if this works