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

Jump to bottom

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

wants to merge 1 commit into from

Conversation

adombeck
Copy link
Contributor

Session mode PASSWD is ambiguous, because authentication includes password authentication. CHANGE_PASSWORD should be clearer.

This also changes session mode AUTH to AUTHENTICATE so that we use a verb (which describes the purpose of the session) for both session modes.

@adombeck adombeck marked this pull request as ready for review January 17, 2025 16:32
@adombeck adombeck requested a review from a team as a code owner January 17, 2025 16:32
3v1n0
3v1n0 reviewed Jan 17, 2025
View reviewed changes
Copy link
Collaborator

@3v1n0 3v1n0 left a comment

Choose a reason for hiding this comment

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

I agree, that's nicer and easier to understand.

Just one note, as I think we can also change the runner arguments.

internal/brokers/auth/consts.go
// SessionModeAuthenticate is used when the session is for user authentication.
SessionModeAuthenticate = "auth"
// SessionModeChangePassword is used when the session is for changing the user password.
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
Member

Choose a reason for hiding this comment

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

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.

Sorry if that was unclear, but was I was shortly suggesting. Doing a transition with brokers supporting new and old syntax until people migrate.

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?

3v1n0
3v1n0 reviewed Jan 17, 2025
View reviewed changes
internal/brokers/auth/consts.go Outdated
// SessionModePasswd is the name of the passwd session.
SessionModePasswd = "passwd"
// SessionModeAuthenticate is used when the session is for user authentication.
SessionModeAuthenticate = "auth"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe for the same spirit, this should be SessionModeLogin? Since it does both authentication and session initialization...

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 love it! Done!

@adombeck adombeck force-pushed the more-descriptive-session-modes branch from f213b36 to caa3d4d Compare January 20, 2025 11:09
@adombeck adombeck requested a review from 3v1n0 January 21, 2025 20:44
@adombeck
Use more descriptive session mode names
d42e0a2 
* PASSWD -> CHANGE_PASSWORD: Session mode PASSWD is ambiguous, because
  authentication includes password authentication. CHANGE_PASSWORD
  should be clearer.
* AUTH -> LOGIN: We also authenticate the user when the session mode is
  CHANGE_PASSWORD. LOGIN makes it clear that the session is for logging
  the user in.
@adombeck adombeck force-pushed the more-descriptive-session-modes branch from caa3d4d to d42e0a2 Compare January 21, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didrocks didrocks didrocks left review comments

@3v1n0 3v1n0 Awaiting requested review from 3v1n0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adombeck @3v1n0 @didrocks