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

Stop revoking privileges #15

Closed
avdata99 opened this issue Jan 14, 2021 · 6 comments · Fixed by #16
Closed

Stop revoking privileges #15

avdata99 opened this issue Jan 14, 2021 · 6 comments · Fixed by #16

Comments

@avdata99
Copy link
Contributor

avdata99 commented Jan 14, 2021

If you manually promote a user as sysadmin this extension will revoke these privileges in the next login

User's email must be explicitly defined in the ckanext.saml2auth.sysadmins_list list
This may not be intuitive and could be interpreted as a bug.

Proposed solution

If ckanext.saml2auth.sysadmins_list is empty or not defined then we don't change any privileges at login time.

@adborden
Copy link

Some background:

For security compliance, we want to be able to manage user privileges in the database. At any point in time, we should be able to query CKAN for users and their authorization level. Having sysadmins in config, but organization admins in the database is confusing to auditors.

Also having the sysadmins in config means our audit log for sysadmin changes is different than the audit log for other privileged users.

@mbocevski
Copy link
Member

This may not be intuitive and could be interpreted as a bug.

I agree, also in scenarios where local login is also enabled this is not ideal. I think that the approach should be if not set then we don't change any privilege at login time. If it's empty or has a list of email addresses then it should change privileges, empty being a valid choice i.e. no sysadmins.

@mbocevski
Copy link
Member

@avdata99 @adborden you ok with #16 ?

@avdata99
Copy link
Contributor Author

avdata99 commented Jan 14, 2021

I agree with #16. it's simple and clear (but too complex to flak8 :) )
I can backport this to 2.8 @adborden if you agree.

@mbocevski
Copy link
Member

but too complex to flak8

Yeah, that code does quite some. I'll move it to the helper instead.

@adborden
Copy link

Thanks, looks good.

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 a pull request may close this issue.

3 participants