Skip to content

chore: Bust OAuth2 client cache for spring boot 3.3 #36660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nidhi-nair merged 1 commit into from
Oct 2, 2024
Merged

chore: Bust OAuth2 client cache for spring boot 3.3 #36660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/RedisConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ public byte[] serialize(Object t) {
boolean allValuesAreClientDTOs = true;
for (final LoginSource loginSource : LoginSource.oauthSources) {
final Object value = data.get(loginSource.name().toLowerCase());
if (value != null && !(value instanceof OAuth2AuthorizedClientDTO)) {
if (value != null && !(value instanceof OAuth2AuthorizedClient)) {
allValuesAreClientDTOs = false;
break;
}
Expand Down
38 changes: 38 additions & 0 deletions ...rc/main/java/com/appsmith/server/migrations/db/ce/Migration063CacheBustSpringBoot3_3.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package com.appsmith.server.migrations.db.ce;

import io.mongock.api.annotations.ChangeUnit;
import io.mongock.api.annotations.Execution;
import io.mongock.api.annotations.RollbackExecution;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.data.redis.core.ReactiveRedisOperations;
import org.springframework.data.redis.core.script.RedisScript;
import reactor.core.publisher.Flux;

@RequiredArgsConstructor
@Slf4j
@ChangeUnit(order = "063", id = "reset_session_oauth2_spring_3_3")
public class Migration063CacheBustSpringBoot3_3 {

private final ReactiveRedisOperations<String, String> reactiveRedisOperations;

@RollbackExecution
public void rollbackExecution() {}

@Execution
public void execute() {
doClearRedisOAuth2AuthClientKeys(reactiveRedisOperations);
}
Comment on lines +23 to +25
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add exception handling in the execute method

It's good practice to handle exceptions to make your code more robust. In the execute() method, consider adding a try-catch block to catch any exceptions that may occur during the Redis operations. This way, you can log the error and handle it appropriately.

public void execute() {
    try {
        doClearRedisOAuth2AuthClientKeys(reactiveRedisOperations);
    } catch (Exception e) {
        log.error("An error occurred while clearing OAuth2 client keys from Redis", e);
        // Optionally, handle the exception or rethrow it
    }
}


public static void doClearRedisOAuth2AuthClientKeys(
ReactiveRedisOperations<String, String> reactiveRedisOperations) {
final String authorizedClientsKey =
"sessionAttr:org.springframework.security.oauth2.client.web.server.WebSessionServerOAuth2AuthorizedClientRepository.AUTHORIZED_CLIENTS";
final String script =
"for _,k in ipairs(redis.call('keys','spring:session:sessions:*')) do local fieldExists = redis.call('hexists', k, '"
+ authorizedClientsKey + "'); if fieldExists == 1 then redis.call('del', k) end end";
Comment on lines +31 to +33
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using KEYS command in Redis scripts

Remember that using the KEYS command in Redis can be problematic in production environments. It can block the Redis server if there are many keys, leading to performance issues.

Instead, consider using the SCAN command, which is non-blocking and more suitable for iterating over keys in a large keyspace. Here's an example of how you can modify the script:

final String script =
    "local cursor = '0' " +
    "repeat " +
    "  local result = redis.call('SCAN', cursor, 'MATCH', 'spring:session:sessions:*') " +
    "  cursor = result[1] " +
    "  local keys = result[2] " +
    "  for _,k in ipairs(keys) do " +
    "    local fieldExists = redis.call('hexists', k, '" + authorizedClientsKey + "') " +
    "    if fieldExists == 1 then " +
    "      redis.call('del', k) " +
    "    end " +
    "  end " +
    "until cursor == '0'";

final Flux<Object> flushdb = reactiveRedisOperations.execute(RedisScript.of(script));

flushdb.blockLast();
}
Comment on lines +27 to +37
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid blocking calls in reactive streams

Dear student, it's important to remember that using blocking calls like blockLast() in a reactive pipeline can undermine the benefits of reactive programming. Blocking operations can lead to performance bottlenecks and thread starvation.

To maintain a non-blocking, asynchronous flow, consider refactoring the code to use reactive operators and subscribing to the sequence without blocking. Here's how you might modify the code:

reactiveRedisOperations.execute(RedisScript.of(script))
    .doOnError(error -> log.error("Error executing Redis script", error))
    .doOnComplete(() -> log.info("Successfully cleared OAuth2 client keys from Redis"))
    .subscribe();

}