refactor(auth): remove legacy auth & ADFS support #1150
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BugGambit
force-pushed
the
f1/remove_legacy_auth_adfs
branch
2 times, most recently
from
65668a8
to
b79184e
Compare
cemreyavuz
reviewed
Sep 18, 2024
| if (isBrowser() && !isUsingSSL() && !options.noAuthMode) { | ||
| console.warn( | ||
| 'You should use SSL (https) when you login with OAuth since CDF only allows redirecting back to an HTTPS site' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Wouldn't this warning be logged even if we're not using legacy auth?
There was a problem hiding this comment.
Yes, but it was placed there due to legacy auth. No reason to restrict to https anymore.
The https restriction came from legacy auth.
Comment on lines
+78
to
+84
| if (!options) { | ||
| throw Error('`CogniteClient` is missing parameter `options`'); | ||
| } | ||
|
|
||
| if (!isString(options.appId)) { | ||
| throw Error('options.appId is required and must be of type string'); | ||
| } | ||
|
|
||
| if (!isString(options.project)) { | ||
| throw Error('options.project is required and must be of type string'); | ||
| } |
There was a problem hiding this comment.
Feels like we can still use verifyOptionsRequiredFields in some form to manage this easier - but i'm also not sure how many new "required" things we would be adding to options.
There was a problem hiding this comment.
The verifyOptionsRequiredFields did a bit more and mostly related to auth options.
Keeping this as required for now as it's aligned with version 9.
cemreyavuz
approved these changes
Sep 18, 2024
BugGambit
force-pushed
the
f1/remove_legacy_auth_adfs
branch
from
b79184e
to
98d09bc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of the v10 release we would like to remove legacy auth (the API is gone so legacy auth doesn't work) and ADFS support. ADFS is only used by Aramco and Cntxt and we would rather transition them to CogIdP.
Jira: