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

Add infrastructure for password guards #121

Merged
merged 14 commits into from
Jul 16, 2021
Merged

Add infrastructure for password guards #121

merged 14 commits into from
Jul 16, 2021

Conversation

Skn0tt
Copy link
Contributor

@Skn0tt Skn0tt commented Jul 12, 2021

This is a chunky PR, let's take a look at individual parts:

  1. LQPasswordGuard contains logic about password hashing. It publishes isValid, which can be used to check if a provided password is valid. There's also LQAcceptingGuard, which acts like a null object.
  2. LiquidPoll now stores a guard instance variable. By default, it's the LQAcceptingGuard null object, which allows any password. Once the poll is started using startWithId:andPassword:..., it's replaced with an actual LQPasswordGuard. The LQPasswordGuard can be safely transferred over the wire, since it contains nothing but a hash.
  3. To relieve users from the unbearable burden of remembering a password, we now have the LQPasswordManager. It generates & stores passwords for polls.

This is a rough first attempt, in that it only implements password protection for Poll >> isOpen, and it uses the cryptographically unsound MD5 hashing algorithm.

This is a big commit, let's take a look at individual parts:
1. LQPasswordGuard contains logic about password hashing. It publishes `isValid`, which can be used to check if a provided password is valid. There's also LQAcceptingGuard, which acts like a null object.
2. LiquidPoll now stores a `guard` instance variable. By default, it's the LQAcceptingGuard null object, which allows *any* password. Once the poll is started using startWithId:andPassword:..., it's replaced with an actual LQPasswordGuard. The LQPasswordGuard can be safely transferred over the wire, since it contains nothing but a hash.
3. To relieve users from the unbearable burden of remembering a password, we now have the LQPasswordManager. It generates & stores passwords for polls.

This is a rough first attempt, in that it only implements password protection for Poll >> isOpen, and it uses the cryptographically unsound MD5 hashing algorithm.

Co-Authored-By: Callista Gratz <[email protected]>
@coveralls
Copy link

coveralls commented Jul 12, 2021

Pull Request Test Coverage Report for Build 1037011211

  • 50 of 56 (89.29%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.8%) to 51.48%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/Liquid-Core.package/LiquidHostMenu.class/instance/closePoll.st 0 6 0.0%
Totals Coverage Status
Change from base Build 1036960272: 1.8%
Covered Lines: 487
Relevant Lines: 946

💛 - Coveralls

Callista Gratz and others added 3 commits July 12, 2021 16:23
@Skn0tt Skn0tt marked this pull request as ready for review July 12, 2021 14:37
@Skn0tt Skn0tt requested review from NikkelM and Dassderdie and removed request for NikkelM July 12, 2021 14:39
Callista Gratz and others added 2 commits July 12, 2021 16:41
co-authored-by: Simon Knott <[email protected]>
co-authored-by: Simon Knott <[email protected]>
Copy link
Contributor

@NikkelM NikkelM left a comment

Choose a reason for hiding this comment

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

I have quite a lot of comments concerning this PR, two of which I classified as [boulders], concerning an error occurring during runtime and the handling of the new passwords in our tests. Most of it is however only of cosmetic nature.
Edit: I also finally found the place where "request changes" comes from. Now that I saw that it is connected to the review as a whole and not to a specific comment, I better understand what it actually does

@Dassderdie
Copy link
Contributor

The current solution has some drawbacks:

  • squeak internal messages (?perform?) can modify the poll without a password
  • we always have to remember where we need password authentication and add this code to the poll

My proposal would be some sort of security detached from the poll class, so that one doesn't have to care about security stuff so much when working in the poll class.
For example a MessageGuard that has a whitelist of messages he allows without authentication and for everything else you have to send a password with it. All the guard etc. stuff would then go out of the Poll class.
There could also be a scenario where a method without password requirement calls one with password requirement. It would be the best if the MessageGuard could guard against such scenarios too.

But this is out of scope for this semester I guess...

@Skn0tt
Copy link
Contributor Author

Skn0tt commented Jul 15, 2021

A whitelist sure sounds like a good idea. I opened a tracking issue for this: #124

@Skn0tt Skn0tt merged commit fc924eb into main Jul 16, 2021
@NikkelM NikkelM deleted the password-guards branch July 16, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants