[ASP.NET Core] Object van Filter naar Action sturen

[NickThissen]

Ik ben bezig met een ASP.NET Core project, en ik merk dat ik in een bepaalde controller ontzettend vaak dezelfde code aan het copy/pasten ben. Dit is natuurlijk niet handig, dus ik zocht een manier om dit te verhelpen.

De controller gaat over een soort van admin interface dat gebruikers kunnen gebruiken om hun account te beheren. Denk een beetje aan een soort Play Store voor apps, je kan apps toevoegen en beheren. In mijn geval heet een app een 'theme'.

In elke action moet ik twee dingen doen:
- Het theme ophalen vanuit de database
- Verifieren dat de huidige gebruiker ook echt de eigenaar van het theme is.

De code ziet er in bijna elke action hetzelfde uit:

C#:

[HttpGet]
public async Task<IActionResult> Dashboard(int? id)
{
    // Find theme
    var theme = await _storeRepository.GetThemeAsync(id);
    if (theme == null || theme.Deleted)
        return NotFound();

    // Find user
    var user = await GetCurrentUserAsync();
    
    // Verify user is theme owner
    if (user.Id != theme.OwnerId)
        return Unauthorized();

    var model = new ThemeStoreViewModel();
    model.User = user;
    model.Theme = theme;

    return View(model);
}

Regels 4 - 14 komen in vrijwel elke action terug, wat natuurlijk niet handig is.


Nu begrijp ik dat dit soort checks thuishoren in een Filter. Na wat zoekwerk kom ik op de volgende implementatie:

C#:

    public class ValidateThemeOwnerAttribute : TypeFilterAttribute
    {
        public ValidateThemeOwnerAttribute() 
            : base(typeof(ValidateThemeOwnerImpl))
        {
        }

        private class ValidateThemeOwnerImpl : IAsyncActionFilter
        {
            private readonly StoreRepository _repository;

            public ValidateThemeOwnerImpl(StoreRepository repository)
            {
                _repository = repository;
            }

            public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
            {
                if (context.ActionArguments.ContainsKey("id"))
                {
                    // Get theme by id
                    var id = context.ActionArguments["id"] as int?;
                    var theme = await _repository.GetThemeAsync(id);

                    // Check exists
                    if (theme == null || theme.Deleted)
                    {
                        context.Result = new NotFoundObjectResult(id.Value);
                        return;
                    }

                    // Get user
                    var controller = (UserManagerControllerBase) context.Controller;
                    var user = await controller.GetCurrentUserAsync();

                    // Check is user is owner
                    if (theme.OwnerId != user?.Id)
                    {
                        context.Result = new UnauthorizedResult();
                        return;
                    }

                    await next();
                }
            }
        }
    }

In principe werkt dit, en kan ik deze code steeds laten draaien door een [ValidateThemeOwner] attribuut te gebruiken.


De vraag nu: ik schiet hier nog steeds niet veel mee op. Ik heb namelijk nog steeds de theme en de user nodig in de action om naar de view te sturen. Ik moet dus in de action alsnog de theme en de user ophalen. Het enige verschil is dat ik er nu "veilig" vanuit kan gaan dat ze beide bestaan en dat de "check for owner" niet meer nodig is. Maar dat zijn twee regels code die ik schrap. Daar tegenover staat dat ik nu twee keer een database query moet doen (een keer in de filter, daarna nog eens in de action). Lijkt me niet goed..?

Hoe stuur ik de theme en user vanuit de filter (die ik daar toch al heb) naar de action (waar ik ze nodig heb en niet opnieuw wil gaan zoeken)?

Ik kan twee manieren vinden op internet:

1. RouteData gebruiken. In de Filter kan ik zoiets doen:

C#:

context.RouteData.Values.Add("theme", theme);

Daarna weer ophalen uit de RouteData in de action. Ok, maar is niet zo handig want ik ga nu "magic strings" gebruiken ("theme"). Niet heel erg flexibel en niet zo mooi. Natuurlijk kan ik een const definieren ergens maar probleem blijft.

