feat: add WithPrompt option for AuthCodeURL #585
Conversation
|
@microsoft-github-policy-service agree company="TomTom" |
apps/public/public.go
Outdated
| authParams.DomainHint = o.domainHint | ||
| authParams.State = uuid.New().String() | ||
| authParams.Prompt = "select_account" | ||
| authParams.Prompt = o.prompt |
There was a problem hiding this comment.
We’re now changing the default from "select_account" to an empty string "".
Will need to test this a bit more to ensure everything behaves as expected.
There was a problem hiding this comment.
I was not aware that the "select_account" was the default value. That seems to be debatable choice in the first place. The default value on the protocol level is empty or completely absent.
In any case, changing the default from "select_account" to empty "" shall not cause catastrophic result because, again that was the default value in the specs and presumably most other libraries do that all the time. The perceived behavior will change, though, as the account selector will not pop up as often; but I think that shall be a welcome change.
There was a problem hiding this comment.
Should this be tested live ?
apps/public/public_test.go
Outdated
| return errors.New("missing query param 'prompt") | ||
| } | ||
| // if q.Get("prompt") == "" { | ||
| // return errors.New("missing query param 'prompt") |
There was a problem hiding this comment.
Commenting this out will fix the failing test, but as I mentioned earlier, changing the default value from "select_account" to "" might introduce unintended behavior.
We'll need to test this further, and once confirmed, re-enable it and update the test to assert the correct value based on the expected response.
There was a problem hiding this comment.
Maybe I should put "select_account" as a default for AcquireInteractiveOption?
|
3 participants
This PR adds WithPrompt option, so that URL can be crafted to use prompt parameter.