Naamgeving Auth

[brussee]

Volgens mij moet de onderstaande functie isAuthorized heten en moet de statische ApiAuthController functie authenticate heten.

    /**
     * @return boolean
     */
    public function authorize() {
        return ApiAuthController::isAuthorized() && LoginModel::mag('P_OUDLEDEN_READ');
    }

[qurben]

@manduro

[qurben]

Afaik is authorize een builtin functie van de restserver lib die wordt gebruikt om iedere request te controleren.

[brussee]

De interpretatie van isAuthorized is dat het al gebeurd is, hier wordt het bepaald dus tegenwoordige tijd.

Als de ApiAuthController een ::mag(P_LOGGED_IN) doet is het authorization, nu doet ie alleen de check of je bekend bent en wie je bent: authentication.

edit2: dit is ook een potential pitfall: iemand die denkt dat als ApiAuthController 'ok' zegt, dat iemand dus ook mag inloggen, dat is dus niet zo!

edit: X-Csr-Authorization zou dus eigenlijk X-Csr-Authentication moeten heten in mijn ogen. Zoals we server side authentication_method hebben.

[Manduro]

Eerste punt: ja, isAuthorized zou beter zijn in dit geval, maar authorize is een standaard functie van de library zoals Gerben zegt en daarvoor een prima keus wat mij betreft. De library weet namelijk niet of je elke request of eenmalig autoriseert. Er is zelfs in het geval van tokens ook wat voor authorize te zeggen, omdat het token elke request gecheckt wordt.

Tweede punt: volgens mij is het wel degelijk authorization. Als iemand een valide token heeft betekent dit namelijk ook dat diegene P_LOGGED_IN rechten heeft.

De naam van de header Authorization is een standaard, die in browsers ook een speciale security status heeft. Ik zou hem dan ook eigenlijk zonder de X-Csr- prefix zou willen gebruiken, maar daar waren wat issues mee. Ook hier geldt dat er impliciet P_LOGGED_IN rechten van toepassing zijn.

Een mooie refactor (zonder breaking changes) die dit soort terminologie een beetje omzeilt zou misschien het onderstaande zijn, waar in isAuthorized de LoginModel::mag() check wordt uitgevoerd.

public function authorize() {
    return ApiAuthController::isAuthorized('P_OUDLEDEN_READ');
}

Voor een uitgebreidere refactor (met mogelijk breaking changes) is het misschien een idee om niet alleen het userId, maar ook de rechten op te slaan in het token. Dan is er ook elke request een query minder nodig om de rechten te checken.

[brussee]

Er is een use case waarbij iemand geen P_LOGGED_IN heeft, maar wel mag inloggen: de wiki. Sowieso is het niet gezegd dat iemand in mag loggen als die een account heeft: account en lid-status zijn losgekoppeld dus kunnen uit de pas lopen; ex-leden mogen niet meer inloggen, maar hebben mogelijk nog wel een account. Er zijn vast nog wel meer gevallen, zie daarom ook edit2 hierboven, heb je die nog gelezen of ging dat even parallel?

[Manduro]

Gezien deze hele discussie alleen over terminologie gaat, vind ik het dan weer erg verwarrend dat iemand zonder P_LOGGED_IN rechten wél mag inloggen ;)

[brussee]

Inloggen zonder rechten kan wel (als je een account hebt), maar mag niet op de interne stek zonder P_LOGGED_IN (zoals ex-leden), en een account mag desondanks als je wiki-rechten hebt wel op de wiki komen. Inloggen is slechts authenticeren van een account, waarbij niet mag(P_LOGGED_IN) wordt gecontroleerd, dat wordt namelijk later gedaan door de Controller->magAction(). Je kunt P_LOGGED_IN misschien beter hernoemen naar P_WWW_INTERN om dit duidelijker te maken.

Kortom: als dit verhaal niet duidelijk is of verkeerd wordt begrepen door de programmeur heb je een beveiligingslek.

[brussee]

Wat er concreet nu mis is:

• [ ] isAuthorized moet hernoemd worden naar authenticate om aan te geven dat die functie de gebruiker gaat vaststellen aan de hand van de toegestane authenticatiemethoden (dit moet nog gebeuren: dus niet isAuthenticated, als je dat wil doe authentication_method != null ofzo).
• [ ] P_LOGGED_IN moet hernoemd worden naar P_STEK_INTERN omdat het geen rechten betreft die je nodig hebt om te kunnen inloggen, inloggen kan altijd als je een account hebt (effectief werkt het nu wel intuitief doordat AclController altijd minimaal P_LOGGED_IN checkt, maar niet alle controllers zijn AclControllers, dus daar ga je alweer!).
• [ ] ApiAuthController moet hernoemd worden naar ApiAuthenticationController, want deze controller doet nu geen check op P_LOGGED_IN (hoewel @Manduro wel in die veronderstelling was als ik het goed begrijp).
• [ ] Controlleer of altijd in elke controller minimaal P_LOGGED_IN gechecked wordt. Voorbeeld hieronder is dus FOUT (daarom opende ik dit ticket, om dat te voorkomen):

  /**
   * @return boolean
   */
  public function authorize() {
      return ApiAuthController::isAuthorized(); // <- zonder  && LoginModel::mag('P_LOGGED_IN');
  }

[brussee]

OF doe wel een check op P_LOGGED_IN bij het inloggen en als je die rechten niet hebt maak het equivalent aan als geen account hebben. Ex-leden kunnen dan automatisch niet meer op de stek bij het wijzigen van de lid-status, ook al hebben ze nog een account. Het authenticate/authorize verhaal is nog steeds van belang, maar omdat je bij het authenticeren nu ook P_LOGGED_IN authorisatie doet mag je de functie-namen zo houden wat mij betreft, als het verhaal maar duidelijk blijft. Hiermee drop je de wiki-gebruiker use case: wel account, geen interne stek, wel wiki.