2. De objecten aan de ActionParameters toevoegen:

C#:

context.ActionParameters["theme"] = theme;
context.ActionParameters["user"] = user;

// In action:
[ValidateThemeOwner]
public async Task<IActionResult> Dashboard(int? id, Theme theme, User user)
{
}

In principle vergelijkbaar probleem met magic strings. Daarnaast is dit nog veel slechter denk ik omdat er nu ineens "random" parameters aan de GET method toegevoegd worden die er eigenlijk niet thuishoren en die door een filter gevuld worden. Code wordt compleet onleesbaar en de API klopt niet eens meer (GET signature zal niet kloppen want je moet geen theme en user meesturen).


Hoe los ik dit op een fatsoenlijke manier op?

[whoami]

Je zou ook gebruik kunnen maken van HttpContext.Items.

Echter, ook in dat geval zit je met 'magic strings', maar op zich hoeft dat volgens mij geen probleem te zijn indien je een aantal const strings declareert die de keys voorstellen.

Aangezien je die keys dan zowel in de controller als in de filter nodig hebt, zou ik deze in een (static) class gaan declareren.

[NickThissen]

Hm ja, het zou wel werken zo, maar vind het niet de beste oplossing.

Misschien is het gebruik van Filters toch niet de goeie keuze om dit op te lossen? Zijn er nog andere opties om deze copy/paste code te vermijden?

[TheNameless]

Even heel praktisch; wat als je een nieuwe service class maakt die 2 objecten terug geeft (Theme en User) en een exceptie gooit als de randvoorwaarden niet kloppen (theme.IsDeleted etc.).

Met een custom HandleErrorAttribute kan je vervolgens deze exceptie opvangen en dan de juiste HTTP statuscode teruggeven.

[ Voor 6% gewijzigd door TheNameless op 05-11-2017 11:28 ]

[NickThissen]

TheNameless schreef op zondag 5 november 2017 @ 11:26:
Even heel praktisch; wat als je een nieuwe service class maakt die 2 objecten terug geeft (Theme en User) en een exceptie gooit als de randvoorwaarden niet kloppen (theme.IsDeleted etc.).

Met een custom HandleErrorAttribute kan je vervolgens deze exceptie opvangen en dan de juiste HTTP statuscode teruggeven.

Hmm, misschien wel een goed idee, bedankt! Eigenlijk hou ik er niet zo van om exceptions te gebruiken om dit soort logica te ondervangen, maar aan de andere kant is de situatie waarbij de checks mis gaan ook echt niet een normale situatie, dus in dit geval lijkt me dit prima. Ik ga het eens proberen.

[whoami]

Ik vind het gebruik van exceptions in dit geval in ieder geval niet de goede manier. Je gaat dan een exception gaan opvangen om gewoon een NotFound terug te geven bv.

[MKoole]

Hoi Nick,
Weet je zeker dat de queries, om de objecten op te halen, twee keer worden uitgevoerd?

Wellicht gebruik je EntityFramework, als een object al ingeladen is in de context, wordt deze niet opnieuw opgehaald namelijk.

[Alex)]

MKoole schreef op maandag 6 november 2017 @ 12:35:

Wellicht gebruik je EntityFramework, als een object al ingeladen is in de context, wordt deze niet opnieuw opgehaald namelijk.

Dat is niet correct.

Entity Framework doet niet aan second level caching: als je EF een vraag stelt zal hij altijd een query uitvoeren tegen de database, zelfs als het object al eerder is opgehaald. In dat laatste geval krijg je wél hetzelfde object terug, want EF trackt de objecten adhv hun key(s).

[MKoole]

Alex) schreef op maandag 6 november 2017 @ 19:26:
[...]

Dat is niet correct.

Entity Framework doet niet aan second level caching: als je EF een vraag stelt zal hij altijd een query uitvoeren tegen de database, zelfs als het object al eerder is opgehaald. In dat laatste geval krijg je wél hetzelfde object terug, want EF trackt de objecten adhv hun key(s).

