Skip to content

block some operations in oauth mode - #784

Merged
naphelps merged 1 commit into
open-horizon:masterfrom
wenyang-cao:fix/oauth-behavior
Jul 9, 2025
Merged

block some operations in oauth mode#784
naphelps merged 1 commit into
open-horizon:masterfrom
wenyang-cao:fix/oauth-behavior

Conversation

@wenyang-cao

@wenyang-cao wenyang-cao commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

references: #782 #783

The Put Users route must only update user records, not create new users on the system when OAuth is configured.

Passwords should not work for non-locally provided accounts. User -> Provider != Open Horizon. Affects Put and Patch User routes, and the change-password route. This is only when OAuth is configured.

Better view with: https://github.com/open-horizon/exchange-api/pull/784/files?w=1

Haven't updated changelog since I am not sure what version it would be when it is ready to be merged. I personally feel that issues #783 #782 #781 can be kept in a same version.

@naphelps @kroczi

@kroczi kroczi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me.

Comment thread src/main/resources/exchange.conf Outdated
root.enabled = ${?EXCHANGE_ROOT_ENABLED}
root.password = ${?EXCHANGE_ROOT_PW}

oauth.enabled = ${?EXCHANGE_OAUTH_ENABLED}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of version 2.130.0 this can now be removed.

Comment thread src/main/resources/reference.conf Outdated
root.enabled = true # If set to false it will not honor the root credentials
root.password = null

oauth.enabled = false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.


validateWithMsg(reqBody.getAnyProblem) {
val timestamp: java.sql.Timestamp = ApiTime.nowUTCTimestamp
val isOAuthEnabled = Configuration.getConfig.getBoolean("api.oauth.enabled")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this:

if (Configuration.getConfig.hasPath("api.authentication.oauth.provider.user_info.url") &&

@wenyang-cao
wenyang-cao force-pushed the fix/oauth-behavior branch from 131011c to 42e4f3a Compare July 3, 2025 23:18
@wenyang-cao
wenyang-cao requested a review from naphelps July 4, 2025 14:22
@wenyang-cao
wenyang-cao force-pushed the fix/oauth-behavior branch from f096f43 to 1aecb38 Compare July 4, 2025 14:25
@wenyang-cao

Copy link
Copy Markdown
Contributor Author

Need to update unit tests since the method to check whether OAuth is enabled has changed. Will do on Monday.

@wenyang-cao
wenyang-cao force-pushed the fix/oauth-behavior branch from 1aecb38 to 225ab93 Compare July 7, 2025 12:39
@wenyang-cao

wenyang-cao commented Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

@kroczi @naphelps Please review.

Some updates:

  • Check if identity provider is "Open Horizon" for patch, put routes when updating a user. According to the ticket only for external user, password and email update will be block in OAuth mode.

  • Update OAuth mode check using ```Configuration.getConfig.hasPath("api.authentication.oauth.provider.user_info.url")`` instead.

  • Update UTs accordingly

One question:
I have to use val isOAuthEnabled = Configuration.getConfig.hasPath("api.authentication.oauth.provider.user_info.url")&& !Configuration.getConfig.getString("api.authentication.oauth.provider.user_info.url").isEmpty to check if OAuth is enabled in some of the endpoints. This is because in unit tests, once configured any value, I can't set the environment variable to null, as AdminConfigRequest does not accept null values. As a workaround, I set it to an empty string "". However, it seems thatcom.typesafe.config still treats an empty string as a valid value when using hasPath, so I need to add the isEmpty check in the endpoints.

Do you have any suggestions for a cleaner or more robust way to handle this? Is there anyway to unset this environment variable in the UT?

Comment on lines -172 to +181
val oauthEnabled = Configuration.getConfig.getBoolean("api.oauth.enabled")
val oauthEnabled = Configuration.getConfig.hasPath("api.authentication.oauth.provider.user_info.url")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be removed "api.oauth.enabled" from .conf files.

Comment on lines -182 to +191
updateConfig("api.oauth.enabled", "true")
updateConfig("api.authentication.oauth.provider.user_info.url", "http://localhost:8080/mock-oauth")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "api.oauth.enabled" approach was cleaner, but if that is the new setup then ok.

@wenyang-cao
wenyang-cao force-pushed the fix/oauth-behavior branch 2 times, most recently from 76c9879 to e870043 Compare July 9, 2025 13:37
Signed-off-by: wenyang-cao <wenyang.cao@ibm.com>
@wenyang-cao
wenyang-cao force-pushed the fix/oauth-behavior branch from e870043 to dfe81b6 Compare July 9, 2025 14:12
@naphelps
naphelps merged commit 248cc50 into open-horizon:master Jul 9, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants