Skip to content

Sinuna persistentId + app security refactor#64

Merged
lsipii merged 22 commits intomainfrom
VFD-275-laitetaan-users-api-lukemaan-sinunan-kayttajatunniste
Aug 17, 2023
Merged

Sinuna persistentId + app security refactor#64
lsipii merged 22 commits intomainfrom
VFD-275-laitetaan-users-api-lukemaan-sinunan-kayttajatunniste

Conversation

@lsipii
Copy link
Copy Markdown
Contributor

@lsipii lsipii commented Jun 22, 2023

Tehty:

  • Sinunan idTokenin claim luetaan persistentId-tietueesta
    • tarkemmin sanottuna claim määritellään kyseisen tunnistautumistavan luokassa metodissa ResolveTokenUserId()
  • asetetaan eri tunnistautumistavat konffattavaksi pois käytöstä eri ajoympäristöissä konffi-muuttujalla: Security.<auth>.IsEnabled.
    • appsettings.json = default, appsettings.<stage>.json = overrides
  • uudelleenjärjestellään tunnistautumiseen ja valtuutuksen koodipohjaa omaksi moduulikseen: ApplicationSecurity
    • moduulilla ladataan appsettings-konffien perusteella dynaamisesti halutut tunnistautumistavat: ISecurityFeature

Ei tehty, mutta olisi hyvä jossain kohti tehdä:

  • varmistaa että esim. sinunan avaimet ovat latautuneet (SinunaSecurityFeature.LoadOpenIdConfigUrl()) ennen kuin käsitellään ensimmäistäkään authorize-kyselyä, tai sitten toisena (parempana) vaihtiksena että se kysely odottaa jos initialize-tila on vielä kesken
    • ehkä tehdä jokin eventtipohjainen tilakone että authorize-kysely tietää odottaa jos haluttu provider vielä jostain syystä latailee

@lsipii
Copy link
Copy Markdown
Contributor Author

lsipii commented Jun 22, 2023

Helpoin tapa testailuun lokaalissa on:

  • selaimella käy hakemassa idTokenit sinunalta, testbediltä ja miksipä ei suomifi:tlä:
    • avaa osoite: https://virtual-finland-development-auth-files.s3.eu-north-1.amazonaws.com/dev/index.html
    • kirjaudu sisään kaikkiin listan kirjautujmistapoihin
    • selaimen developer-toolsseilla konsolissa aja näinkin nätti one-lineri: console.log(Object.entries({ ...localStorage }).reduce((idTokens, authStateEntry) => { const state = JSON.parse(authStateEntry[1]); idTokens[authStateEntry[0]] = state.authFields?.idToken; return idTokens;},{}));
    • ota talteen idToken kyseiselle auth-tavalle konsolin outputista "authflow-test_<provider-tunniste>_authState"
    • tai sit vain devtoolsseilla localstoragea inspectoimalla sama setti
  • kysele users-api:n /user-endponttia postmanilla tai vaikka curlilla:
curl --location 'http://localhost:5001/user' \
--header 'Accept: application/json' \
--header 'Authorization: Bearer <idToken>

korvaa <idToken> kyseisellä auth-tavan tokenilla

Elikkäs:

  • jos tunnistautumistokenin issuer (eli security-feature) ei ole enabloitu appsettings.local.json:Security:<tunnistautumistapa>:IsEnabled: <boolean> = false
    • pitäisi tulla vastauksena viesti 401 - Invalid token provided
  • jos käyttäjää ei ole olemassa (lokaalissa db) pitäis tulla 401 - User could not be identified as a valid user
  • jos käyttäjä on olemassa: 200 ja detskut!

