Skip to content

snap: auto-import will not try to auto-create users on managed devices#9293

Closed
mvo5 wants to merge 1 commit intocanonical:masterfrom
mvo5:no-auto-add-users-for-already-managed
Closed

snap: auto-import will not try to auto-create users on managed devices#9293
mvo5 wants to merge 1 commit intocanonical:masterfrom
mvo5:no-auto-add-users-for-already-managed

Conversation

@mvo5
Copy link
Copy Markdown
Contributor

@mvo5 mvo5 commented Sep 8, 2020

The snap auto-import code right will always try to create all
known system-users when it imports any assertions. However this
leads to systemd errors and a degraded boot when a device is already
managed and a removable device with a user assertion is attached
to the device.

This commit changes the auto-import code to only try to create
known users if the device is unmanaged. It will still import
assertions thought.

This fixes https://bugs.launchpad.net/newparis/+bug/1893331

The `snap auto-import` code right will always try to create all
known system-users when it imports any assertions. However this
leads to systemd errors and a degraded boot when a device is already
managed and a removable device with a user assertion is attached
to the device.

This commit changes the auto-import code to only try to create
known users if the device is unmanaged. It will still import
assertions thought.

This fixes https://bugs.launchpad.net/newparis/+bug/1893331
Copy link
Copy Markdown
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@anonymouse64 anonymouse64 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this

@mvo5 mvo5 added this to the 2.47 milestone Sep 14, 2020
@mvo5 mvo5 added the Needs Samuele review Needs a review from Samuele before it can land label Sep 15, 2020
}

// only try to create users for unmanaged devices
isManaged, err := x.isManaged()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My preference would be to make this a problem of the daemon, by having a new flag option "automatic" in the request. It would also replace for this use case the other current flags we set, that would make it easier to support later as we discussed recently having system-user users that are not sudoers. With the current approach the decision is done here which would be the wrong place then.

@mvo5
Copy link
Copy Markdown
Contributor Author

mvo5 commented Oct 13, 2020

Closing in favor of #9498

@mvo5 mvo5 closed this Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Samuele review Needs a review from Samuele before it can land

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants