-
Notifications
You must be signed in to change notification settings - Fork 8
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
Pam random cleanups and fixes #155
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #155 +/- ##
==========================================
- Coverage 82.89% 82.56% -0.34%
==========================================
Files 58 58
Lines 4800 4828 +28
==========================================
+ Hits 3979 3986 +7
- Misses 633 649 +16
- Partials 188 193 +5 ☔ View full report in Codecov by Sentry. |
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.
Thanks for the changes! Overall, they are fixing and bringing needed fixes to the project. I have few comments though on keeping the code idiomatic and how we avoid introducing the lifecycle in the proto files.
833b2ca
to
653aa96
Compare
a03d1a9
to
8396e82
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.
Nice work, all makes sense to me :) the atomic commits make for an easier review
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.
Looks good to me! Just a minor comment about a missing comment, but it's easily addressable.
03a6887
to
2d22aef
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.
Nice work!
I'll wait merging this until #173 decision on package name is finalized. |
a11594b
to
572f2a0
Compare
Ok rebased on main. @didrocks this is ready too from my POV, so feel free to merge if you're fine with it too. |
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.
Here we go. I’m surprised by the changes in golden files which make me think that the diff in the previous PR wasn’t really looked at. I suggest a double look at the "weird cases" I am currently spotting now.
Also, ensuring that we don’t stutter the error to the user is important. That could be only the CLI text rendering, but please double check those.
Then, the rest are minor small changes.
pam/integration-tests/testdata/TestCLIIntegration/golden/authenticate_user_switching_broker
Outdated
Show resolved
Hide resolved
pam/integration-tests/testdata/TestCLIIntegration/golden/authenticate_user_with_mfa
Outdated
Show resolved
Hide resolved
...gration-tests/testdata/TestCLIIntegration/golden/deny_authentication_if_max_attempts_reached
Outdated
Show resolved
Hide resolved
pam/integration-tests/testdata/TestCLIIntegration/golden/exit_authd_if_user_sigints
Outdated
Show resolved
Hide resolved
I've also added an extra change to introduce
|
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.
Answered the comments + a new one in the newly introduced code.
0c45d61
to
4d248c8
Compare
…om the UI This is not happening right now since our UI implements some modes, but it may happen that a remote implementation (gdm) may not.
…ilable We should not ignore the fact that no authentication modes are available so return a proper error instead of ignoring it.
This was supposed to be a logic or, not a bitwise one. As per this, update the golden files to reflect the expected output.
Mimic the behavior that we have with authentication
400ff17
to
d7140e0
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.
Ok, there is the initial state that should be working and not unknown that is remaining to be fixed as discussed.
The rest of the changes looks good. Thanks for making the golden files way easier to read!
In some implementations (e.g. gdm) we need to communicate with the UI the stage that is expected to be shown. Since we have the same already defined in the PAM module for local implementation, we can just expose the same values in a newly defined pam protocol so that can be reused by gdm too. As per this, add a Stage enum type to the protocol and reuse these values inside the pam module. We just need to also define an undefined state to have an invalid value.
Avoid repeating the same operations multiple times or handling change stage messages when not needed. To make this to work we need to ensure that the initial state is not marked as user selection. We do not define a new stage value since it would be unused, and that's good that is not.
This makes the output of the golden files clearer and it makes explicit the Pam status code that is returned.
Print it once authenticated so that we are sure that the expected user is passed to the pam stack.
Our model may handle multiple pam client types, and depending on them it could behave differently, so indicate this through an enum instead of a simple boolean, since we will have multiple types not just interactive and non interactive clients. As for now, just return an error if a non-interactive client is used, since that's the only thing we support right now.
If an error occurred we need to also return the model, otherwise we'd end up on a invalid memory address or nil pointer dereference in bubbletea
d7140e0
to
b4feed5
Compare
Thanks @didrocks, rebased and squashed, so feel free to merge now. |
Some cleanups and fixes as prerequisite of bigger gdm changes