chore: adjust auth server endpoints to match server values #20304
Conversation
spartacus
|
||||||||||||||||||||||||||||||
| Project |
spartacus
|
| Branch Review |
feature/CXSPA-9984
|
| Run status |
|
| Run duration | 12m 28s |
| Commit |
|
| Committer | Eric Polesky |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
3
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
245
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Merge Checks Failed |
Merge Checks Failed |
01de987 to
43c8416
Compare
43c8416 to
366b841
Compare
366b841 to
eb78987
Compare
Merge Checks Failed |
| @@ -52,9 +51,10 @@ export const defaultAuthConfig: AuthConfig = { | |||
| customTokenParameters: ['token_type'], | |||
| strictDiscoveryDocumentValidation: false, | |||
| skipIssuerCheck: true, | |||
| disablePKCE: true, | |||
| disablePKCE: false, | |||
There was a problem hiding this comment.
I think changes in default config values can lead to unexpected behaviour.
Seems to be a breaking change, isn't it?
There was a problem hiding this comment.
This default object is internal, so changes will not affect customers. It is not exported out of the module and is only used when applied in the UserAuthModule. Even this is not strictly true anymore now that we use a temporary provider factory to set the default configuration, but we will eventually return to using the object directly.
The changes to the default object are to reduce ambiguity when the JDK17 authorization server support is dropped.
The default object is set to the "future" JDK21 default, and the provider factory converts the object to match the "legacy" version.
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.
If, in customer's application, customer provided authConfig with only this values (just a dummy example):
provideConfig(<AuthConfig>{
authentication: {
clientId: 'spartacus-spa',
OAuthLibConfig: {
strictDiscoveryDocumentValidation: true,
},
},
}),
Customer provided only these values, because the rest values from the default config are suitable for him,
in the result his config will looks like this:
const oAuthConfig = {
OAuthLibConfig: {
clearHashAfterLogin: false,
customTokenParameters: ['token_type'],
disablePKCE: true,
oidc: false,
scope: '',
skipIssuerCheck: true,
strictDiscoveryDocumentValidation: true, // From CUSTOM config
},
clientId: 'spartacus-spa', // From CUSTOM config
client_id: 'mobile_android',
client_secret: 'secret',
loginUrl: '/oauth/authorize',
revokeEndpoint: '/oauth/revoke',
tokenEndpoint: '/oauth/token',
};
Are we sure that changes in the default config that we are doing for 100% cannot affect his application?
Can customer's app broke because of not expected resposeType: code for example?
There was a problem hiding this comment.
The factory emits the default configuration exactly as it did before this change. This is proven by the spec test that was added for the factory. The normal merging rules applying, so the customer's property will be merged into the configuration with a higher priority.
The only thing that could break something is the new property sendAuthHeaderOnRevoke conflicting with the customer's custom properties.
There was a problem hiding this comment.
new property sendAuthHeaderOnRevoke and removal of responseType; - delete config.authentication.OAuthLibConfig.responseType, and changing value of disablePKCE, no?
I can miss the context here, talking more in general about configs, than about this exact case. So if you think that it is safe change - feel free to ignore me
| /** | ||
| * Determine if the `Authorization` header should be sent with revocation requests. | ||
| */ | ||
| enableTokenRevocationInterceptor?: boolean; |
There was a problem hiding this comment.
just nitpicks:
- the name of this config describes the implementation (e.g. mentions the concept of interceptor).
Other config properties are related to the characteristics of the auth standard/process itself.
What do you think about renaming the config property, to rather describe the effect on the auth process?
e.g. shouldSendAuthHeaderOnRevoke
(If only you agree, then let's rename also analogical places, like the method in AuthConfigService)
- If this config property is needed only for JDK21, for clarity let's mention this fact also in this the JSDoc of this config property
Uh oh!
There was an error while loading. Please reload this page.