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

Rework user management #1220

Merged
merged 11 commits into from
May 26, 2022
Merged

Conversation

svartkanin
Copy link
Collaborator

@svartkanin svartkanin commented May 20, 2022

Re-worked user management

Fixes

Changes

New file format for user credentials

The format for the credentials config file has changed to allow specifying multiple users. This change is backwards compatible, so if old configurations get passed they are parseable and will internally be transformed into the new format. When saving configurations they will be saved in the new format.

Old format

{
	"!root-password": "<root password>",
	"!users": {
		"username": {"!password": "<user password>"}
	},
	"!superusers": {
		"admin": {"!password": "<admin password>"}
	}
}

New format

{
	"!root-password": "<root password>",
	"!users": [
		{
			"username": "<USERNAME>",
			"!password": "<PASSWORD>",
			"sudo": false
		},
		{
			"username": "<SUDO_USERNAME>",
			"!password": "<PASSWORD>",
			"sudo": true
		}
	]
}

User object

!superuser does not exist anymore, instead only the internal !users is handled whereas the entries are now a User class object

@dataclass
class User:
	username: str
	password: str
	sudo: bool

This greatly simplified the code in some parts and makes it easier to avoid bugs :)

Update to menu

The menu has now only 1 entry to define users in which the sudo privileges can be specified.

image

@Torxed I'll let you know when it's ready, probably tomorrow :)

@svartkanin svartkanin requested a review from Torxed as a code owner May 20, 2022 12:35
@dylanmtaylor
Copy link
Contributor

Can you also update the docs and schema for this change?

@svartkanin
Copy link
Collaborator Author

Yep will do that too :)

@Torxed
Copy link
Member

Torxed commented May 20, 2022

I don't know why i did'nt do this right away haha, way more clean! Thank you!

@dylanmtaylor
Copy link
Contributor

May I suggest we don't merge this until 2.5.0 since it's a schema-breaking change?

@svartkanin
Copy link
Collaborator Author

It's not a breaking change since it's backwards compatible :)

I think the current state is way more fragile since we only support a single user in a file. Which means - and has been pointed out in the issues - that users will lose their actual settings if they try to export them.

In addition I think the 1 different settings of "users" and "superusers" seemed to have stirred quite some confusion which this resolves as well

@Torxed
Copy link
Member

Torxed commented May 21, 2022

Yea it appears backwards compatible, I get that there's still some work left.
But I'd be happy to run tests on this on the backwards compatibility side and if they're successful I'd be happy to merge this in time for the release :)

@dylanmtaylor
Copy link
Contributor

Yea it appears backwards compatible, I get that there's still some work left. But I'd be happy to run tests on this on the backwards compatibility side and if they're successful I'd be happy to merge this in time for the release :)

I like this plan. I was under the impression that this was a breaking change

@svartkanin
Copy link
Collaborator Author

@Torxed This should be ready for a review now :)

@Torxed
Copy link
Member

Torxed commented May 26, 2022

@Torxed This should be ready for a review now :)

Testing commences now! :D

@Torxed
Copy link
Member

Torxed commented May 26, 2022

It loads the old format where I created a !superuser and a !user.
screenshot

It also installs with the old configuration:
screenshot

And no shocker i guess, but the permissions are set correctly to:
screenshot

@Torxed Torxed merged commit 870da40 into archlinux:master May 26, 2022
@Torxed
Copy link
Member

Torxed commented May 26, 2022

Great work, especially making it backwards compatible. Merged and done! :)

@svartkanin
Copy link
Collaborator Author

Fantastic, thanks for double checking everything :)

@svartkanin
Copy link
Collaborator Author

I think the related issues where not closed automatically

@Torxed
Copy link
Member

Torxed commented May 27, 2022

You are correct heh, gotta love Github.

@svartkanin
Copy link
Collaborator Author

I'm not sure what triggers it, maybe only mentioning it in the title 🤷‍♂️

@Torxed
Copy link
Member

Torxed commented May 27, 2022

It's if the word fix is in the same sentence as the issue link I think.
There are some other aliases for it, but it has to be in the same sentence i recon.
But I don't mind closing the manually too hehe, gives me a sense of purpose 👼

@erikdubois
Copy link

Could you add a text when we can start testing it?

@svartkanin
Copy link
Collaborator Author

It is in master now so basically from there now :), @Torxed will this make it into the new iso?

@Torxed
Copy link
Member

Torxed commented May 27, 2022

It is in master now so basically from there now :), @Torxed will this make it into the new iso?

It will :) I'm planning on running several tests throughout the day.
If they all succeed, I'm aiming to release the next version later this evening or very early tomorrow :)

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.

None yet

4 participants