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

Only cache present values in the CachingAuthenticator. Fixes #947 #1082

Merged
merged 1 commit into from May 27, 2015

Conversation

Projects
None yet
4 participants
@jplock
Member

jplock commented May 27, 2015

No description provided.

@jplock jplock added this to the 0.9.0 milestone May 27, 2015

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls May 27, 2015

Coverage Status

Coverage increased (+0.08%) to 71.51% when pulling fdc29d6 on jplock:jp-gh947 into 5d0dd61 on dropwizard:master.

coveralls commented May 27, 2015

Coverage Status

Coverage increased (+0.08%) to 71.51% when pulling fdc29d6 on jplock:jp-gh947 into 5d0dd61 on dropwizard:master.

carlo-rtr added a commit that referenced this pull request May 27, 2015

Merge pull request #1082 from jplock/jp-gh947
Only cache present values in the CachingAuthenticator. Fixes #947

@carlo-rtr carlo-rtr merged commit b82f002 into dropwizard:master May 27, 2015

final Optional<P> result = underlying.authenticate(key);
// only cache present values
if (!result.isPresent()) {
throw new AuthenticationException("Failed to load security context into cache");

This comment has been minimized.

@joschi

joschi May 27, 2015

Member

Won't this exception be thrown (and propagated!) every time invalid credentials are provided?

@joschi

joschi May 27, 2015

Member

Won't this exception be thrown (and propagated!) every time invalid credentials are provided?

This comment has been minimized.

@carlo-rtr

carlo-rtr May 27, 2015

Member

You are right, I missed that.

@carlo-rtr

carlo-rtr May 27, 2015

Member

You are right, I missed that.

when(underlying.authenticate(anyString())).thenReturn(Optional.<Principal>absent());
try {
cached.authenticate("credentials");
failBecauseExceptionWasNotThrown(AuthenticationException.class);

This comment has been minimized.

@joschi

joschi May 27, 2015

Member

Invalid credentials should in fact never cause a AuthenticationException. That exception should only be used if the backend is unavailable and the credentials can't be verified.

@joschi

joschi May 27, 2015

Member

Invalid credentials should in fact never cause a AuthenticationException. That exception should only be used if the backend is unavailable and the credentials can't be verified.

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@joschi

This comment has been minimized.

Show comment
Hide comment
@joschi

joschi May 27, 2015

Member

@carlo-rtr I'm not sure that using a CacheLoader is sensible at all in this case. After all the assumption with a CacheLoader is (https://code.google.com/p/guava-libraries/wiki/CachesExplained):

Is there some sensible default function to load or compute a value associated with a key? If so, you should use a CacheLoader.

Unfortunately there is no sensible default value in our case.

The CachingAuthenticator#authenticate() should explicitly add a successfully authenticated principal to the cache instead and omit the failed ones.

Member

joschi commented May 27, 2015

@carlo-rtr I'm not sure that using a CacheLoader is sensible at all in this case. After all the assumption with a CacheLoader is (https://code.google.com/p/guava-libraries/wiki/CachesExplained):

Is there some sensible default function to load or compute a value associated with a key? If so, you should use a CacheLoader.

Unfortunately there is no sensible default value in our case.

The CachingAuthenticator#authenticate() should explicitly add a successfully authenticated principal to the cache instead and omit the failed ones.

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@carlo-rtr

carlo-rtr May 27, 2015

Member

@joschi That makes sense

Member

carlo-rtr commented May 27, 2015

@joschi That makes sense

@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock May 27, 2015

Member

So what would you like me to do with this @joschi to address the original issue as described in #947?

Member

jplock commented May 27, 2015

So what would you like me to do with this @joschi to address the original issue as described in #947?

@carlo-rtr

This comment has been minimized.

Show comment
Hide comment
@jplock

This comment has been minimized.

Show comment
Hide comment
@jplock

jplock May 27, 2015

Member

Resubmitted as #1084

Member

jplock commented May 27, 2015

Resubmitted as #1084

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment