Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User authentication should be done in middleware #39

Closed
mickvandijke opened this issue Jul 1, 2022 · 7 comments
Closed

User authentication should be done in middleware #39

mickvandijke opened this issue Jul 1, 2022 · 7 comments
Assignees
Labels
Easy Good for Newcomers Enhancement / Feature Request Something New
Milestone

Comments

@mickvandijke
Copy link
Member

User authentication is currently done manually in each endpoint that requires authentication. Ideally, we would have an authentication middleware instead. The authentication middleware could pass Option<User> to the endpoint handler.

@mickvandijke mickvandijke added Enhancement / Feature Request Something New Easy Good for Newcomers labels Jul 1, 2022
@josecelano
Copy link
Member

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,
)

@mario-nt
Copy link
Contributor

WIP.

mario-nt added a commit to mario-nt/torrust-index that referenced this issue Nov 30, 2023
The upload torrent handler now uses the extractor to get the user id.
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Nov 30, 2023
The upload torrent handler now uses the extractor to get the user id.
@josecelano
Copy link
Member

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.

@mario-nt mario-nt self-assigned this Nov 30, 2023
@josecelano
Copy link
Member

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?:

@cgbosse cgbosse added this to the v3.0.0-beta milestone Jan 12, 2024
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Jan 24, 2024
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Jan 24, 2024
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Jan 24, 2024
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Jan 24, 2024
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Jan 24, 2024
mario-nt added a commit to mario-nt/torrust-index that referenced this issue Jan 24, 2024
@josecelano
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Good for Newcomers Enhancement / Feature Request Something New
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants