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

Add a permission group that allows members to create names with underscores #521

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

oyvindhagberg
Copy link
Contributor

This PR adds a new permission group that allows members of that group to use the underscore character in names when creating all types of DNS records.
Regular users are still only allowed to use underscores in SRV records.

Bakgrunn:

Utdrag fra epost-tråd mellom Øyvind H. og Mikael D. fredag 27.oktober:

Øyvind:
Magnus H. og jeg hadde en samtale hvor vi snakket om problemet med Windows-domenekontrollere som skaper behov for hostnavn med understrek.
Slik de jobber så trenger de å gjøre endringer i DNS stadig vekk, og de ønsker å kunne bruke mreg til det.
Vi var innom noen alternative løsninger (f.eks. at de bruker en egen DNS-server og går utenom mreg) men fant egentlig ingen annen god løsning som ville dekket behovene.

Mikael:
Magnus og jeg hadde også en diskusjon om dette. Som jeg nevnte så er jeg
tvilende til å åpne for _ (utenom SRV) for "hvermansen" - påtvunget
force eller ei. Å gi superbruker bare for dette er prinsipielt heller
ikke riktig løsning. Det er jo ikke veldig mange som vil trenge dette;
dvs. endre eller lage slike records såpass ofte at man ikke bare kan be
hostmaster eller tilsvarende hver gang. Så jeg er enig i at en klasse
til for dette som man kan gi de med behov er en grei løsning. Så må de
som ønsker denne rollen selvsagt først be hostmaster på sine knær og
ofre et tilstrekkelig antall Snickers!

This commit adds a new permission group that allows members of that
group to use the underscore character in names when creating all types
of DNS records.
Regular users are still only allowed to use underscores in SRV records.
@oyvindhagberg oyvindhagberg added the enhancement New feature or request label Nov 23, 2023
@oyvindhagberg oyvindhagberg self-assigned this Nov 23, 2023
@coveralls
Copy link
Collaborator

Coverage Status

coverage: 99.164% (+0.002%) from 99.162%
when pulling 5d9b1d7 on dns-underscore-group-202311
into ac43c12 on master.

Copy link
Collaborator

@terjekv terjekv left a comment

Choose a reason for hiding this comment

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

Conforms to current design. Looks acceptable to me. ;)

if '_' in name and not isinstance(view, (mreg.api.v1.views.SrvDetail,
mreg.api.v1.views.SrvList)):
mreg.api.v1.views.SrvList)) \
and not request_in_settings_group(request, DNS_UNDERSCORE_GROUP):
Copy link
Collaborator

@terjekv terjekv Nov 23, 2023

Choose a reason for hiding this comment

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

This... works, and as the code base is now this is probably the correct implementation, but checking permissions by either the class of calling view or if the request (hopefully request.user) is a member of a group? It looks really ugly in my opinion. However, it's the way it's done right now and we should probably do a larger cleanup pass rather than making piecemeal changes that only adds to the confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Let's take a look at how the permissions are implented at a later time. I think a re-write may be good, we can do things in a consistent way across the whole code base.

Copy link
Member

@pederhan pederhan left a comment

Choose a reason for hiding this comment

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

LGTM

@terjekv terjekv merged commit 348d1a7 into master Nov 24, 2023
39 checks passed
@oyvindhagberg oyvindhagberg deleted the dns-underscore-group-202311 branch November 29, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants