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

feat: Associate an API Key with accounts #1044

Merged
merged 6 commits into from
Dec 3, 2021

Conversation

jbe-dw
Copy link
Contributor

@jbe-dw jbe-dw commented Nov 16, 2021

As discussed, the feature is added with few changes:

  • An API Key can be linked to accounts, as for domains.
  • Key is allowed on all domains owned by the linked accounts
  • Key with User Role should at least have one domain or one account
  • Key with Administrator or Operator role don't have domains or accounts because they are granted anyway. Any update to existing keys will remove all domains.
  • The API output has slightly been changed: ApiKeys have an "accounts" key and Accounts have an "apikeys" key

@RGanor The code is functionnal. I tested use cases on my docker instance and it should be fine. But, as I may have told, I don't have much time to spend on the feature so It would be cool if folks could test it and report bugs.

Any comment regarding the adopted behaviour is welcome, and any help looking at the swagger definition would be appreciated.

I open this PR as a preview accessible for PDA Users and contributors but consider it "not finished"

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2021

This pull request introduces 4 alerts when merging bbf1151 into bfaf565 - view on LGTM.com

new alerts:

  • 3 for Unused import
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2021

This pull request introduces 1 alert when merging 26b8554 into bfaf565 - view on LGTM.com

new alerts:

  • 1 for Unused import

@RGanor
Copy link
Contributor

RGanor commented Nov 19, 2021

Hey @jbe-dw, Great job with this pull request! this feature was requested for a long time now.
Im testing this right now and I notice something that can cause a lot of confusion. When adding domains/accounts into non user key, then the values are not saved and after refresh the list return to blank.
I understand that this is not a bug, when a key is administrator or operator then it is useless to update its accounts/domains, this is just a cosmetic thing that could lead to confusion.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Nov 19, 2021

Yes you are right. I was thinking about some js to disable and clear the lists whenever changing the role in the selection box but it's not my cup of tea.

If you are not "more comfortable than me" with the jquery stuff and angulaer things, i'll try to add this in the weekend, but if it is easy for you, you're welcome 😺 .

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Nov 19, 2021

Also not that i've slightly updated the template locally but not pushed. I have put the account section first and changed the title and content too. I will add a new commit depending on your ability to help me or not on the js stuff above.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Nov 20, 2021

Added Javascript to enhance behaviour on the ApiKey Edit/Add Page:

  • Selecting Administrator or Operator role hide the selection lists, and changing the role to one of these roles clears the selection
  • A popup appears if you use one of these roles and you have account or domain selected to warn the user that it won't be saved (though it should not happen because of the previous point).
  • A popup appears if you choose a User role and set no account and no domain. The form is not submitted.

Let me know if I have to change something !

Regards

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Nov 20, 2021

There may be a better way to change the selection adressing the widget directly rather than the dom elements, if someone knows ..

@jbe-dw jbe-dw force-pushed the featLinkApiKeyWithAccounts branch 2 times, most recently from 00059bf to c1c3cc5 Compare November 21, 2021 21:56
@RGanor
Copy link
Contributor

RGanor commented Nov 30, 2021

Hey @jbe-dw,
Please resolve the new conflicts and I'll merge this ASAP.
Notice that the previous merge have upgraded the schema so I think you'll need to upgrade as well.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Nov 30, 2021

Hello,

I've updated the swagger file to reflect all the possible error code on the API. It needs a bit of polishing to customize the error messages.

I use it to build my terraform provider, it's cleaner to have a real message rather than "return code is not handled by the client" or a poor "message=({"Error": "", "Errors": []}).

I'll check the conflicts too.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Nov 30, 2021

I don't get why there was a conflict because I didn't make any change on the conflicting section, but I have rebased and dropped a fix into a separate PR. I'll finish updating the Swagger file tomorrow and will push it afterward.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Dec 1, 2021

Finally there's too much extra work because return codes and message are unconsistent. I'll open another PR to review all error codes on the API side and change all "abort(HTTP_CODE)" to a proper "raise ERROR" structure with a clear message, including the requested changes, and edit the swagger file accordingly.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Dec 1, 2021

I let you take a look at the doc changes and check the edit page again.

Changing a key with user to admin clears the domains and accounts, so changing back to user don't set the previous values again, user has to cancel.

If LGTM is ok it's ready to merge then.

@lgtm-com
Copy link

lgtm-com bot commented Dec 1, 2021

This pull request introduces 1 alert when merging 7b7f8b0 into 701a442 - view on LGTM.com

new alerts:

  • 1 for Unused import

@jbe-dw jbe-dw changed the title WIP: feat: Associate an API Key with accounts feat: Associate an API Key with accounts Dec 1, 2021
@jbe-dw jbe-dw changed the title feat: Associate an API Key with accounts WIP: feat: Associate an API Key with accounts Dec 1, 2021
@jbe-dw
Copy link
Contributor Author

jbe-dw commented Dec 1, 2021

Last minute check: I need to add a light modif because the history patch is broken by one of my change

@RGanor
Copy link
Contributor

RGanor commented Dec 1, 2021

@jbe-dw have you finished with this pr? I want to test and merge it.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Dec 2, 2021

Pretty soon. My editor changed tabs for spaces on files or section not concerned by this patch. I reverted it because it was not easy to see what has really changed or not. Doing so and rebuilding gave me a surprise that I have fixed. I want to test it again today before merging.

We introduced broken new features in the past, I want to be sure it is ok. I'll do my best to finish today.

@jbe-dw
Copy link
Contributor Author

jbe-dw commented Dec 2, 2021

Looks OK to me

@jbe-dw jbe-dw changed the title WIP: feat: Associate an API Key with accounts feat: Associate an API Key with accounts Dec 2, 2021
@RGanor RGanor merged commit f45ff2c into PowerDNS-Admin:master Dec 3, 2021
RGanor added a commit that referenced this pull request Dec 3, 2021
@RGanor RGanor linked an issue Dec 6, 2021 that may be closed by this pull request
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>
@jbe-dw jbe-dw deleted the featLinkApiKeyWithAccounts branch April 27, 2022 18:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Keys should be linkable to accounts
2 participants