Bedankt voor je uitleg! Ergens wel logisch, EF kan het resultaat van die query niet van tevoren weten

[NickThissen]

Ik denk dat we het allemaal ongeveer eens zijn dat er flink wat "niet mooie" oplossingen zijn maar ik ben nog niet echt dichterbij een mooie oplossing

Dit moet toch een veel voorkomend probleem zijn? Elke website die iets met user accounts doet waarbij users een eigen soort admin pagina moeten hebben zal tegen exact dit probleem aanlopen (elke action moet gechecked worden of de user wel toestemming heeft waarvoor zowel in de authorizatie als in het vervolg iets uit de database gehaald moet worden). Ik kan er echter weinig over vinden op google...

De beste oplossing tot nu toe is gedoe met de RouteData of HttpContext.Items (lijkt vrijwel hetzelfde) maar daar vind ik de logica wel erg "verborgen" achter uitwisseling van data via een of andere key.

Misschien zou ik in de base class van de Controller iets kunnen maken wat de objecten in en uit de RouteData haalt waardoor de code iets logischer zou moeten zijn om te lezen.

[dominicr]

Voor authenticatie in groffe zin (een hele pagina of hele controller) voeg ik een specifieke role claim toe aan de userprincipal, maar voor zoiets als een specifiek theme zou ik toch gewoon iedere keer een query uitvoeren.

[NickThissen]

Ik kan volgens mij geen claims e.d. gebruiken want ik heb gewoon een lijstje users en een lijstje themes en elke user kan van elke theme "owner" zijn (nou ja, een theme heeft maar een owner, maar dat kan elke user zijn). Users kunnen hun eigen themes maken (en dan zijn ze owner), en ik moet natuurlijk checken dat user A niet het theme van user B kan aanpassen.

Wat bedoel je met "iedere keer een query uitvoeren" precies? Uiteraard moet er elke action een query naar de db, maar momenteel heb ik twee keuzes:
- De code die "is owner" checkt in elke action copy/pasten (Bah! Wat als ik later nog een conditie wil toevoegen? Dit vraagt om problemen)
- Een filter gebruiken en de query elke keer twee keer uitvoeren...

[sig69]

A: De query om een user en een thema op te halen stellen niets voor natuurlijk, dus daar zou ik me niet al te veel zorgen over maken.
B: Dat gezegd hebbende ben ik het met je eens dat het beter moet kunnen.

Wat de thema's betreft: dat kunnen ook gewoon claims zijn natuurlijk (in principe kan zo'n beetje alles een claim zijn). Dit is misschien wel een aardig startpunt: https://docs.microsoft.co...rity/authorization/claims.
Met een custom policy die het thema ophaalt en checkt of deze in de claims van de user voorkomt ben je er dan volgens mij. Enige nadeel is dat de aanwezigheid van een themeId in de routeData dan "by convention" is, maar dat is half ASP.Net MVC toch al.

[ChaZy]

Misschien is dit een optie: https://docs.microsoft.co...cebased?tabs=aspnetcore2x

Hier schrijf je een AuthorizationHandler waarbij je de resources die je nodig hebt vanuit je controller mee geeft. Zo heb je de resources die je nodig hebt in je controller en je kunt ze hergebruiken in je Authorization

[NickThissen]

ChaZy schreef op maandag 13 november 2017 @ 13:43:
Misschien is dit een optie: https://docs.microsoft.co...cebased?tabs=aspnetcore2x

Hier schrijf je een AuthorizationHandler waarbij je de resources die je nodig hebt vanuit je controller mee geeft. Zo heb je de resources die je nodig hebt in je controller en je kunt ze hergebruiken in je Authorization

Dit ziet er wel interessant uit, echter zie ik nog niet zo snel wat hier het voordeel van is vergeleken met gewoon mijn eigen service die precies hetzelfde kan doen. Het is wat netter misschien maar uiteindelijk blijft het nog steeds een aantal standaard regels code kopieren in elke action.

Het moet dan maar, het ziet er niet naar uit dat ik dit kan vermijden

Toch bedankt!