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

Admin Logs - Fixed Log Settings errors #4399

Merged
merged 5 commits into from
Jan 12, 2021

Conversation

daguiler
Copy link
Contributor

@daguiler daguiler commented Jan 5, 2021

Fixes #4398

Summary

This pull request reverts 4d1f352 and changes the Admin Logs panel to show error notifications when Log Settings operations fail (instead of success notifications)

@daguiler
Copy link
Contributor Author

daguiler commented Jan 5, 2021

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 4399 in repo dnnsoftware/Dnn.Platform

@daguiler
Copy link
Contributor Author

daguiler commented Jan 5, 2021

Hi @bdukes can you help me rebuild this PR?
I believe the build error is not caused by my changes

@daguiler daguiler marked this pull request as ready for review January 5, 2021 20:14
@bdukes
Copy link
Contributor

bdukes commented Jan 6, 2021

@dnnsoftware/approvers do you need to be a superuser to manage log settings? It looks like there's already logic so that a non-superuser can only manage settings for their current portal. Anyone have a reason why this restriction would need to stay in place?

@sleupold
Copy link
Contributor

sleupold commented Jan 6, 2021

site specific settings should be able to be managed by portal admins (and superusers), however, all system events (relevent for all sites) should be managed by superusers only.

@daguiler
Copy link
Contributor Author

daguiler commented Jan 6, 2021

Actually, the Log Settings tab is not even displayed if you are not a super user.

@mitchelsellers
Copy link
Contributor

I would agree with @sleupold on this one, and I am not 100% sure that we have the proper distinction in being able to do this within the configuration settings.

For example, an Admin should not be able to turn off logging of user logins etc.

@daguiler
Copy link
Contributor Author

daguiler commented Jan 6, 2021

The issue that was reported to us is that admins/superusers are not allowed to add log settings. And given that this issue has been in place for more than 3 years, I think it's fair to assume that nobody is using this feature at all.
So my suggestion is to just remove this restriction (current changes in this PR) and then create a new issue describing the optimal behavior of this part of DNN.
Otherwise this will become a design change, and it would go out of scope of my bugfix.

@mitchelsellers
Copy link
Contributor

I don't think we can just remove the restriction, as that changes the security profile quite a bit.. Let me chat with the others about this one.

@valadas
Copy link
Contributor

valadas commented Jan 6, 2021

I agree that this should be reserved to hosts only, admins should view the logs but not be able to add log types for instance...

@daguiler
Copy link
Contributor Author

daguiler commented Jan 6, 2021

Ok, I agree it would have changed the security profile.
I now pushed 2 commits to fix the issue for superusers without impacting security in any other way.

Co-authored-by: Brian Dukes <bdukes@engagesoftware.com>
@daguiler
Copy link
Contributor Author

daguiler commented Jan 6, 2021

Thanks @bdukes
It seems Github didn't honor the encoding and/or the new line characters, so it looks like a huge change now. But if you enable the Hide whitespace changes checkbox, it will look fine.

@bdukes bdukes added this to the 9.9.0 milestone Jan 6, 2021
Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

Thanks!

@donker donker merged commit 8543fbd into dnnsoftware:develop Jan 12, 2021
@daguiler daguiler deleted the bugfix/DNN-46166 branch January 12, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin Logs - Add Log Setting returns 401 for admins
6 participants