Giter VIP home page Giter VIP logo

Comments (7)

josecelano avatar josecelano commented on September 10, 2024 2

Hi @mario-nt just a clarification, regarding authentication there are three types of endpoints:

  1. Endpoints that don't require a logged-in user at all.
  2. Endpoints that require a logged-in user (for example, to upload a torrent).
  3. Endpoint that a logged-in user is optional. If you are logged in you get a different response but you can request the same endpoint without a logged-in user.

The "extractor that could be " would be used for the type-3 endpoitns.

from torrust-index.

josecelano avatar josecelano commented on September 10, 2024 1

I've been discussing with @mario-nt how to use the auth service in the middleware.

pub async fn upload_torrent_handler(
    State(app_data): State<Arc<AppData>>,
    Extract(maybe_bearer_token): Extract,
    multipart: Multipart,
) -> Response {
    let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
        Ok(user_id) => user_id,
        Err(error) => return error.into_response(),
    };

    let add_torrent_form = match build_add_torrent_request_from_payload(multipart).await {
        Ok(torrent_request) => torrent_request,
        Err(error) => return error.into_response(),
    };

    match app_data.torrent_service.add_torrent(add_torrent_form, user_id).await {
        Ok(response) => new_torrent_response(&response).into_response(),
        Err(error) => error.into_response(),
    }
}

We want to move this code:

    let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
        Ok(user_id) => user_id,
        Err(error) => return error.into_response(),
    };

to the middleware. We found these links:

How do you access state in a custom Extractor?:

from torrust-index.

josecelano avatar josecelano commented on September 10, 2024

Hi @WarmBeer in the new Axum implementation I'm using an extractor:

https://github.com/torrust/torrust-index-backend/blob/develop/src/web/api/v1/extractors/bearer_token.rs

because some endpoints require authentication and others don't.

And this is how it looks like in the upload torrent endpoint:

pub async fn upload_torrent_handler(
    State(app_data): State<Arc<AppData>>,
    Extract(maybe_bearer_token): Extract,
    multipart: Multipart,
) -> Response {
    let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
        Ok(user_id) => user_id,
        Err(error) => return error.into_response(),
    };

    let torrent_request = match get_torrent_request_from_payload(multipart).await {
        Ok(torrent_request) => torrent_request,
        Err(error) => return error.into_response(),
    };

    let info_hash = torrent_request.torrent.info_hash().clone();

    match app_data.torrent_service.add_torrent(torrent_request, user_id).await {
        Ok(torrent_id) => new_torrent_response(torrent_id, &info_hash).into_response(),
        Err(error) => error.into_response(),
    }
}

We could improve it. We could have a new Axum extractor that could be Optional:

ExtractLoggedInUser(logged_in_user): ExtractLoggedInUser, 
// or
ExtractAuthenticatedUser(authenticated_user): ExtractAuthenticatedUser,
pub async fn upload_torrent_handler(
    State(app_data): State<Arc<AppData>>,
    Extract(maybe_bearer_token): Extract,
    user_id: Option<ExtractAuthenticatedUser>
    multipart: Multipart,
)

That way, we could remove this from the handler:

    let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
        Ok(user_id) => user_id,
        Err(error) => return error.into_response(),
    };

In case the endpoint requires the user to be authenticated, we could use it without the optional:

pub async fn upload_torrent_handler(
    State(app_data): State<Arc<AppData>>,
    ExtractAuthenticatedUser(user_id): ExtractAuthenticatedUser
    multipart: Multipart,
)

from torrust-index.

mario-nt avatar mario-nt commented on September 10, 2024

WIP.

from torrust-index.

mario-nt avatar mario-nt commented on September 10, 2024

Tasks:

from torrust-index.

josecelano avatar josecelano commented on September 10, 2024

Relates to: torrust/torrust-index-gui#424

Hi @mario-nt. There are two functions for authorization:

  • get_user_id_from_bearer_token: this is used when a logged-in user is required
  • get_optional_logged_in_user: this is used when a logged-in used is optional.

get_user_id_from_bearer_token

    let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
        Ok(user_id) => user_id,
        Err(error) => return error.into_response(),
    };

Used in these handlers (torrent context):

  • upload_torrent_handler
  • update_torrent_info_handler
  • delete_torrent_handler

get_optional_logged_in_user

        let opt_user_id = match get_optional_logged_in_user(maybe_bearer_token, app_data.clone()).await {
            Ok(opt_user_id) => opt_user_id,
            Err(error) => return error.into_response(),
        };

