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.

Sign up for GitHub

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

Use more descriptive session mode names #738

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use more descriptive session mode names #738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examplebroker/broker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ func (b *Broker) NewSession(ctx context.Context, username, lang, mode string) (s
return "", "", fmt.Errorf("user %q does not exist", username)
}

if info.sessionMode == auth.SessionModePasswd {
if info.sessionMode == auth.SessionModeChangePassword {
info.neededAuthSteps++
info.pwdChange = mustReset
}
Expand Down
10 changes: 6 additions & 4 deletions internal/brokers/auth/consts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ const (
var Replies = []string{Granted, Denied, Cancelled, Retry, Next}

const (
// SessionModeAuth is the name of the authentication session.
SessionModeAuth = "auth"
// SessionModePasswd is the name of the passwd session.
SessionModePasswd = "passwd"
// SessionModeLogin is used when the session is for user login.
// TODO: We can change this to "login" once all broker installations are updated to use the new name.
SessionModeLogin = "auth"
// SessionModeChangePassword is used when the session is for changing the user password.
// TODO: We can change this to "change-password" once all broker installations are updated to use the new name.
SessionModeChangePassword = "passwd"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think at this point we should also rename this into "change-password", being the command line of the runner (and please also update pam/Hacking.md accordingly.

It may require regenerate some golden files I think... But that's a minor thing (do this change in a following commit, in case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think at this point we should also rename this into "change-password"

I would love to do that, but this string is part of the API between authd and the broker, so it would be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can take the risk and have the brokers for now have a newName || "passwd", wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, that's an option, but for what I meant, was the one in use by the runner... Adding an extra const in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can take the risk and have the brokers for now have a newName || "passwd", wdyt?

I don't see how we can do that. The session mode is passed from authd to the broker in the NewSession call. Current broker versions don't know about a "change-password" session mode, so they will break if they receive a call from authd which uses that.

What we could do is to keep using "passwd" in authd and start supporting both "change-password" and "passwd" in the next broker release. Then once all broker installations have been updated, we can start using "change-password" in authd in the following release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented in ubuntu/authd-oidc-brokers#324

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not particularly in love on changing what's on the wire... Also because it would likely imply changes to gdm too, making the transition harder (we'd need to cross dependencies and so on...).

So, I wouldn't really change what's the string value or the proto ID for it, while using better naming anyways (you can still add another go file to the proto folder that does CHANGE_PASSWORD = PASSWD.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also because it would likely imply changes to gdm too

Oh, where are the session mode names being used in GDM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And is there currently no dependency between authd and gdm? What's the plan when we really need to make changes to the protocol?

Copy link
Collaborator

@3v1n0 3v1n0 Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, where are the session mode names being used in GDM?

Oh, no sorry... It's not used there I got confused by the layout modes...

Still, it doesn't change the fact that I'd prefer not to change the .proto file, if we want to be reliable (it's also an exercise for the future) we can add things but not change them in the run, until a new version is defined.

And is there currently no dependency between authd and gdm?

It can't be, because both can work without the other side. We only can suggest, but nothing else...

What's the plan when we really need to make changes to the protocol?

One thing we could do is making authd Breaks gnome-shell versions prior to X... However my idea when designing it, since the gdm protocol is also versioned, to change the version in case we need to and until we can support both versions.

)
2 changes: 1 addition & 1 deletion internal/brokers/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ func TestNewSession(t *testing.T) {
wantErr bool
}{
"Successfully start a new auth session": {username: "success"},
"Successfully start a new passwd session": {username: "success", sessionMode: auth.SessionModePasswd},
"Successfully start a new passwd session": {username: "success", sessionMode: auth.SessionModeChangePassword},
"Successfully start a new session with the correct broker": {username: "success", configuredBrokers: []string{t.Name() + "_Broker1.conf", t.Name() + "_Broker2.conf"}},

"Error when broker does not exist": {brokerID: "does_not_exist", wantErr: true},
Expand Down
156 changes: 78 additions & 78 deletions internal/proto/authd/authd.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions internal/proto/authd/authd.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ message StringResponse {

enum SessionMode {
UNDEFINED = 0;
AUTH = 1;
PASSWD = 2;
LOGIN = 1;
CHANGE_PASSWORD = 2;
}

message SBRequest {
Expand Down
8 changes: 4 additions & 4 deletions internal/services/pam/pam.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ func (s Service) SelectBroker(ctx context.Context, req *authd.SBRequest) (resp *

var mode string
switch req.GetMode() {
case authd.SessionMode_AUTH:
mode = auth.SessionModeAuth
case authd.SessionMode_PASSWD:
mode = auth.SessionModePasswd
case authd.SessionMode_LOGIN:
mode = auth.SessionModeLogin
case authd.SessionMode_CHANGE_PASSWORD:
mode = auth.SessionModeChangePassword
default:
return nil, status.Error(codes.InvalidArgument, "invalid session mode")
}
Expand Down
16 changes: 8 additions & 8 deletions internal/services/pam/pam_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ func TestSelectBroker(t *testing.T) {

wantErr bool
}{
"Successfully select a broker and creates auth session": {username: "success", sessionMode: auth.SessionModeAuth},
"Successfully select a broker and creates passwd session": {username: "success", sessionMode: auth.SessionModePasswd},
"Successfully select a broker and creates auth session": {username: "success", sessionMode: auth.SessionModeLogin},
"Successfully select a broker and creates passwd session": {username: "success", sessionMode: auth.SessionModeChangePassword},

"Error when not root": {username: "success", currentUserNotRoot: true, wantErr: true},
"Error when username is empty": {wantErr: true},
Expand Down Expand Up @@ -229,10 +229,10 @@ func TestSelectBroker(t *testing.T) {

var sessionMode authd.SessionMode
switch tc.sessionMode {
case auth.SessionModeAuth, "":
sessionMode = authd.SessionMode_AUTH
case auth.SessionModePasswd:
sessionMode = authd.SessionMode_PASSWD
case auth.SessionModeLogin, "":
sessionMode = authd.SessionMode_LOGIN
case auth.SessionModeChangePassword:
sessionMode = authd.SessionMode_CHANGE_PASSWORD
case "-":
sessionMode = authd.SessionMode_UNDEFINED
}
Expand Down Expand Up @@ -571,7 +571,7 @@ func TestIDGeneration(t *testing.T) {
sbResp, err := client.SelectBroker(context.Background(), &authd.SBRequest{
BrokerId: mockBrokerGeneratedID,
Username: usernamePrefix + testutils.IDSeparator + tc.username,
Mode: authd.SessionMode_AUTH,
Mode: authd.SessionMode_LOGIN,
})
require.NoError(t, err, "Setup: failed to create session for tests")

Expand Down Expand Up @@ -812,7 +812,7 @@ func startSession(t *testing.T, client authd.PAMClient, username string) string
sbResp, err := client.SelectBroker(context.Background(), &authd.SBRequest{
BrokerId: mockBrokerGeneratedID,
Username: username,
Mode: authd.SessionMode_AUTH,
Mode: authd.SessionMode_LOGIN,
})
require.NoError(t, err, "Setup: failed to create session for tests")
return sbResp.GetSessionId()
Expand Down
4 changes: 2 additions & 2 deletions pam/integration-tests/cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func TestCLIAuthenticate(t *testing.T) {

localgroupstestutils.RequireGPasswdOutput(t, gpasswdOutput, golden.Path(t)+".gpasswd_out")

requireRunnerResultForUser(t, authd.SessionMode_AUTH, tc.clientOptions.PamUser, got)
requireRunnerResultForUser(t, authd.SessionMode_LOGIN, tc.clientOptions.PamUser, got)
})
}
}
Expand Down Expand Up @@ -306,7 +306,7 @@ func TestCLIChangeAuthTok(t *testing.T) {
got := td.ExpectedOutput(t, outDir)
golden.CheckOrUpdate(t, got)

requireRunnerResult(t, authd.SessionMode_PASSWD, got)
requireRunnerResult(t, authd.SessionMode_CHANGE_PASSWORD, got)
})
}
}
Expand Down
4 changes: 2 additions & 2 deletions pam/integration-tests/native_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ func TestNativeAuthenticate(t *testing.T) {
localgroupstestutils.RequireGPasswdOutput(t, gpasswdOutput, golden.Path(t)+".gpasswd_out")

if !tc.skipRunnerCheck {
requireRunnerResultForUser(t, authd.SessionMode_AUTH, tc.clientOptions.PamUser, got)
requireRunnerResultForUser(t, authd.SessionMode_LOGIN, tc.clientOptions.PamUser, got)
}
})
}
Expand Down Expand Up @@ -435,7 +435,7 @@ func TestNativeChangeAuthTok(t *testing.T) {
golden.CheckOrUpdate(t, got)

if !tc.skipRunnerCheck {
requireRunnerResult(t, authd.SessionMode_PASSWD, got)
requireRunnerResult(t, authd.SessionMode_CHANGE_PASSWORD, got)
}
})
}
Expand Down
Loading
Loading