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

add a flag to disable authorization error logs #4076

Merged
merged 10 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .changesets/feat_geal_authz_log_errors.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
### add a flag to disable authorization error logs ([PR #4076](https://github.com/apollographql/router/pull/4076))

Authorization errors can be seen as common usage of the service when filtering fields from queries depending on the client's rights, so they might not warrant error logs to be analyzed by the router operators

THos logs can be disabled in the configuration:
Geal marked this conversation as resolved.
Show resolved Hide resolved

```yaml
authorization:
preview_directives:
log_errors: true
```
Comment on lines +7 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong feelings about this, but ould this be better as:

authorization:
  preview_directives:
    logging:
      enabled: true

?
We seem to be settling on this enabled true|false style and I wonder if it's just about enabling logging for directives or specifically errors.

also: We should specify if enabled or not by default here and in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @BrynCooke as you might have a preference in light of the telemetry recent work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends if we see further logging configuration in the future.
If we think there will be more config:

authorization:
  preview_directives:
    log:
      errors: true

We can always tackle this with a migration though, so happy with for now:

authorization:
  preview_directives:
    log_errors: true


By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/4076
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,11 @@ expression: "&schema"
"description": "enables the `@authenticated` and `@requiresScopes` directives",
"default": true,
"type": "boolean"
},
"log_errors": {
"description": "Log authorization errors",
"default": true,
"type": "boolean"
}
}
},
Expand Down
26 changes: 25 additions & 1 deletion apollo-router/src/plugins/authorization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,19 @@ pub(crate) struct Directives {
/// enables the `@authenticated` and `@requiresScopes` directives
#[serde(default = "default_enable_directives")]
enabled: bool,
/// Log authorization errors
#[serde(default = "enable_log_errors")]
log_errors: bool,
}

fn enable_log_errors() -> bool {
true
}

#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
pub(crate) struct UnauthorizedPaths {
pub(crate) paths: Vec<Path>,
pub(crate) log_errors: bool,
}

