Skip to content

Security Features Documentation#100

Merged
lsipii merged 7 commits intomainfrom
VFD-329-dokumentoidaan-jarjestelman-suojaus
Nov 15, 2023
Merged

Security Features Documentation#100
lsipii merged 7 commits intomainfrom
VFD-329-dokumentoidaan-jarjestelman-suojaus

Conversation

@lsipii
Copy link
Copy Markdown
Contributor

@lsipii lsipii commented Nov 14, 2023

  • add documentation of the users api security features

@lsipii lsipii changed the title docs: security readme Security Features Documentation Nov 14, 2023
@lsipii lsipii requested a review from LauriGofore November 14, 2023 15:49
Comment thread Docs/README.security.md

Users API supports a couple of different approaches on validating the audience claim in the authentication token. The audience claim is used to validate that the token is intented to be used by the API. Users API treats the audience claim as an identity of the external application and with that it's possible to restrict access only to specific applications. The supported approaches are using a static configuration value list of allowed audiences or using external service API to validate the audience exists and belongs in an allowed group.

The audience validation is configured in the [appsettings.json](../VirtualFinland.UserAPI/src/VirtualFinland.UsersAPI/appsettings.json)-file properties block: `Security:Authorization:<provider>:AudienceGuard` and has settings for both approaches: `StaticConfig` and `Service`. The Service -approach requires the security feature (authentication provider implementation) implement the `ValidateSecurityTokenAudienceByService()` method of the [SecurityFeature](../VirtualFinland.UserAPI/src/VirtualFinland.UsersAPI/Security/Features/SecurityFeature.cs) class.
Copy link
Copy Markdown
Contributor

@LauriGofore LauriGofore Nov 15, 2023

Choose a reason for hiding this comment

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

Teknisempi juttu, mutta tuota ValidateSecurityTokenAudienceByService() -metodi ei vissiin oikeasti ole "required"? Jos vaikka tuo SinunaSecurityFeature jättäisi tuon metodin implementoimatta, niin ei anna herjaa asiasta. Jos muutti tuon base luokan abstraktiksi ja kyseisen metodin myös, niin alkoi herjaus. Mutta ehkä tämän ei ollut tarkoitus olla niin stricti tai se erhe tulee sitten jossain muualla vastaan, en ala väittää että tietäisin paremmin. :D

Copy link
Copy Markdown
Contributor

@LauriGofore LauriGofore Nov 15, 2023

Choose a reason for hiding this comment

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

Nyt jälkiajatuksena, että niinhän se sitten tökkäsee viimeistään tuohon NotImplementedException. Se ei vaan koodaajalle sano suoraan ennen kovaa ajoa, että tämä pitäisi implementoida.

Copy link
Copy Markdown
Contributor Author

@lsipii lsipii Nov 15, 2023

Choose a reason for hiding this comment

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

Tuossa valinnassa on haettu sitä että ei olisi pakko implementoida jos toiminto ei ole käytössä. Toiminto menee taas päälle ja pois ajonaikaisilla asetuksilla niin siitä ei taida koodiin saada kätevästi herjaa tässä systeemissä eli hieman ikävästi sit vasta ajon aikana näkee virheen. Voisihan sen sit toki määritellä abstraktiksi ja tehdä tyhjät toteutukset sinne missä ei tarvetta, ja samalla tehdä testikeisseille oma testi-sec-feat joka perii, vaan se tie ei jotenkin ollut tätä tehdessä tarpeeksi houkutteleva. Lisäsin nyt ensiavuksi kommentin siihen, että milloin funkkari tulisi impliä.

Copy link
Copy Markdown
Contributor

@LauriGofore LauriGofore left a comment

Choose a reason for hiding this comment

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

👏 Varsin selkeästi selitetty ja kun seuraa mainituilla tiedostorefeillä, niin saa oikein hyvän käsityksen toiminnasta. Well done.

…com:Virtual-Finland-Development/users-api into VFD-329-dokumentoidaan-jarjestelman-suojaus
@lsipii lsipii merged commit 3adf0d4 into main Nov 15, 2023
@lsipii lsipii deleted the VFD-329-dokumentoidaan-jarjestelman-suojaus branch November 15, 2023 08:37
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.

2 participants