Skip to content

Commit

Permalink
FIX-5017 Refactoring based on PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@yashmurty
yashmurty committed Aug 8, 2023
1 parent 7e3b667 commit ba55e14
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 146 deletions.
28 changes: 17 additions & 11 deletions core/src/main/java/com/linecorp/armeria/server/auth/AuthService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static java.util.Objects.requireNonNull;

import java.util.concurrent.TimeUnit;
import java.util.function.Function;

import org.slf4j.Logger;
Expand All @@ -28,14 +29,13 @@
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.metric.MoreMeters;
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.ServiceConfig;
import com.linecorp.armeria.server.ServiceRequestContext;
import com.linecorp.armeria.server.SimpleDecoratingHttpService;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Tags;
import io.micrometer.core.instrument.Timer;

/**
Expand Down Expand Up @@ -80,22 +80,27 @@ public static AuthServiceBuilder builder() {
private final Authorizer<HttpRequest> authorizer;
private final AuthSuccessHandler defaultSuccessHandler;
private final AuthFailureHandler defaultFailureHandler;
private final MeterRegistry meterRegistry;
private Timer timer;

AuthService(HttpService delegate, Authorizer<HttpRequest> authorizer,
AuthSuccessHandler defaultSuccessHandler, AuthFailureHandler defaultFailureHandler,
MeterRegistry meterRegistry) {
AuthSuccessHandler defaultSuccessHandler, AuthFailureHandler defaultFailureHandler) {
super(delegate);
this.authorizer = authorizer;
this.defaultSuccessHandler = defaultSuccessHandler;
this.defaultFailureHandler = defaultFailureHandler;
this.meterRegistry = meterRegistry;
}

@Override
public void serviceAdded(ServiceConfig cfg) throws Exception {
super.serviceAdded(cfg);
final MeterRegistry meterRegistry = cfg.server().meterRegistry();
timer = Timer.builder("armeria.server.auth")
.register(meterRegistry);
}

@Override
public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exception {
final Timer timer = MoreMeters.newTimer(meterRegistry, "authorization", Tags.empty());
final Timer.Sample sample = Timer.start();
final long startNanos = System.nanoTime();

return HttpResponse.of(AuthorizerUtil.authorizeAndSupplyHandlers(authorizer, ctx, req)
.handleAsync((result, cause) -> {
Expand All @@ -116,9 +121,10 @@ public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exc
} catch (Exception e) {
return Exceptions.throwUnsafely(e);
} finally {
// Record the time taken to authorize the request
sample.stop(timer);
}
// Record the time taken to authorize the request
final long endNanos = System.nanoTime();
timer.record(endNanos - startNanos, TimeUnit.NANOSECONDS);
}
}, ctx.eventLoop()));
}

Expand Down
16 changes: 2 additions & 14 deletions core/src/main/java/com/linecorp/armeria/server/auth/AuthServiceBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.Service;

import io.micrometer.core.instrument.MeterRegistry;

/**
* Builds a new {@link AuthService}.
*/
Expand All @@ -47,7 +45,6 @@ public final class AuthServiceBuilder {
}
return HttpResponse.of(HttpStatus.UNAUTHORIZED);
};
private MeterRegistry meterRegistry;

/**
* Creates a new instance.
Expand Down Expand Up @@ -156,26 +153,17 @@ public AuthServiceBuilder onFailure(AuthFailureHandler failureHandler) {
return this;
}

/**
* Sets the {@link MeterRegistry} that collects stats.
* If unspecified, a default one is used.
*/
public AuthServiceBuilder meterRegistry(MeterRegistry meterRegistry) {
this.meterRegistry = requireNonNull(meterRegistry, "meterRegistry");
return this;
}

/**
* Returns a newly-created {@link AuthService} based on the {@link Authorizer}s added to this builder.
*/
public AuthService build(HttpService delegate) {
return new AuthService(requireNonNull(delegate, "delegate"), authorizer(),
successHandler, failureHandler, meterRegistry);
successHandler, failureHandler);
}

private AuthService build(HttpService delegate, Authorizer<HttpRequest> authorizer) {
return new AuthService(requireNonNull(delegate, "delegate"), authorizer,
successHandler, failureHandler, meterRegistry);
successHandler, failureHandler);
}

