Add first order broker parameters #7348
Conversation
konstantin-msft
commented
Sep 27, 2024
•
konstantin-msft
commented
- Add first order embedded client id parameter.
- Instrument embedded client id and embedded redirect uri.
- Default redirect uri to the current page in auth config.
konstantin-msft
requested review from
sameerag,
tnorling,
hectormmg,
jo-arroyo,
peterzenz,
Robbie-Microsoft,
lalimasharda and
shylasummers
as code owners
konstantin-msft
force-pushed
the
instrument_broker_params
branch
from
1feef7c
to
ff65f71
Compare
github-actions
bot
added
msal-browser
konstantin-msft
force-pushed
the
instrument_broker_params
branch
3 times, most recently
from
14cec29
to
feb1025
Compare
konstantin-msft
force-pushed
the
instrument_broker_params
branch
from
7424edf
to
a2e6af9
Compare
| @@ -274,6 +275,7 @@ function buildAuthOptions(authOptions: AuthOptions): Required<AuthOptions> { | |||
| azureCloudOptions: DEFAULT_AZURE_CLOUD_OPTIONS, | |||
| skipAuthorityMetadataCache: false, | |||
| instanceAware: false, | |||
| redirectUri: "https://localhost", | |||
There was a problem hiding this comment.
We should not be using or defaulting to localhost here - suggest making redirectUri required on the AuthOptions object to avoid
There was a problem hiding this comment.
Made redirectUri mandatory in msal-common and update msal-browser and msal-node accordingly.
konstantin-msft
requested review from
bgavrilMS,
trwalke,
pmaytak and
gladjohn
as code owners
konstantin-msft
force-pushed
the
instrument_broker_params
branch
from
e66460d
to
abe3cb8
Compare
konstantin-msft
force-pushed
the
instrument_broker_params
branch
2 times, most recently
from
b3e38fa
to
db921b7
Compare
- Instrument embedded client id.
konstantin-msft
force-pushed
the
instrument_broker_params
branch
from
db921b7
to
0f9eb68
Compare
| // generate the correlationId if not set by the user and add | ||
| const correlationId = | ||
| request.correlationId || | ||
| this.config.cryptoInterface.createNewGuid(); |
There was a problem hiding this comment.
Is correlationId not a required param on request? CorrelationId should have long been generated by the time we reach this point.
There was a problem hiding this comment.
It should be a required param as CommonAuthorizationUrlRequest extends BaseAuthRequest. I moved this piece from here. Not sure if this.config.cryptoInterface.createNewGuid() was added on purpose or just copy-pasted.
There was a problem hiding this comment.
I see. This is likely leftover from a time when correlationId was still optional. This would be a good opportunity to clean it up if we're moving it.
