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

pam/gdm/extension: Add PAM gdm JSON extension protocol implementation #125

Merged
merged 3 commits into from
Dec 11, 2023

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Dec 1, 2023

Add go bindings to encode and decode PAM binary messages using the GDM
Custom JSON protocol.

The protocol will be available as part of GNOME 46, but for being early
consumers we include in the repo the headers where the messages are
defined, until that's possible to switch on the repo version.

See: https://gitlab.gnome.org/GNOME/gdm/-/merge_requests/227

@3v1n0 3v1n0 requested a review from a team as a code owner December 1, 2023 05:46
@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (bf88ae2) 88.14% compared to head (5b16b2a) 88.62%.
Report is 31 commits behind head on main.

Files Patch % Lines
pam/gdm/conversation.go 81.25% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
+ Coverage   88.14%   88.62%   +0.47%     
==========================================
  Files          32       34       +2     
  Lines        2430     2532     +102     
==========================================
+ Hits         2142     2244     +102     
  Misses        221      221              
  Partials       67       67              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Some nitpicks here and there, but it's looking very good. Well done!

pam/gdm/extension_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
@3v1n0 3v1n0 requested a review from denisonbarbosa December 1, 2023 16:08
@3v1n0 3v1n0 force-pushed the gdm-binary-protocol branch from f1ccd43 to b14c161 Compare December 2, 2023 03:12
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Almost there!

pam/gdm/conversation_test.go Outdated Show resolved Hide resolved
pam/gdm/conversation_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
@3v1n0 3v1n0 requested a review from denisonbarbosa December 4, 2023 20:50
@3v1n0 3v1n0 force-pushed the gdm-binary-protocol branch from b4088b3 to 9f805d7 Compare December 5, 2023 20:54
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

This looks good to me now. Well done!

Copy link
Contributor

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

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

Great work here! Had a couple minor test related comments

pam/gdm/conversation_test.go Show resolved Hide resolved
pam/gdm/conversation_test.go Outdated Show resolved Hide resolved
pam/gdm/extension_test.go Outdated Show resolved Hide resolved
@3v1n0 3v1n0 requested a review from GabrielNagy December 11, 2023 17:39
Copy link
Contributor

@GabrielNagy GabrielNagy left a comment

Choose a reason for hiding this comment

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

Great!

Add go bindings to encode and decode PAM binary messages using the GDM
Custom JSON protocol.

The protocol will be available as part of GNOME 46, but for being early
consumers we include in the repo the headers where the messages are
defined, until that's possible to switch on the repo version.

See: https://gitlab.gnome.org/GNOME/gdm/-/merge_requests/227
…nary protocol

Add an utility function to perform GDM-PAM binary conversations using
the custom JSON protocol.

This function does not much per se, but it allows us to do some proper
testing via the dummy module transaction and so we can test how this
data is passed back and forth the two sides, and stress the C <-> go
structures conversions.
@3v1n0 3v1n0 force-pushed the gdm-binary-protocol branch from 5b16b2a to ae93edc Compare December 11, 2023 21:15
@GabrielNagy GabrielNagy merged commit babdf7b into ubuntu:main Dec 11, 2023
4 checks passed
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