/**
Expand Down
18 changes: 3 additions & 15 deletions ...java/com/linecorp/armeria/client/endpoint/healthcheck/HealthCheckedEndpointGroupTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,6 @@
import com.linecorp.armeria.server.logging.LoggingService;
import com.linecorp.armeria.testing.junit5.server.ServerExtension;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import io.netty.channel.EventLoopGroup;

class HealthCheckedEndpointGroupTest {
Expand Down Expand Up @@ -435,13 +433,9 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
final String password = token.password();
return completedFuture(password.equals(usernameToPassword.get(username)));
};
final MeterRegistry meterRegistry = new SimpleMeterRegistry();
sb.service(
"/basic",
ok.decorate(AuthService.builder()
.addBasicAuth(httpBasicAuthorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().addBasicAuth(httpBasicAuthorizer).newDecorator())
.decorate(LoggingService.newDecorator()));

// Auth with OAuth1a
Expand All @@ -450,21 +444,15 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
"dummy_consumer_key@#$!".equals(token.consumerKey()));
sb.service(
"/oauth1a",
ok.decorate(AuthService.builder()
.addOAuth1a(oAuth1aAuthorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().addOAuth1a(oAuth1aAuthorizer).newDecorator())
.decorate(LoggingService.newDecorator()));

// Auth with OAuth2
final Authorizer<OAuth2Token> oAuth2Authorizer = (ctx, token) ->
completedFuture("dummy_oauth2_token".equals(token.accessToken()));
sb.service(
"/oauth2",
ok.decorate(AuthService.builder()
.addOAuth2(oAuth2Authorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().addOAuth2(oAuth2Authorizer).newDecorator())
.decorate(LoggingService.newDecorator()));
}
};
Expand Down
61 changes: 22 additions & 39 deletions core/src/test/java/com/linecorp/armeria/server/auth/AuthServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import static com.linecorp.armeria.common.util.UnmodifiableFuture.completedFuture;
import static org.assertj.core.api.Assertions.assertThat;

import java.lang.reflect.Field;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.Base64.Encoder;
Expand Down Expand Up @@ -52,6 +53,7 @@
import com.linecorp.armeria.common.auth.BasicToken;
import com.linecorp.armeria.common.auth.OAuth1aToken;
import com.linecorp.armeria.common.auth.OAuth2Token;
import com.linecorp.armeria.common.metric.MoreMeters;
import com.linecorp.armeria.common.util.UnmodifiableFuture;
import com.linecorp.armeria.internal.testing.AnticipatedException;
import com.linecorp.armeria.server.AbstractHttpService;
Expand All @@ -62,6 +64,7 @@
import com.linecorp.armeria.testing.junit5.server.ServerExtension;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Timer;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import io.netty.util.AsciiString;

Expand Down Expand Up @@ -90,18 +93,14 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
return HttpResponse.of(HttpStatus.OK);
}
};
final MeterRegistry meterRegistry = new SimpleMeterRegistry();

// Auth with arbitrary authorizer
final Authorizer<HttpRequest> authorizer = (ctx, req) ->
CompletableFuture.supplyAsync(
() -> "unit test".equals(req.headers().get(AUTHORIZATION)));
sb.service(
"/",
ok.decorate(AuthService.builder()
.add(authorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().add(authorizer).newDecorator())
.decorate(LoggingService.newDecorator()));

// Auth with HTTP basic
Expand All @@ -113,16 +112,12 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
};
sb.service(
"/basic",
ok.decorate(AuthService.builder()
.addBasicAuth(httpBasicAuthorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().addBasicAuth(httpBasicAuthorizer).newDecorator())
.decorate(LoggingService.newDecorator()));
sb.service(
"/basic-custom",
ok.decorate(AuthService.builder()
.addBasicAuth(httpBasicAuthorizer, CUSTOM_TOKEN_HEADER)
.meterRegistry(meterRegistry)
.newDecorator())
.decorate(LoggingService.newDecorator()));

Expand All @@ -132,16 +127,11 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
"dummy_consumer_key@#$!".equals(token.consumerKey()));
sb.service(
"/oauth1a",
ok.decorate(AuthService.builder()
.addOAuth1a(oAuth1aAuthorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().addOAuth1a(oAuth1aAuthorizer).newDecorator())
.decorate(LoggingService.newDecorator()));
sb.service(
"/oauth1a-custom",
ok.decorate(AuthService.builder()
.addOAuth1a(oAuth1aAuthorizer, CUSTOM_TOKEN_HEADER)
.meterRegistry(meterRegistry)
ok.decorate(AuthService.builder().addOAuth1a(oAuth1aAuthorizer, CUSTOM_TOKEN_HEADER)
.newDecorator())
.decorate(LoggingService.newDecorator()));

Expand All @@ -150,18 +140,14 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
completedFuture("dummy_oauth2_token".equals(token.accessToken()));
sb.service(
"/oauth2",
ok.decorate(AuthService.builder()
.addOAuth2(oAuth2Authorizer)
.meterRegistry(meterRegistry)
.newDecorator())
ok.decorate(AuthService.builder().addOAuth2(oAuth2Authorizer).newDecorator())
.decorate(LoggingService.newDecorator()));

// Auth with OAuth2 on custom header
sb.service(
"/oauth2-custom",
ok.decorate(AuthService.builder()
.addOAuth2(oAuth2Authorizer, CUSTOM_TOKEN_HEADER)
.meterRegistry(meterRegistry)
.newDecorator())
.decorate(LoggingService.newDecorator()));

Expand All @@ -172,7 +158,6 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
ok.decorate(AuthService.builder()
.addTokenAuthorizer(INSECURE_TOKEN_EXTRACTOR,
insecureTokenAuthorizer)
.meterRegistry(meterRegistry)
.newDecorator())
.decorate(LoggingService.newDecorator()));

Expand All @@ -183,36 +168,28 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
.addBasicAuth(httpBasicAuthorizer)
.addOAuth1a(oAuth1aAuthorizer)
.addOAuth2(oAuth2Authorizer)
.meterRegistry(meterRegistry)
.build(ok)
.decorate(LoggingService.newDecorator()));

// Authorizer fails with an exception.
sb.service(
"/authorizer_exception",
ok.decorate(AuthService.builder()
.add((ctx, data) -> {
throw new AnticipatedException("bug!");
})
.meterRegistry(meterRegistry)
.newDecorator()
)
ok.decorate(AuthService.builder().add((ctx, data) -> {
throw new AnticipatedException("bug!");
}).newDecorator())
.decorate(LoggingService.newDecorator()));

// Authorizer returns a future that resolves to null.
sb.service(
"/authorizer_resolve_null",
ok.decorate(AuthService.builder()
.add((ctx, data) -> completedFuture(null))
.meterRegistry(meterRegistry)
ok.decorate(AuthService.builder().add((ctx, data) -> completedFuture(null))
.newDecorator())
.decorate(LoggingService.newDecorator()));

// Authorizer returns null.
sb.service(
"/authorizer_null",
ok.decorate(AuthService.builder().add((ctx, data) -> null)
.meterRegistry(meterRegistry)
.newDecorator())
.decorate(LoggingService.newDecorator()));

Expand All @@ -223,7 +200,6 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
.onSuccess((delegate, ctx, req) -> {
throw new AnticipatedException("bug!");
})
.meterRegistry(meterRegistry)
.newDecorator())
.decorate(LoggingService.newDecorator()));

Expand All @@ -234,7 +210,6 @@ protected HttpResponse doGet(ServiceRequestContext ctx, HttpRequest req) {
.onFailure((delegate, ctx, req, cause) -> {
throw new AnticipatedException("bug!");
})
.meterRegistry(meterRegistry)
.newDecorator())
.decorate(LoggingService.newDecorator()));
}
Expand Down Expand Up @@ -449,7 +424,6 @@ void testOnFailureException() throws Exception {

@Test
void shouldPeelRedundantAuthorizerExceptions() throws Exception {
final MeterRegistry meterRegistry = new SimpleMeterRegistry();
final AtomicReference<Throwable> causeRef = new AtomicReference<>();
final AuthService service =
AuthService.builder()
Expand All @@ -461,10 +435,19 @@ void shouldPeelRedundantAuthorizerExceptions() throws Exception {
causeRef.set(cause);
return HttpResponse.of(HttpStatus.FORBIDDEN);
})
.meterRegistry(meterRegistry)
.build((ctx, req) -> HttpResponse.of("OK"));

// Create a meter registry and set a timer to the service.
final MeterRegistry meterRegistry = new SimpleMeterRegistry();
final Field timerField = AuthService.class.getDeclaredField("timer");
timerField.setAccessible(true);
final Timer timer = Timer.builder("armeria.server.auth").register(meterRegistry);
timerField.set(service, timer);
assertThat(MoreMeters.measureAll(meterRegistry)).containsEntry("armeria.server.auth#count", 0.0);

final ServiceRequestContext ctx = ServiceRequestContext.of(HttpRequest.of(HttpMethod.GET, "/"));
final HttpResponse response = service.serve(ctx, ctx.request());

assertThat(response.aggregate().join().status()).isEqualTo(HttpStatus.FORBIDDEN);
assertThat(causeRef.get()).isInstanceOf(AnticipatedException.class);
}
Expand Down

0 comments on commit ba55e14

Please sign in to comment.