fix: misleading error after OAuth is disabled#299
Conversation
In the edge case where the user disables the OAuth authentication while the authorization page is opened on the server, and then user hits Allow, the URI executes. But the plugin tries to initialize the rest api client and CLI without checking that OAuth is no longer allowed. This raises a misleading error. The fix is somewhat simple, before exchanging the authorization code with an acces token we check if OAuth is still enabled and if not we fail fast with a proper error message. For cases where Cancel is selected by the user on the authorization page we allow the flow to go as usual in Toolbox. - raised coder/coder#24912 while investigating this issue - resolves https://linear.app/codercom/issue/DEVEX-222
fioan89
requested review from
code-asher,
jeremyruppel,
matifali and
zenithwolf1000
| "OAuth2 server did not respond back with an access token" | ||
| ) | ||
|
|
||
| // before going forward we check to make sure OAuth is not disabled in the meantime |
There was a problem hiding this comment.
What does disabling mean here? Disabled on the server or the user has not enabled it in the plugin settings?
There was a problem hiding this comment.
It means disabled in the plugin.
|
|
||
| // before going forward we check to make sure OAuth is not disabled in the meantime | ||
| if (!context.settingsStore.preferOAuth2IfAvailable) { | ||
| context.logAndShowError( |
There was a problem hiding this comment.
other cases in this function return context.logAndShowError(), but here we have the log and then an empty return. should we care about the return? or perhaps it's a good idea to be consistent about the return?
There was a problem hiding this comment.
In the other cases I used the elvis operator and a little trick from Kotlin that allows return statements to exit early if an expression does not match a certain value (in the particular cases if some variables have non null values). logAndShowError is a "void" function. Technically void functions in Kotlin always return a Unit type. And then Unit allows the trick of writing return Unit which causes the enclosing function to exit early.
In this case particular case I can't do that trick.
4 participants
In the edge case where the user disables the OAuth authentication while the authorization page is opened on the server, and then user hits Allow, the URI executes. But the plugin tries to initialize the rest api client and CLI without checking that OAuth is no longer allowed. This raises a misleading error.
The fix is somewhat simple, before exchanging the authorization code with an acces token we check if OAuth is still enabled and if not we fail fast with a proper error message.
For cases where Cancel is selected by the user on the authorization page we allow the flow to go as usual in Toolbox.