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

History Tab Overhaul & Domain Record Modifications Changelog #1042

Merged
merged 23 commits into from
Nov 30, 2021

Conversation

kkmanos
Copy link
Contributor

@kkmanos kkmanos commented Nov 10, 2021

This PR is a continuation of the raised issue #1025.

We have created a wiki page providing full documentation on the proposed feature.

This PR also addresses issue #872, by adding a zone-specific changelog.

Note that the Domain Changelog does not contain Reverse records that were created automatically with the auto_ptr setting enabled, because they do not produce a history log, as stated in issue #1041.

@lgtm-com
Copy link

lgtm-com bot commented Nov 10, 2021

This pull request introduces 25 alerts when merging e05b923 into bfaf565 - view on LGTM.com

new alerts:

  • 17 for Unused local variable
  • 6 for Testing equality to None
  • 1 for Unused import
  • 1 for Module is imported with 'import' and 'import from'

@RGanor
Copy link
Contributor

RGanor commented Nov 11, 2021

Amazing job you guys did here with the history overhaul!
Because of the amount of changes you introduce I would like the hear from the community their thought of these changes to make sure it compatible with their setup and needs.

I will test feature in the coming weekend but until then I got few points regarding this.

  1. Look into lgtm alerts and fix them if possible
  2. I saw that you added jqurey-ui in base.html. please add this in offline mode as well (in order to work without internet access)
  3. Why you changed the Line in custom.js and commented out javascript function in user_profile.html ?

Thanks for you contribution!

@vmarkop
Copy link
Contributor

vmarkop commented Nov 11, 2021

@RGanor Thank you for your feedback!

After following your pointers we looked into and resolved the LGTM alerts raised, whilst also adding the jquery-ui files locally, for use in offline mode.

Now, as far as the third point is concerned:

  • custom.js: We added an auto reload when applying the changes for record modifications, because the record-changelog isn't up-to-date when the modification is completed otherwise.
  • user_profile.html: After including jquery-ui for the calendar datepicker that is used by the history search filters, we noticed that some existing tabs in the app inherited a gray background element. We found that removing the "url anchor tags" function reverses this unwanted effect, while retaining the tabs' original functionality, which we ensured after rigorous testing.

For any further observations/clarifications, please feel free to contact us at vmarkop@gunet.gr

@vmarkop
Copy link
Contributor

vmarkop commented Nov 15, 2021

@RGanor A slight mishap occured and we linked you the wrong email address. Please contact us at barbarousisk@gunet.gr instead in case you need anything. We apologise if any inconvenience was caused.

@RGanor RGanor merged commit 1332c8d into PowerDNS-Admin:master Nov 30, 2021
@ghost
Copy link

ghost commented Nov 30, 2021

Hi @kkmanos,

thank you guys for this great work! I've yet to try out the advanced history feature, it'll come in very handy for sure.

One question on fb01274, why has powerdnsadmin/static/assets/jquery-ui-smooth-datepicker/ been committed - shouldn't that be generated via $ flask assets build?

@kkmanos kkmanos mentioned this pull request Dec 1, 2021
@kkmanos
Copy link
Contributor Author

kkmanos commented Dec 1, 2021

Hello @zoeller-freinet ,

We removed jquery-ui smoothness library for the datepicker. Bootstrap datepicker will now be used for the datepicker functionality. This change has been commited in a new PR #1059.

Thanks for the feedback.

@kkmanos kkmanos deleted the account-search-for-user branch December 20, 2021 08:24
RGanor added a commit to RGanor/PowerDNS-Admin that referenced this pull request Dec 25, 2021
* remove otp token from login page, depending on Setting

* show traceback

* handle decode error, output warning

* remove traceback

* added new Dockerfile, to support more than one process running in docker, using s6 overlay

* add new dockerfile with s6 overlay and multiple proccesses to have background jobs updating accounts and zones

* add setting to hide otp_token field on login page

* add environment to cron

* add 'custom_css' setting to model; check for 'custom_css' in template; create custom css dir in dockerfile

* add rule for 'custom_css' setting

* fix include

* remove unrelated files and changes as best as possible

* remove unrelated files and changes as best as possible

* add css to base as well

* fix some jinja typos