fn default_enable_directives() -> bool {
Expand Down Expand Up @@ -107,6 +120,17 @@ impl AuthorizationPlugin {
Ok(has_config.unwrap_or(true) && has_authorization_directives)
}

pub(crate) fn log_errors(configuration: &Configuration) -> bool {
let has_config = configuration
.apollo_plugins
.plugins
.iter()
.find(|(s, _)| s.as_str() == "authorization")
.and_then(|(_, v)| v.get("preview_directives").and_then(|v| v.as_object()))
.and_then(|v| v.get("log_errors").and_then(|v| v.as_bool()));
has_config.unwrap_or(true)
}
Comment on lines +126 to +135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that looks useless but could you write a small test to check that function ? Basically it's to prevent breaking change when we will remove preview_ prefix and if we decide to change the configuration shape later


pub(crate) async fn query_analysis(
query: &str,
schema: &Schema,
Expand Down Expand Up @@ -436,7 +460,7 @@ impl Plugin for AuthorizationPlugin {
fn execution_service(&self, service: execution::BoxService) -> execution::BoxService {
ServiceBuilder::new()
.map_request(|request: execution::Request| {
let filtered = !request.query_plan.query.unauthorized_paths.is_empty();
let filtered = !request.query_plan.query.unauthorized.paths.is_empty();
let needs_authenticated = request.context.contains_key(AUTHENTICATED_KEY);
let needs_requires_scopes = request.context.contains_key(REQUIRED_SCOPES_KEY);

Expand Down
8 changes: 6 additions & 2 deletions apollo-router/src/query_planner/bridge_query_planner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ use crate::json_ext::Object;
use crate::json_ext::Path;
use crate::plugins::authorization::AuthorizationPlugin;
use crate::plugins::authorization::CacheKeyMetadata;
use crate::plugins::authorization::UnauthorizedPaths;
use crate::query_planner::labeler::add_defer_labels;
use crate::services::layers::query_analysis::ParsedDocument;
use crate::services::layers::query_analysis::ParsedDocumentInner;
Expand Down Expand Up @@ -231,7 +232,10 @@ impl BridgeQueryPlanner {
fragments,
operations,
filtered_query: None,
unauthorized_paths: vec![],
unauthorized: UnauthorizedPaths {
paths: vec![],
log_errors: AuthorizationPlugin::log_errors(&self.configuration),
},
subselections,
defer_stats,
is_original: true,
Expand Down Expand Up @@ -547,7 +551,7 @@ impl BridgeQueryPlanner {
executable: new_doc.to_executable(&self.schema.api_schema().definitions),
ast: new_doc,
});
selections.unauthorized_paths = unauthorized_paths;
selections.unauthorized.paths = unauthorized_paths;
}

if selections.contains_introspection() {
Expand Down
7 changes: 4 additions & 3 deletions apollo-router/src/services/execution_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,8 +237,9 @@ impl ExecutionService {
tracing::debug_span!("format_response").in_scope(|| {
let mut paths = Vec::new();
if let Some(filtered_query) = query.filtered_query.as_ref() {
let unauthorized_paths = query.unauthorized_paths.iter().map(|path| path.to_string()).collect::<Vec<_>>();
if !unauthorized_paths.is_empty() {
if query.unauthorized.log_errors && !query.unauthorized.paths.is_empty() {
let unauthorized_paths = query.unauthorized.paths.iter().map(|path| path.to_string()).collect::<Vec<_>>();

event!(Level::ERROR, unauthorized_query_paths = ?unauthorized_paths, "Authorization error",);
}

Expand All @@ -250,7 +251,7 @@ impl ExecutionService {
variables_set,
);

for path in &query.unauthorized_paths {
for path in &query.unauthorized.paths {
response.errors.push(Error::builder()
.message("Unauthorized field or type")
.path(path.clone())
Expand Down
7 changes: 4 additions & 3 deletions apollo-router/src/spec/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use crate::json_ext::Object;
use crate::json_ext::Path;
use crate::json_ext::ResponsePathElement;
use crate::json_ext::Value;
use crate::plugins::authorization::UnauthorizedPaths;
use crate::query_planner::fetch::OperationKind;
use crate::services::layers::query_analysis::ParsedDocument;
use crate::services::layers::query_analysis::ParsedDocumentInner;
Expand Down Expand Up @@ -59,7 +60,7 @@ pub(crate) struct Query {
#[derivative(PartialEq = "ignore", Hash = "ignore")]
pub(crate) subselections: HashMap<SubSelectionKey, SubSelectionValue>,
#[derivative(PartialEq = "ignore", Hash = "ignore")]
pub(crate) unauthorized_paths: Vec<Path>,
pub(crate) unauthorized: UnauthorizedPaths,
#[derivative(PartialEq = "ignore", Hash = "ignore")]
pub(crate) filtered_query: Option<Arc<Query>>,
#[derivative(PartialEq = "ignore", Hash = "ignore")]
Expand Down Expand Up @@ -97,7 +98,7 @@ impl Query {
},
operations: Vec::new(),
subselections: HashMap::new(),
unauthorized_paths: Vec::new(),
unauthorized: UnauthorizedPaths::default(),
filtered_query: None,
defer_stats: DeferStats {
has_defer: false,
Expand Down Expand Up @@ -296,7 +297,7 @@ impl Query {
fragments,
operations,
subselections: HashMap::new(),
unauthorized_paths: Vec::new(),
unauthorized: UnauthorizedPaths::default(),
filtered_query: None,
defer_stats,
is_original: true,
Expand Down
4 changes: 2 additions & 2 deletions apollo-router/src/spec/query/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5660,7 +5660,7 @@ fn filtered_defer_fragment() {
subselections,
defer_stats,
is_original: true,
unauthorized_paths: vec![],
unauthorized: UnauthorizedPaths::default(),
validation_error: None,
};

Expand Down Expand Up @@ -5688,7 +5688,7 @@ fn filtered_defer_fragment() {
subselections,
defer_stats,
is_original: false,
unauthorized_paths: vec![],
unauthorized: UnauthorizedPaths::default(),
validation_error: None,
};

Expand Down
14 changes: 14 additions & 0 deletions docs/source/configuration/authorization.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -610,3 +610,17 @@ You can enable [query deduplication](../configuration/traffic-shaping/#query-ded
Introspection is turned off in the router by default, [as is best production practice](https://www.apollographql.com/blog/graphql/security/why-you-should-disable-graphql-introspection-in-production/). If you've chosen to [enable it](./overview/#introspection), keep in mind that **authorization directives don't affect introspection**. All fields that require authorization remain visible. However, directives applied to fields _aren't_ visible. If introspection might reveal too much information about internal types, then be sure it hasn't been enabled in your router configuration.

With introspection turned off, you can use GraphOS's [schema registry](/graphos/delivery/) to explore your supergraph schema and empower your teammates to do the same. If you want to completely remove fields from a graph rather than just preventing access (even with introspection on), consider building a [contract graph](/graphos/delivery/contracts/).

## Configuration options

The authorization plugin behaviour can be modified with various options.

### log_errors

```yaml title="router.yaml"
authorization:
preview_directives:
log_errors: true
```

By default, when part of a query is filtered by authorization, the list of filtered paths is added to the response and logged by the router. Filtering parts of a query according to the client's rights may be seen as normal operation, and not warrant investigation by platform operators, so those logs can be disabled.