microbiomedata / nmdc-runtime

Runtime system for NMDC data management and orchestration
https://microbiomedata.github.io/nmdc-runtime/
Other
4 stars 3 forks source link

Simplify authentication and authorization #310

Open shreddd opened 11 months ago

shreddd commented 11 months ago

Currently nmdc-runtime has a somewhat confusing authentication setup including

  1. distinction between sites and users
  2. multiple authorization models (via site_admin and _runtime.api.allow/deny lists)

We should look to simplify this process so that there is just a single entity (we can call it user or client or agent) that authenticates, and access permissions are simply determined by the role this user has in a single common place.

We can use internal naming conventions to determine whether a user is a "site" or a "person" but from the API's standpoint all we need to have are users and roles.

The Runtime API would need to support the suite of features needed to manage users and roles.

shreddd commented 11 months ago

See also #95 (but I think that level of granularity may not be needed either)

PeopleMakeCulture commented 7 months ago

Moving @dwinston comments from metaissue #381

This is a draft of our proposed models for {agents,roles} collections to replace {users,sites, _runtime.api.allow, _runtime.api.deny} collections:

from enum import Enum
from typing import Annotated, Optional

from pydantic import BaseModel, StringConstraints

Orcid = Annotated[str, StringConstraints(pattern=r"^\d{4}-\d{4}-\d{4}-\d{4}")]
ClientID = str
ResourceExpression = str
Action = str

class AgentEnum(str, Enum):
    orcid = "orcid"
    client = "client"

class Privilege(BaseModel):
    re: ResourceExpression
    actions: list[Action]

class Role(BaseModel):
    id: str
    privileges: list[Privilege]

class UserRole(BaseModel):
    role: Role
    re: ResourceExpression

class Agent(BaseModel):
    id: Orcid | ClientID
    type: AgentEnum
    name: Optional[str]
    contact_email: Optional[str]
    roles: list[UserRole]
    privileges: list[Privilege]
    hashed_secret: Optional[str]

The above is based on MongoDB's Role-Based Access Control (RBAC) scheme.

A resource expression would be an API endpoint path regex (e.g. ^/metadata/changesheets, ^/metadata/.+, etc.)

Actions would be more granular than POST, e.g. submit, delete, etc.

Only a type=='client' agent has a hashed_secret (i.e. a client_secret), whereas an orcid agent needs to supply an orcid JWT (not stored) as the secret for authn.

PeopleMakeCulture commented 7 months ago

Note from @shreddd: Make sure to also plan for a migration from current model

ssarrafan commented 7 months ago

Sprint over, no updates, removing from sprint and adding backlog label. If still active please move to new sprint.

turbomam commented 7 months ago

Sprint over, no updates, removing from sprint and adding backlog label. If still active please move to new sprint.

I sure hope that this issue, or some related issue is still active!

dwinston commented 7 months ago

@turbomam yes, planning to tackle this after the related #423

PeopleMakeCulture commented 5 months ago

Code freeze until after GSP

eecavanna commented 5 months ago

I'm posting this comment (1) so I receive notifications about future comments and (2) to say that I don't understand why the Agent class has its own privileges: list[Privilege] attribute, given that it also has a roles: list[UserRole] attribute (the UserRole class has a role: Role attribute, and the Role class has a privileges: list[Privilege]). Maybe the author's intention was to allow a specific user to have certain privileges that aren't represented by a role. 🤷

dwinston commented 5 months ago

I think it's conceptually simpler for privileges to belong to roles, i.e. removing Agent.privileges.

Also, looking over https://www.mongodb.com/docs/manual/core/authorization/, I think it will be useful -- and eliminate some repetition -- if "a role can include one or more existing roles in its definition, in which case the role inherits all the privileges of the included roles" -- e.g., we add a Role.includes = list[Role] attribute.