* Bug domain parse (PowerDNS-Admin#936)

* Fix when no records returned by API (PowerDNS-Admin#923)

For some reason when some programs delete a record we get an entry returned with records: []

* Remove otp field (PowerDNS-Admin#942)

* fix for api key (PowerDNS-Admin#950)

* allow users to remove domain (PowerDNS-Admin#952)

* Replace [ZONE] placeholder with domain_name (PowerDNS-Admin#960)

* bg_domain button for operators and higher (PowerDNS-Admin#993)

* fix: Accounts API is broken (PowerDNS-Admin#996)

* Remove unnecessary build step (PowerDNS-Admin#1003)

The builder image does not need to cleanup itself, 
the whole purpose of it is to be dropped after the final artifacts are copied out.

* fix if condition in pretty_domain_name (PowerDNS-Admin#1008)

* user_profile tpl: set email input type attr to "email" (PowerDNS-Admin#1020)

It is then consistent with the email address input elements declared in
admin_edit_account.html, admin_edit_user.html and register.html.

* fix: jsmin 2.2.2 no longer available. Use 3.0.0 (PowerDNS-Admin#1021)

* Update CI

Signed-off-by: Khanh Ngo <khanh.ngo@taxfix.de>

* Update README.md

* Update CI

Signed-off-by: Khanh Ngo <khanh.ngo@taxfix.de>

* Update CI

Signed-off-by: Khanh Ngo <khanh.ngo@taxfix.de>

* Update CI

Signed-off-by: Khanh Ngo <khanh.ngo@taxfix.de>

* Update CI

Signed-off-by: Khanh Ngo <khanh.ngo@taxfix.de>

* strip() whitespace from new local user master data (PowerDNS-Admin#1019)

When creating a new local user, there is a chance that, due to a copy &
paste or typing error, whitespace will be introduced at the start or end
of the username. This can lead to issues when trying to log in using the
affected username, as such a condition can easily be overlooked - no
user will be found in the database if entering the username without the
aforementioned whitespace. This commit therefore strip()s the username
string within routes/{admin,index}.py.

The firstname, lastname and email strings within
routes/{admin,index,user}.py are also strip()ped on this occasion.

* feat: enable_api_rr_history setting (PowerDNS-Admin#998)

* feat: introduce enable_api_rr_history setting to disable api record
changes

* fix: actually store OIDC logout URL (PowerDNS-Admin#988)

* Env oauth url (PowerDNS-Admin#1030)

Overriding settings in DB using environment variable in docker

* Update docker image build script

* Update docker image build script

* Update docker image build script

* Clarify salt re-use for API keys (PowerDNS-Admin#1037)

* OIDC list accounts (PowerDNS-Admin#994)

Added the function to use lists instead of a single string in account autoprovision.

* History Tab Overhaul & Domain Record Modifications Changelog (PowerDNS-Admin#1042)

Co-authored-by: Konstantinos Kouris <85997752+konkourgr@users.noreply.github.com>
Co-authored-by: vmarkop <billy.mark.b.m.10@gmail.com>
Co-authored-by: KostasMparmparousis <mparmparousis.kostas@gmail.com>
Co-authored-by: dimpapac <demispapa@gmail.com>

* Add Keycloak documentation (PowerDNS-Admin#1053)

* default config: add exemplary URL encoding step for SQLA DB URL params

SQLAlchemy database URLs follow RFC-1738, so parameters like username
and password need to be encoded accordingly.

https://docs.sqlalchemy.org/en/13/core/engines.html#database-urls

* Datepicker replace (PowerDNS-Admin#1059)

* replaced jquery-ui-datepicker with bootstrap-datepicker

* removed obsolete static files

* feat: Associate an API Key with accounts (PowerDNS-Admin#1044)

* feat: Associate an API Key with accounts (PowerDNS-Admin#1044)

* Merge branch 'quotes-fix'

Conflicts:
	powerdnsadmin/routes/admin.py

* Quotes fix (PowerDNS-Admin#1066)

* minor fix in history
* made key access more generic

* Use secrets module for generating new API keys and passwords

The implementation of `random.choice()` uses the Mersenne Twister, the
output of which is predictable by observing previous output, and is as
such unsuitable for security-sensitive applications. A cryptographically
secure pseudorandom number generator - which the `secrets` module relies
on - should be used instead in those instances.

* models.user: get_accounts(): order by name

The order of account names returned by User.get_accounts() affects the
order account names are displyed in on /domain/add if the current user
neither has the Administrator role nor the Operator role and the
`allow_user_create_domain` setting is enabled at the same time.

If the current user does have the Administrator or Operator role,
routes.domain.add() already returns accounts ordered by name, so this
change makes it consistent.

* routes/admin.py: refactor DetailedHistory

- Run HTML through the template engine, preventing XSS from various
  vectors
- Fix uncaught exception when a history entry about domain template
  deletion is processed
- Adapt indentation to 4 space characters per level

* routes/admin.py: DetailedHistory: backward-compatibility

See PowerDNS-Admin#1066

* fix: Check user zone create/delete permission

Co-authored-by: zoeller-freinet <86965592+zoeller-freinet@users.noreply.github.com>

* chore: remove funding and sponsor badges (PowerDNS-Admin#1073)

* PDNS-API: factor in 'dnssec_admins_only' basic setting (PowerDNS-Admin#1055)

`GET cryptokeys/{cryptokey_id}` returns the private key, which justifies
that the setting is honored in this case.

* fix: Error in the swagger AccountSummary definition

* Add 'otp_force' basic setting (PowerDNS-Admin#1051)

If the 'otp_force' and 'otp_field_enabled' basic settings are both enabled, automatically enable 2FA for the user after login or signup, if needed, by setting a new OTP secret. Redirect the user to a welcome page for scanning the QR code.

Also show the secret key in ASCII form on the user profile page for easier copying into other applications.

* Bump python-ldap from 3.3.1 to 3.4.0

Bumps [python-ldap](https://github.com/python-ldap/python-ldap) from 3.3.1 to 3.4.0.
- [Release notes](https://github.com/python-ldap/python-ldap/releases)
- [Commits](python-ldap/python-ldap@python-ldap-3.3.1...python-ldap-3.4.0)

---
updated-dependencies:
- dependency-name: python-ldap
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

* login.html: don't suggest previous OTP tokens

This change has been tested to work with:
- Chromium 96.0.4664.93
- Firefox 95.0
- Edge 96.0.1054.57

Co-authored-by: root <root@docker01.uvensys.de>
Co-authored-by: Steffen Schwebel <s.schwebel@uvensys.de>
Co-authored-by: steschuser <steffen@schwebel.online>
Co-authored-by: Carsten Rosenberg <c.rosenberg@heinlein-support.de>
Co-authored-by: Mark Zealey <6083471+mzealey@users.noreply.github.com>
Co-authored-by: Khanh Ngo <khanh.ngo@taxfix.de>
Co-authored-by: Hidde <hvanderheide@nexuz.net>
Co-authored-by: jbe-dw <50663045+jbe-dw@users.noreply.github.com>
Co-authored-by: Andreas Dirnberger <dirnberger@cwtech.at>
Co-authored-by: zoeller-freinet <86965592+zoeller-freinet@users.noreply.github.com>
Co-authored-by: Khanh Ngo <k@ndk.name>
Co-authored-by: Vitali Quiering <vitali@quiering.com>
Co-authored-by: Daniel Molkentin <daniel@molkentin.de>
Co-authored-by: benshalev849 <87974262+benshalev849@users.noreply.github.com>
Co-authored-by: ManosKoukoularis <39616014+kkmanos@users.noreply.github.com>
Co-authored-by: Konstantinos Kouris <85997752+konkourgr@users.noreply.github.com>
Co-authored-by: vmarkop <billy.mark.b.m.10@gmail.com>
Co-authored-by: KostasMparmparousis <mparmparousis.kostas@gmail.com>
Co-authored-by: dimpapac <demispapa@gmail.com>
Co-authored-by: Nick Bouwhuis <nickbouwhuis@users.noreply.github.com>
Co-authored-by: Dominic Zöller <zoeller@freinet.de>
Co-authored-by: Jérôme BECOT <jerome.becot@deveryware.com>
Co-authored-by: Vasileios Markopoulos <83133031+vmarkop@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@ghost ghost mentioned this pull request Jan 1, 2022
@AzorianMatt
Copy link
Member

@kkmanos do you happen to still have detailed memories of this work? I'm trying to figure out exactly what lead to merging in very broken HTML markup that I tracked back to this PR. It seems the original design used a very typical table but during this overhaul, the table was more or less stripped out in favor of tabs but much of the markup from the table was retained with a completely broken syntax.

@kkmanos
Copy link
Contributor Author

kkmanos commented Feb 20, 2023

After taking a quick look at the file changes, we utilized macros to separate components of the template into more compact templates which were re-used, resulting in a cleaner and modular code.
Aside from that, I am not able to recall any details from the changes which were made.

@AzorianMatt
Copy link
Member

@kkmanos duly noted, thank you for the fast response!

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

7 participants