Used in these handlers (torrent context):

  • download_torrent_handler
  • get_torrent_info_handler

Corner case: valid user for non-existing user

Sometimes you can get an unauthorized response. See torrust/torrust-index-gui#424.

That's because the token is valid but the user does not exist anymore. Since we are not removing users from the application, that should happen only for development when you reset the database or manually remove it.

Anyway, I think we should consider this corner case, in case we can remove users in the future.

These are the functions we used in the handlers:

pub async fn get_optional_logged_in_user(
    maybe_bearer_token: Option<BearerToken>,
    app_data: Arc<AppData>,
) -> Result<Option<UserId>, ServiceError> {
    match maybe_bearer_token {
        Some(bearer_token) => match app_data.auth.get_user_id_from_bearer_token(&Some(bearer_token)).await {
            Ok(user_id) => Ok(Some(user_id)),
            Err(error) => Err(error),
        },
        None => Ok(None),
    }
}

// ...

    pub async fn get_user_id_from_bearer_token(&self, maybe_token: &Option<BearerToken>) -> Result<UserId, ServiceError> {
        let claims = self.get_claims_from_bearer_token(maybe_token).await?;
        Ok(claims.user.user_id)
    }

For the endpoints that require a logged-in user we are not checking if the user exists in the handler. For example:

pub async fn upload_torrent_handler(
    State(app_data): State<Arc<AppData>>,
    Extract(maybe_bearer_token): Extract,
    multipart: Multipart,
) -> Response {
    let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
        Ok(user_id) => user_id,
        Err(error) => return error.into_response(),
    };

    let add_torrent_form = match build_add_torrent_request_from_payload(multipart).await {
        Ok(torrent_request) => torrent_request,
        Err(error) => return error.into_response(),
    };

    match app_data.torrent_service.add_torrent(add_torrent_form, user_id).await {
        Ok(response) => new_torrent_response(&response).into_response(),
        Err(error) => error.into_response(),
    }
}

We check whether the user exists or not in the add_torrent service.

For the endpoints that do not require a logged-in user we are checking if the user exists in the middleware. For example:

pub async fn download_torrent_handler(
    State(app_data): State<Arc<AppData>>,
    Extract(maybe_bearer_token): Extract,
    Path(info_hash): Path<InfoHashParam>,
) -> Response {
    let Ok(info_hash) = InfoHash::from_str(&info_hash.lowercase()) else {
        return errors::Request::InvalidInfoHashParam.into_response();
    };

    debug!("Downloading torrent: {:?}", info_hash.to_hex_string());

    if let Some(redirect_response) = redirect_to_download_url_using_canonical_info_hash_if_needed(&app_data, &info_hash).await {
        debug!("Redirecting to URL with canonical info-hash");
        redirect_response
    } else {
        let opt_user_id = match get_optional_logged_in_user(maybe_bearer_token, app_data.clone()).await {
            Ok(opt_user_id) => opt_user_id,
            Err(error) => return error.into_response(),
        };

        let torrent = match app_data.torrent_service.get_torrent(&info_hash, opt_user_id).await {
            Ok(torrent) => torrent,
            Err(error) => return error.into_response(),
        };

        let Ok(bytes) = parse_torrent::encode_torrent(&torrent) else {
            return ServiceError::InternalServerError.into_response();
        };

        torrent_file_response(
            bytes,
            &format!("{}.torrent", torrent.info.name),
            &torrent.canonical_info_hash_hex(),
        )
    }
}

Notice that get_optional_logged_in_userreturn an error if the user does not exist.

I think we should normalize the app behavior in these cases. I think we should:

  • When we receive a token we should always check in the handler if the user exists.
  • If the user does not exist we should return an unauthorized response. Even if the user is optional for that endpoint.
  • The frontend should invalidate the token if it receives an unauthorized response and remove it from the local storage, and do not send it anymore to the server.

@mario-nt your implementation is fine so far because you are doing that for the case when a logged-in user is required.

If you implement the other extractor to extract the optional logged-in user you have to reject the request if a token is provided but the user does not exist. I think the extractor would be very similar but you can return None or the UserId(an optional UserId). But when there is a token we should return unauthorized response even if the user is optional.

from torrust-index.

josecelano avatar josecelano commented on September 10, 2024

Tasks:

Done.

from torrust-index.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.