-
Notifications
You must be signed in to change notification settings - Fork 6k
Replace deprecated #check calls with #authorize #16965
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright 2002-2023 the original author or authors. | ||
* Copyright 2002-2025 the original author or authors. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
|
@@ -74,6 +74,7 @@ void setup() { | |
void verifyWhenDefaultsThenObserves() { | ||
given(this.handler.supportsContext(any())).willReturn(true); | ||
given(this.authorizationManager.check(any(), any())).willReturn(this.grant); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's leave unit tests as-is so that we know we are still backward compatible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll change it to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); | ||
this.tested.verify(this.token, this.object); | ||
ArgumentCaptor<Observation.Context> captor = ArgumentCaptor.forClass(Observation.Context.class); | ||
verify(this.handler).onStart(captor.capture()); | ||
|
@@ -92,6 +93,7 @@ void verifyWhenErrorsThenObserves() { | |
this.tested.setMessageSource(source); | ||
given(this.handler.supportsContext(any())).willReturn(true); | ||
given(this.authorizationManager.check(any(), any())).willReturn(this.deny); | ||
given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); | ||
given(source.getMessage(eq("AbstractAccessDecisionManager.accessDenied"), any(), any(), any())) | ||
.willReturn("accessDenied"); | ||
assertThatExceptionOfType(AccessDeniedException.class) | ||
|
@@ -116,6 +118,7 @@ void verifyWhenLooksUpAuthenticationThenObserves() { | |
((Supplier<Authentication>) invocation.getArgument(0)).get(); | ||
return this.grant; | ||
}); | ||
given(this.authorizationManager.authorize(any(), any())).willCallRealMethod(); | ||
this.tested.verify(this.token, this.object); | ||
ArgumentCaptor<Observation.Context> captor = ArgumentCaptor.forClass(Observation.Context.class); | ||
verify(this.handler).onStart(captor.capture()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to leave the
check
method as is. There's no point in calling theauthorize
method from a deprecated method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we are moving away from
check
method it's reasonable to stop calling user-definedcheck
implementations in our code, instead it's good to indicate to the user that they should fully migrate to usingauthorize
method if they have mix of both, the same approach is used inAuthorizationManagers#anyOf
for example.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jzheaux what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points from both of you.
Given that this is work that can be done in advance of the next major, I think that it is worthwhile. In this way, the only thing that remains to be done when the time comes is for
check
to be removed.