Käytännössä users-apiin voi luoda käyttäjiä api-kutsuilla tai sit vaikka virtual-finland sovelluksilla (jos ajaa vfd-toolssilla vfd up -p virtual-finland niin domaineissa: http://virtual-finland.localhost/ -> Sinuna, http://features.virtual-finland.localhost/ -> Testbed

@lsipii
Copy link
Copy Markdown
Contributor Author

lsipii commented Jun 22, 2023

Jätän tämän nyt tähän, keskeneräisenä/draftina: en kerennyt pidemmälle miettimään kun kävi niin että olin jo järvessä ja juhannuksen vietossa. EDIT: no laittelen kuitenni avoimeksi kun teknisesti näyttäisi toimivan.

Tässä PR:ssä olennaisin asia on vain toi sinunan persistent_id-muutos joka toteutuksena löytyy ekasta kommitista:

if (jwtSecurityToken.Issuer == "https://login.iam.qa.sinuna.fi") // @TODO: read from SinunaIdentityProviderConfig

Toissijaista on refaktorointi ja sit featti että appsettings.json ja sen stage-konffiyliajoilla voi poistaa tietyn tunnistautumistavan sovelluksen kyvykkyyksistä: tässä on hyötynä se että jos vaikka testbed on disabloitu niin palvelin jättää tekemättä kyselyn sinne testbedin pubkey-osoitteisiin.

Tosiaan hieman pikaisesti raavittu kasaan tämä eli mun puolesta saapi tätä vaikka kehitellä eteenpäin, kokonaan uudelleenkirjoittaa, hyväksyä sellaisenaan tai jättää huomiotta. Käytännössä mitkä vain kommentit helpottaa kun jos tätä vilkaisee uusiksi syssymmällä.

@lsipii lsipii marked this pull request as ready for review June 22, 2023 17:59
@LauriGofore
Copy link
Copy Markdown
Contributor

LauriGofore commented Jun 26, 2023

Jotain havaintoja kun tätä disablointia/enablointia testasi lokaalissa: SuomiFI token ei kelpaa riippumatta siitä oliko enabled vai ei. Tarina ei tosin tarkenna pitikö toimiakaan ja oleellisinta varmaan tässä vaiheessa, että SInuna ja Testbed pelittää, jotka kyllä pelittivätkin (enablointi ja disablointi). 👍

e: näyttäisi koodin perusteella, että tuki myös SuomiFi:lle pitäisi olla - lienee jossain muualla vika kun tuossa enabloinnissa.

e2: virheen jonka heittää on 401: Access denied, vs. 401 Invalid token

@LauriGofore
Copy link
Copy Markdown
Contributor

LauriGofore commented Jun 26, 2023

Näissä Security/Features luokissa on jonkun verran saman toistoa (BuildAuthentication, BuildAuthorization, GetSecurityPolicySchemeName) - voisko näille luoda vaikka jonkun abstraktin luokan mistä nämä metodit voisi inherittaa, että ois ns. enemmän DRY. Abstrakti luokka tuli ekana mieleen, en tosin tiedä oisko se tälläsessä oikea patterni ja onko syystäkin kuitekin samat metodit joka luokassa toistettuna. 🤔

Copy link
Copy Markdown
Contributor

@lassipatanen lassipatanen left a comment

Choose a reason for hiding this comment

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

Näyttää ihan toimivalta ratkaisulta. Varmaan jos haluaisi tehdä enemmän .net tyylillä, niin konfiguroisi nuo palikat service collection extension luokissa minne voi vaikka passata optionseja, mutta eipä se toimintaa muuta ja on toistaalta vähän turhaa ajan käyttöä. Sitten jos katsoo vielä että koodi on yksikkötestattavaa niin olisi varmaan ihan jees.

Vähän hahmottelin tuonne shared repoon miten tuota voisi toteuttaa extension metodeilla, mutta ei siitä mitenkään valmista tullut.

Virtual-Finland-Development/vfd-shared-libraries@57d6186

public static string TestBedBearerScheme => "DefaultTestBedBearerScheme";
public static string SuomiFiBearerScheme => "SuomiFiBearerScheme";
public static string SinunaScheme => "SinunaScheme";
public static string AllPoliciesPolicy => "AllPolicies";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tätä ei käytetä missään

Comment on lines +104 to +107
public string GetSecurityPolicyScheme()
{
throw new NotImplementedException();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tämä on turha


namespace VirtualFinland.UserAPI.Security.Features;

public class SuomiFiSecurityFeature : ISecurityFeature
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ehkä suomi fi kirjautumiseen liittyvät koodit voisi vain poistaa, kun ei niillä taida olla mitään käyttöä, ja jos taas joskus pitää saada ne mukaan niin vnahoista koodeista varmaan selviää miten se pitää konffata.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Näin voisi olla juu, ehkä pittääpi tehdä siitä taski erikseen.

@lsipii
Copy link
Copy Markdown
Contributor Author

lsipii commented Jun 29, 2023

SuomiFI token ei kelpaa riippumatta siitä oliko enabled vai ei. Tarina ei tosin tarkenna pitikö toimiakaan

Jotain havaintoja kun tätä disablointia/enablointia testasi lokaalissa: SuomiFI token ei kelpaa riippumatta siitä oliko enabled vai ei. Tarina ei tosin tarkenna pitikö toimiakaan ja oleellisinta varmaan tässä vaiheessa, että SInuna ja Testbed pelittää, jotka kyllä pelittivätkin (enablointi ja disablointi). 👍

e: näyttäisi koodin perusteella, että tuki myös SuomiFi:lle pitäisi olla - lienee jossain muualla vika kun tuossa enabloinnissa.

e2: virheen jonka heittää on 401: Access denied, vs. 401 Invalid token

Ah-so, sori tähän taisi liittyä virheelliset ohjeet suomifi:n kohdalta: kun ajaa users-api:a local-stagessa niin ei kelpaa dev-stagen suomifi-tunniste vaan se vaatisi local-stagen suomifitokenia. Sen saisi haettua siten että lokaalissa (vfd-toolssilla) vfd up -s authentication-gw ja sit navigoi http://virtualfinland-authgw-demo.localhost/ josta sit vastaavasti saisi suomifi:n lokaalin idtokenin.

Mutta joo tuon nyt voinee jättää huomiotta kun ei ole suomifi-toteutusta tulossa käyttöön.

@lsipii
Copy link
Copy Markdown
Contributor Author

lsipii commented Jul 17, 2023

Varmaan jos haluaisi tehdä enemmän .net tyylillä, niin konfiguroisi nuo palikat service collection extension luokissa minne voi vaikka passata optionseja

Toi esimerkki näyttää oivalta👍. Pitääpä reworkkia tätä hieman lomalta palaneiden silmien (🔥🔥) kanssa!

EDIT: hieman reworkkia tässä kohti tapahtunut!

@lsipii lsipii merged commit 5b7f9f6 into main Aug 17, 2023
@lsipii lsipii deleted the VFD-275-laitetaan-users-api-lukemaan-sinunan-kayttajatunniste branch August 17, 2023 12:13
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.

3 participants