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

Extend Organisation with a uuid property and setter. #88

Merged
merged 3 commits into from
Sep 15, 2020
Merged

Conversation

byrman
Copy link
Member

@byrman byrman commented Sep 14, 2020

Changing unique_id into a UUIDField might break things (?), but a separate property seems both harmless and useful (e.g. for serializing unique_id as a proper UUID).

@byrman byrman assigned reinout and byrman and unassigned reinout Sep 14, 2020
@byrman byrman requested review from caspervdw and reinout and removed request for caspervdw September 14, 2020 10:47
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 75.0% when pulling d26de1c on byrman_uuid into d0f82cd on master.

Copy link
Contributor

@reinout reinout left a comment

Choose a reason for hiding this comment

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

Nog even kijken of er een commentaarregel bij het veld danwel de property moet.

lizard_auth_client/models.py Show resolved Hide resolved
lizard_auth_client/models.py Show resolved Hide resolved
@caspervdw
Copy link
Contributor

Is het mogelijk om er ook een validator aan te hangen zodat wanneer een unique_id in de admin wordt ingevuld, wordt gecheckt of het een valide UUID is?

Op dit moment gaat dat regelmatig fout.

@byrman
Copy link
Member Author

byrman commented Sep 15, 2020

Op dit moment gaat dat regelmatig fout.

Hoe dan?

Screenshot 2020-09-15 at 08 50 01

@byrman
Copy link
Member Author

byrman commented Sep 15, 2020

Ah, je bedoelt in Lizard i.p.v. SSO? Raar dat je daar organisaties kunt aanmaken, want dat zou alleen in de SSO moeten gebeuren.

@caspervdw
Copy link
Contributor

Ja, ik bedoel in Lizard. De organisaties in de SSO worden genegeerd door Lizard (en door 3Di ook geloof ik).

@byrman
Copy link
Member Author

byrman commented Sep 15, 2020

Ja, ik bedoel in Lizard. De organisaties in de SSO worden genegeerd door Lizard (en door 3Di ook geloof ik).

Dat is nooit de bedoeling geweest: organisaties zouden maar op 1 plek worden gegenereerd. Alleen dan gebruiken Lizard en 3Di dezelfde UUIDs voor dezelfde organisaties.

Anyway, jouw probleem moet dus in Lizard worden opgelost en niet in deze repo?

@caspervdw
Copy link
Contributor

Als ik reinout goed begrijp, dan is de aanwezigheid van een Organisation model in de SSO een overblijfsel van eerdere tijden toen authorisatie ook nog op de SSO werd geregeld. Nu doet SSO alleen authenticatie. Users mogen wel lid zijn van een organisatie op de SSO, applicaties doen helemaal niets met die info.

Het organisation model in Lizard staat gedefinieerd in lizard-auth-client, dus het is het makkelijkst om een model-field-validator aan het model in deze repo te hangen.

@reinout
Copy link
Contributor

reinout commented Sep 15, 2020

Ah, dit is de lizard/3di-versie van het organisation model. Ik haalde lizard-auth-server en -client even door elkaar en dacht dat dit een wijziging op de SSO was :-)

Ik mis waarschijnlijk wat achtergrond: gaat het wel goed met de coordinatie tussen 3di en lizard dan? SSO (lizard-auth-server) zit de uuid niet in. Lizard en 3di gaan elk een ander uuid genereren als het goed is. => huh?

Maar goed, het zal wel nodig zijn voor de amazon koppeling?

@byrman
Copy link
Member Author

byrman commented Sep 15, 2020

Als ik reinout goed begrijp

Volgens mij zit het toch echt anders: SSO doet authenticatie èn definieert organisaties. Lid maken van een organisatie doet de SSO niet meer, maar dat heb ik ook nooit beweerd.

dus het is het makkelijkst om een model-field-validator aan het model in deze repo te hangen.

Dat breekt tests onmiddellijk (maar is te fixen) en is niet backward compatible. Ik weet niet of andere applicaties ook echt UUIDs gebruiken. Dat is alleen zo als ze gebruik maken van de automatische gegenereerde waarden.

@caspervdw
Copy link
Contributor

Ik mis waarschijnlijk wat achtergrond: gaat het wel goed met de coordinatie tussen 3di en lizard dan? SSO (lizard-auth-server) zit de uuid niet in. Lizard en 3di gaan elk een ander uuid genereren als het goed is. => huh?

Volgens mij zit het toch echt anders: SSO doet authenticatie èn definieert organisaties. Lid maken van een organisatie doet de SSO niet meer, maar dat heb ik ook nooit beweerd.

De organisatie-uuids worden handmatig gelijk gehouden tussen Lizard en SSO; applicatiebeheer gebruikt hier copy-paste voor. Het proces van een nieuwe organisatie toevoegen is: Organisation aanmaken in SSO, UUID kopieren, Organisation aanmaken in Lizard. Hier gaat het vaak fout omdat de streepjes niet worden verwijderd.

Dat breekt tests onmiddellijk (maar is te fixen) en is niet backward compatible. Ik weet niet of andere applicaties ook echt UUIDs gebruiken. Dat is alleen zo als ze gebruik maken van de automatische gegenereerde waarden.

Goed punt, ik kan alleen geen voorbeelden bedenken. Ik denk dat we wel het risico kunnen nemen dat andere applicaties hierop breken. Het veld is duidelijk bedoeld geweest als UUID.

@reinout
Copy link
Contributor

reinout commented Sep 15, 2020

Ok, duidelijk.
Inderdaad, er stáát gewoon al een unique_nogwat in lizard-auth-server :-)

@byrman byrman merged commit 869d8bb into master Sep 15, 2020
@byrman byrman deleted the byrman_uuid branch September 15, 2020 09:35
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

4 participants