Migrate from Keycloak to internal OAuth2 authorization server#1
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved Keycloak artifacts and DB bootstrap scripts; added a multi-module Gradle project (core, console, iam, migration) with Spring Boot services, Liquibase migrations, distributed-lock and password-encoder auto-configurations, CI/formatting tooling, native/JVM Dockerfiles, and VS Code workspace settings. Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap as IAM Bootstrap
participant Lock as LockRegistry
participant Encoder as PasswordEncoder
participant Repo as RegisteredClientRepository
participant DB as Database (Postgres)
Note over Bootstrap,Repo: Startup client registration under distributed lock
Bootstrap->>Lock: acquireLock("iam.migration", 30s)
Lock-->>Bootstrap: lock acquired
Bootstrap->>Bootstrap: iterate configured clients
Bootstrap->>Encoder: encode(client.secret) [if present]
Encoder-->>Bootstrap: encodedSecret
Bootstrap->>Repo: save(RegisteredClient(..., encodedSecret, scopes, grants))
Repo->>DB: INSERT/UPDATE oauth2_registered_client
DB-->>Repo: OK
Bootstrap->>Lock: releaseLock
Lock-->>Bootstrap: lock released
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
phongphongg
force-pushed
the
feat/authorization-server
branch
7 times, most recently
from
9ed03c3 to
c69e6d3
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
from
c69e6d3 to
4cb3846
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
from
4cb3846 to
a3d7e71
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
7 times, most recently
from
555cead to
37af8e4
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
3 times, most recently
from
765fbf5 to
35aba1d
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
from
35aba1d to
bc68cc0
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
2 times, most recently
from
38710e2 to
67b4f63
Compare
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
phongphongg
force-pushed
the
feat/authorization-server
branch
from
fddb676 to
a5b5468
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
phongphongg
force-pushed
the
feat/authorization-server
branch
from
d4e2614 to
0b1495d
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
from
0b1495d to
26f97a8
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
from
26f97a8 to
cbdc5cd
Compare
phongphongg
changed the title
coderabbitai
Bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
.github/scripts/sqruff_to_sarif.py (2)
9-9: Use the canonical SARIF 2.1.0 schema URL.The
oasis-tcs/sarif-specrepository's default branch ismain, notmaster. The link still resolves today via GitHub's redirect, but pinning to a non-default branch is fragile. Prefer the OASIS-hosted canonical URL or themainbranch.-SARIF_SCHEMA = "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json" +SARIF_SCHEMA = "https://json.schemastore.org/sarif-2.1.0.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/sqruff_to_sarif.py at line 9, The SARIF_SCHEMA constant points to a fragile GitHub URL using the non-default 'master' branch; update the SARIF_SCHEMA value to the canonical OASIS-hosted SARIF 2.1.0 schema URL (or at least use the repository 'main' branch) so the schema reference is stable; locate the SARIF_SCHEMA constant in .github/scripts/sqruff_to_sarif.py and replace the existing URL with the canonical OASIS URL for SARIF 2.1.0.
85-96: Add error handling so a sqruff failure doesn't break the SARIF upload step.The workflow runs this script with
if: always()precisely so SARIF is uploaded even when sqruff fails, butjson.loads(input_path.read_text())will raise (FileNotFoundError / JSONDecodeError) if the previous step crashed mid-write. That bubbles up and prevents the upload step from running. Consider tolerating missing/empty input by writing an empty-but-valid SARIF document.♻️ Suggested fallback
def main() -> None: tmp = Path(os.environ["RUNNER_TEMP"]) input_path = tmp / "sqruff-raw.json" output_path = tmp / "sqruff.sarif" - data = json.loads(input_path.read_text()) + try: + raw = input_path.read_text() if input_path.exists() else "" + data = json.loads(raw) if raw.strip() else {} + except json.JSONDecodeError as e: + print(f"⚠️ Could not parse {input_path}: {e}; emitting empty SARIF.") + data = {} output_path.write_text(json.dumps(build_sarif(data), indent=2))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/scripts/sqruff_to_sarif.py around lines 85 - 96, The main function currently reads and parses sqruff raw JSON directly which can raise FileNotFoundError or JSONDecodeError and abort the SARIF upload; wrap the read/parse around a try/except that catches FileNotFoundError and json.JSONDecodeError, and on error set data to an empty mapping (e.g. {}) so build_sarif(data) still produces a valid empty SARIF; ensure you still write output_path and keep references to input_path, output_path, main, and build_sarif when making the change so total = sum(len(v) for v in data.values()) and the final prints behave correctly.iam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.java (1)
53-64:defaultSecurityFilterChainlacks CSRF and logout configuration; OAuth2 login comment is dead code.Two minor points on the secondary chain:
- The commented-out
.oauth2Login(...)(Line 61) is dead code — either delete it or convert to a// TODO:with a tracking issue, otherwise it lingers as scaffolding.- With
formLogin(Customizer.withDefaults())and authenticatedanyRequest(), you get default CSRF protection (good), but no.logout(...)— users authenticated via form login will not have a clean session-termination endpoint that also clears authorization-server cookies. Worth adding.logout(Customizer.withDefaults())(or wiring anOidcLogoutConfigurer) before this ships.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@iam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.java` around lines 53 - 64, The SecurityFilterChain bean defaultSecurityFilterChain is missing explicit logout handling and contains dead commented oauth2Login code; remove the commented-out ".oauth2Login(...)" or convert it to a "// TODO: ..." with a tracking issue, and add a logout configuration to the chain (e.g., call .logout(Customizer.withDefaults()) or wire an OidcLogoutConfigurer) alongside the existing .formLogin(Customizer.withDefaults()) so form-authenticated users get a proper session/authorization-cookie termination endpoint.core/src/main/java/com/workastra/core/infrastructure/JdbcLockConfiguration.java (1)
16-28: Consider gating the conditionals on the interface types.
@ConditionalOnMissingBeanwithout arguments matches by the method's return type (DefaultLockRepository/JdbcLockRegistry). If a downstream module ever publishes its ownLockRepositoryorLockRegistryimplementation, this auto-config would still register the JDBC ones, causing duplicateLockRegistry<DistributedLock>beans and breaking the constructor injection inBootstrap. Gating on the interfaces makes the override semantics correct.♻️ Suggested refactor
`@Bean` - `@ConditionalOnMissingBean` + `@ConditionalOnMissingBean`(LockRepository.class) `@ConditionalOnBean`(DataSource.class) DefaultLockRepository lockRepository(DataSource dataSource) { return new DefaultLockRepository(dataSource); } `@Bean` - `@ConditionalOnMissingBean` + `@ConditionalOnMissingBean`(LockRegistry.class) `@ConditionalOnBean`(LockRepository.class) JdbcLockRegistry jdbcLockRegistry(LockRepository lockRepository) { return new JdbcLockRegistry(lockRepository); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/workastra/core/infrastructure/JdbcLockConfiguration.java` around lines 16 - 28, The conditional annotations currently use no-arg `@ConditionalOnMissingBean` (which checks the concrete return types DefaultLockRepository and JdbcLockRegistry), so downstream beans implementing the LockRepository/LockRegistry interfaces won't prevent these auto-configurations; update the two bean definitions (methods lockRepository and jdbcLockRegistry) to use `@ConditionalOnMissingBean` with the interface types (e.g., `@ConditionalOnMissingBean`(LockRepository.class) for lockRepository and `@ConditionalOnMissingBean`(LockRegistry.class) for jdbcLockRegistry) while keeping the existing `@ConditionalOnBean` checks and return types (DefaultLockRepository, JdbcLockRegistry) unchanged so override semantics rely on the interface contracts.iam/src/main/java/com/workastra/iam/configuration/UserInfoMapper.java (1)
43-43: Improve error fidelity when user lookup fails or principal name is malformed.Two minor concerns on this single line:
UUID.fromString(authentication.getName())will throwIllegalArgumentExceptionif the JWT subject ever isn't a UUID (e.g., principal mapping changes upstream). It surfaces as an opaque 500 instead of an OAuth2 protocol error.Optional.orElseThrow()raises a bareNoSuchElementExceptionwithout a message, making it hard to distinguish "user deleted after token issuance" in logs.♻️ Suggested refactor
- User user = this.userRepository.findById(UUID.fromString(authentication.getName())).orElseThrow(); + UUID userId; + try { + userId = UUID.fromString(authentication.getName()); + } catch (IllegalArgumentException ex) { + throw new IllegalStateException("JWT subject is not a valid UUID: " + authentication.getName(), ex); + } + User user = this.userRepository.findById(userId) + .orElseThrow(() -> new IllegalStateException("No user found for id: " + userId));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@iam/src/main/java/com/workastra/iam/configuration/UserInfoMapper.java` at line 43, Replace the single-line lookup in UserInfoMapper that uses UUID.fromString(authentication.getName()) and orElseThrow() with explicit handling: first attempt to parse the principal to a UUID inside a try/catch and on IllegalArgumentException throw an OAuth2AuthenticationException (or another OAuth2-compatible exception) with an "invalid_token" error and a clear message that the principal is not a UUID; second, change the repository lookup from userRepository.findById(...).orElseThrow() to orElseThrow(() -> new OAuth2AuthenticationException(new OAuth2Error("invalid_token", "User not found for id: <parsed-UUID-or-principal>", null))) so the missing-user case produces a descriptive OAuth2 error; reference UUID.fromString, userRepository.findById, UserInfoMapper and use OAuth2AuthenticationException/OAuth2Error (or equivalent OAuth2 exception types in the project) to surface protocol-safe, informative errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.docker/database/01-create-workastra.sql:
- Line 3: The SQL currently hardcodes the password for user workastra_user;
change the bootstrapping so the password is provided from an environment
variable (e.g., read ${WORKASTRA_DB_PASSWORD} in an init-shell script and pass
it into the CREATE USER statement) instead of embedding 'workastra_password',
and remove the trailing spaces before the semicolons to tidy the SQL; ensure the
init script validates the env var is set and fails fast with a clear message if
not.
- Around line 1-5: The CREATE/GRANT statements create workastra_db and
workastra_user but do not allow workastra_user to create objects in the public
schema, so Flyway (when run as workastra_user) will fail to create the schema
history table; fix by either making workastra_user the owner of workastra_db (so
it has schema CREATE rights) or explicitly granting schema-level privileges on
public to workastra_user (e.g., connect to workastra_db and GRANT CREATE, USAGE
ON SCHEMA public TO workastra_user), and ensure these changes are applied after
the CREATE DATABASE and CREATE USER steps so workastra_user can create
schemas/objects.
In `@compose.yaml`:
- Around line 11-14: The compose service is binding Postgres to all interfaces
via the ports entry "- 5432:5432" and still references the "backend" network
even though it's unused; update the ports mapping to a loopback-only binding
(e.g., change "- 5432:5432" to "127.0.0.1:5432:5432" or remove the host mapping
entirely for dev), and either remove the "backend" network reference from the
database service and delete the top-level networks: backend block, or leave the
network and add a clear comment explaining why it must remain; also ensure any
hardcoded credentials referenced nearby (e.g., superadmin/password) are noted
for separate cleanup.
- Around line 8-10: The compose YAML mounts the named volume at
/var/lib/postgresql/18/data which doesn't match the postgres:18 PGDATA location,
so PostgreSQL will write into the container's internal
/var/lib/postgresql/18/docker and your named volume will stay empty; update the
volumes entry for database_data to either mount the parent directory (use
database_data:/var/lib/postgresql) so the image can create the versioned subdir,
or mount directly to the PGDATA path
(database_data:/var/lib/postgresql/18/docker) so data persists across container
recreation.
In
`@iam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.java`:
- Around line 29-51: Add JWT resource-server support to the authorization-server
filter chain so Bearer access tokens become JwtAuthenticationToken for the OIDC
/userinfo flow: update the SecurityFilterChain method
authorizationServerSecurityFilterChain to call oauth2ResourceServer(jwt) on the
same HttpSecurity instance used in oauth2AuthorizationServer and configure JWT
decoding (ensure a JwtDecoder bean derived from your JWKSource/OIDC provider is
available). This change ensures the userInfoMapper (used in
oidc.userInfoEndpoint with OidcUserInfoAuthenticationContext) will receive
JwtAuthenticationToken instances instead of causing the IllegalStateException in
the default branch.
In `@iam/src/main/java/com/workastra/iam/entity/User.java`:
- Around line 129-147: The four UserDetails methods (isAccountNonExpired,
isAccountNonLocked, isCredentialsNonExpired, isEnabled) currently unbox Boolean
wrapper fields (accountNonExpired, accountNonLocked, credentialsNonExpired,
enabled) causing possible NPEs; change those fields (and emailVerified) from
Boolean to primitive boolean (preferred) so the methods can safely return
primitives, update any builder/defaults to provide sensible defaults consistent
with the DB NOT NULL constraint, and remove any unboxing in the corresponding
getters; alternatively, if you must keep wrappers, replace each method body with
a defensive Boolean.TRUE.equals(field) check (e.g.,
Boolean.TRUE.equals(accountNonExpired)) for all four methods and similarly for
emailVerified.
In `@migration/src/main/resources/db/migration/postgresql/V0_1_0__2026-04-23` -
Hello World.sql:
- Around line 97-114: The User.updatedAt mapping is marked `@Column`(updatable =
false) which blocks Hibernate from refreshing the trigger-managed timestamp
after writes; update the mapping for the updatedAt field in the User entity
(symbol: User.updatedAt) to either annotate it with `@Generated`(event = {INSERT,
UPDATE}) so Hibernate will re-read the DB value after insert/update, or remove
updatable = false so JPA can write the timestamp itself (relying on the DB
trigger's WHEN (OLD IS DISTINCT FROM NEW) to avoid spurious writes); after
changing the mapping, verify code paths that call
UserInfoMapper.buildUserInfoClaims() still behave correctly without requiring an
explicit findById reload.
---
Nitpick comments:
In @.github/scripts/sqruff_to_sarif.py:
- Line 9: The SARIF_SCHEMA constant points to a fragile GitHub URL using the
non-default 'master' branch; update the SARIF_SCHEMA value to the canonical
OASIS-hosted SARIF 2.1.0 schema URL (or at least use the repository 'main'
branch) so the schema reference is stable; locate the SARIF_SCHEMA constant in
.github/scripts/sqruff_to_sarif.py and replace the existing URL with the
canonical OASIS URL for SARIF 2.1.0.
- Around line 85-96: The main function currently reads and parses sqruff raw
JSON directly which can raise FileNotFoundError or JSONDecodeError and abort the
SARIF upload; wrap the read/parse around a try/except that catches
FileNotFoundError and json.JSONDecodeError, and on error set data to an empty
mapping (e.g. {}) so build_sarif(data) still produces a valid empty SARIF;
ensure you still write output_path and keep references to input_path,
output_path, main, and build_sarif when making the change so total = sum(len(v)
for v in data.values()) and the final prints behave correctly.
In
`@core/src/main/java/com/workastra/core/infrastructure/JdbcLockConfiguration.java`:
- Around line 16-28: The conditional annotations currently use no-arg
`@ConditionalOnMissingBean` (which checks the concrete return types
DefaultLockRepository and JdbcLockRegistry), so downstream beans implementing
the LockRepository/LockRegistry interfaces won't prevent these
auto-configurations; update the two bean definitions (methods lockRepository and
jdbcLockRegistry) to use `@ConditionalOnMissingBean` with the interface types
(e.g., `@ConditionalOnMissingBean`(LockRepository.class) for lockRepository and
`@ConditionalOnMissingBean`(LockRegistry.class) for jdbcLockRegistry) while
keeping the existing `@ConditionalOnBean` checks and return types
(DefaultLockRepository, JdbcLockRegistry) unchanged so override semantics rely
on the interface contracts.
In
`@iam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.java`:
- Around line 53-64: The SecurityFilterChain bean defaultSecurityFilterChain is
missing explicit logout handling and contains dead commented oauth2Login code;
remove the commented-out ".oauth2Login(...)" or convert it to a "// TODO: ..."
with a tracking issue, and add a logout configuration to the chain (e.g., call
.logout(Customizer.withDefaults()) or wire an OidcLogoutConfigurer) alongside
the existing .formLogin(Customizer.withDefaults()) so form-authenticated users
get a proper session/authorization-cookie termination endpoint.
In `@iam/src/main/java/com/workastra/iam/configuration/UserInfoMapper.java`:
- Line 43: Replace the single-line lookup in UserInfoMapper that uses
UUID.fromString(authentication.getName()) and orElseThrow() with explicit
handling: first attempt to parse the principal to a UUID inside a try/catch and
on IllegalArgumentException throw an OAuth2AuthenticationException (or another
OAuth2-compatible exception) with an "invalid_token" error and a clear message
that the principal is not a UUID; second, change the repository lookup from
userRepository.findById(...).orElseThrow() to orElseThrow(() -> new
OAuth2AuthenticationException(new OAuth2Error("invalid_token", "User not found
for id: <parsed-UUID-or-principal>", null))) so the missing-user case produces a
descriptive OAuth2 error; reference UUID.fromString, userRepository.findById,
UserInfoMapper and use OAuth2AuthenticationException/OAuth2Error (or equivalent
OAuth2 exception types in the project) to surface protocol-safe, informative
errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e8cd162-936b-430c-905b-5c6c7c8526ab
📒 Files selected for processing (63)
.docker/database/01-create-keycloak.sql.docker/database/01-create-workastra.sql.docker/database/02-create-workastra.sql.docker/keycloak/realm/master.json.docker/keycloak/realm/workastra.json.editorconfig.github/dependabot.yaml.github/scripts/sqruff_to_sarif.py.github/workflows/linter_formatter.yaml.github/workflows/workastra_platform.yaml.gitignore.prettierignore.prettierrc.yaml.sqruff.squawk.toml.vscode/launch.json.vscode/settings.json.yamllint.yamlbuild.gradle.ktscompose.yamlconfig/checkstyle/checkstyle.xmlconfig/checkstyle/suppressions.xmlconsole/Dockerfileconsole/build.gradle.ktsconsole/src/main/java/com/workastra/console/ConsoleApplication.javaconsole/src/main/java/com/workastra/console/controllers/AuthController.javaconsole/src/main/java/com/workastra/console/controllers/package-info.javaconsole/src/main/java/com/workastra/console/package-info.javaconsole/src/main/resources/application.yamlconsole/src/test/java/com/workastra/console/ConsoleApplicationTests.javacore/build.gradle.ktscore/src/main/java/com/workastra/core/infrastructure/JdbcLockConfiguration.javacore/src/main/java/com/workastra/core/infrastructure/PasswordEncoderConfiguration.javacore/src/main/java/com/workastra/core/infrastructure/package-info.javacore/src/main/java/com/workastra/core/package-info.javacore/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.importsiam/Dockerfileiam/build.gradle.ktsiam/jvm.Dockerfileiam/src/main/java/com/workastra/iam/Bootstrap.javaiam/src/main/java/com/workastra/iam/IAMApplication.javaiam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.javaiam/src/main/java/com/workastra/iam/configuration/TokenCustomizer.javaiam/src/main/java/com/workastra/iam/configuration/UserInfoMapper.javaiam/src/main/java/com/workastra/iam/configuration/package-info.javaiam/src/main/java/com/workastra/iam/entity/User.javaiam/src/main/java/com/workastra/iam/entity/package-info.javaiam/src/main/java/com/workastra/iam/package-info.javaiam/src/main/java/com/workastra/iam/repository/UserRepository.javaiam/src/main/java/com/workastra/iam/repository/package-info.javaiam/src/main/java/com/workastra/iam/service/CustomUserDetailsService.javaiam/src/main/java/com/workastra/iam/service/package-info.javaiam/src/main/resources/application.yamlmigration/Dockerfilemigration/build.gradle.ktsmigration/jvm.Dockerfilemigration/src/main/java/com/workastra/migration/MigrationApplication.javamigration/src/main/java/com/workastra/migration/package-info.javamigration/src/main/resources/application.yamlmigration/src/main/resources/banner.txtmigration/src/main/resources/db/migration/postgresql/V0_1_0__2026-04-23 - Hello World.sqlqodana.yamlsettings.gradle.kts
💤 Files with no reviewable changes (4)
- .docker/database/02-create-workastra.sql
- .docker/keycloak/realm/master.json
- .docker/database/01-create-keycloak.sql
- .docker/keycloak/realm/workastra.json
✅ Files skipped from review due to trivial changes (31)
- .github/dependabot.yaml
- iam/src/main/java/com/workastra/iam/configuration/package-info.java
- console/src/main/java/com/workastra/console/controllers/package-info.java
- .gitignore
- iam/src/main/java/com/workastra/iam/service/package-info.java
- iam/src/main/resources/application.yaml
- console/src/main/java/com/workastra/console/package-info.java
- .squawk.toml
- migration/src/main/resources/banner.txt
- iam/src/main/java/com/workastra/iam/repository/package-info.java
- core/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports
- config/checkstyle/suppressions.xml
- console/src/test/java/com/workastra/console/ConsoleApplicationTests.java
- migration/src/main/java/com/workastra/migration/package-info.java
- iam/src/main/java/com/workastra/iam/entity/package-info.java
- .sqruff
- iam/src/main/java/com/workastra/iam/IAMApplication.java
- .editorconfig
- .vscode/launch.json
- iam/src/main/java/com/workastra/iam/repository/UserRepository.java
- iam/build.gradle.kts
- console/src/main/java/com/workastra/console/controllers/AuthController.java
- migration/build.gradle.kts
- qodana.yaml
- iam/src/main/java/com/workastra/iam/package-info.java
- .yamllint.yaml
- migration/jvm.Dockerfile
- .vscode/settings.json
- core/src/main/java/com/workastra/core/infrastructure/PasswordEncoderConfiguration.java
- config/checkstyle/checkstyle.xml
- migration/src/main/java/com/workastra/migration/MigrationApplication.java
🚧 Files skipped from review as they are similar to previous changes (9)
- migration/src/main/resources/application.yaml
- core/src/main/java/com/workastra/core/infrastructure/package-info.java
- console/src/main/resources/application.yaml
- core/src/main/java/com/workastra/core/package-info.java
- .prettierrc.yaml
- .github/workflows/linter_formatter.yaml
- iam/jvm.Dockerfile
- .github/workflows/workastra_platform.yaml
- build.gradle.kts
Comment on lines
+1
to
+5
| CREATE DATABASE workastra_db; | ||
|
|
||
| CREATE USER workastra_user WITH ENCRYPTED PASSWORD 'workastra_password' ; | ||
|
|
||
| GRANT ALL PRIVILEGES ON DATABASE workastra_db TO workastra_user ; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type d -name "migration" | head -20Repository: workastra/platform
Length of output: 168
🏁 Script executed:
find . -type d -name "db" -path "*/migration/*" | head -20Repository: workastra/platform
Length of output: 96
🏁 Script executed:
fd "flyway\|migration" -t d | head -20Repository: workastra/platform
Length of output: 44
🏁 Script executed:
git ls-files | grep -i migration | head -20Repository: workastra/platform
Length of output: 451
🏁 Script executed:
cat -n "./migration/src/main/resources/db/migration/postgresql/V0_1_0__2026-04-23 - Hello World.sql"Repository: workastra/platform
Length of output: 6635
🏁 Script executed:
cat -n "./migration/src/main/resources/application.yaml"Repository: workastra/platform
Length of output: 325
🏁 Script executed:
find . -name "docker-compose*" -type fRepository: workastra/platform
Length of output: 44
🏁 Script executed:
cat "./migration/Dockerfile"Repository: workastra/platform
Length of output: 836
🏁 Script executed:
cat "./migration/jvm.Dockerfile"Repository: workastra/platform
Length of output: 797
🏁 Script executed:
wc -l "./migration/src/main/resources/application.yaml"Repository: workastra/platform
Length of output: 113
🏁 Script executed:
cat -n "./migration/src/main/resources/application.yaml" | tail -50Repository: workastra/platform
Length of output: 325
🏁 Script executed:
find . -name "docker-compose.yml" -o -name "docker-compose.yaml" | head -10Repository: workastra/platform
Length of output: 44
🏁 Script executed:
find . -name "Dockerfile" -path "*database*" | head -10Repository: workastra/platform
Length of output: 44
🏁 Script executed:
cat "./.docker/database/Dockerfile" 2>/dev/null || echo "Not found"Repository: workastra/platform
Length of output: 72
🏁 Script executed:
ls -la ./.docker/Repository: workastra/platform
Length of output: 231
🏁 Script executed:
find . -name "application-*.yaml" -o -name "application-*.yml" | head -20Repository: workastra/platform
Length of output: 44
🏁 Script executed:
find . -name "docker-compose*" | xargs ls -la 2>/dev/null || echo "No docker-compose files found"Repository: workastra/platform
Length of output: 1795
🏁 Script executed:
ls -la .docker/database/Repository: workastra/platform
Length of output: 243
🏁 Script executed:
cat .docker/database/Dockerfile 2>/dev/null || echo "No Dockerfile in database"Repository: workastra/platform
Length of output: 88
🏁 Script executed:
cat compose.yamlRepository: workastra/platform
Length of output: 420
🏁 Script executed:
cat migration/build.gradle.ktsRepository: workastra/platform
Length of output: 772
🏁 Script executed:
grep -r "datasource" . --include="*.yaml" --include="*.yml" --include="*.gradle.kts" 2>/dev/null | head -20Repository: workastra/platform
Length of output: 44
🏁 Script executed:
grep -r "flyway" . --include="*.yaml" --include="*.yml" --include="*.gradle.kts" 2>/dev/null | head -30Repository: workastra/platform
Length of output: 420
🏁 Script executed:
grep -r "workastra_user\|SPRING_DATASOURCE" . --include="*.yaml" --include="*.yml" --include="*.sh" --include="*.gradle.kts" 2>/dev/nullRepository: workastra/platform
Length of output: 44
🏁 Script executed:
find . -name "*.yaml" -o -name "*.yml" | xargs grep -l "spring:" 2>/dev/nullRepository: workastra/platform
Length of output: 152
🏁 Script executed:
cat ./iam/src/main/resources/application.yamlRepository: workastra/platform
Length of output: 209
🏁 Script executed:
find . -path "*/src/main/resources/application*.yaml" -o -path "*/src/main/resources/application*.yml" | xargs catRepository: workastra/platform
Length of output: 420
🏁 Script executed:
cat ./config/* 2>/dev/null || echo "No config files"Repository: workastra/platform
Length of output: 78
🏁 Script executed:
find ./core -name "application*.yaml" -o -name "application*.yml"Repository: workastra/platform
Length of output: 44
🏁 Script executed:
find . -name "docker-compose*" -o -name "compose*.yaml" -o -name "compose*.yml" | xargs cat 2>/dev/nullRepository: workastra/platform
Length of output: 420
🏁 Script executed:
grep -r "SPRING_DATASOURCE\|DATABASE_" . --include="Dockerfile*" --include="*.sh" --include="*.yaml" 2>/dev/nullRepository: workastra/platform
Length of output: 44
🏁 Script executed:
find . -name "*.sh" -type f | xargs grep -l "java\|flyway\|migration" 2>/dev/nullRepository: workastra/platform
Length of output: 46
🏁 Script executed:
find . -path "*/.github/workflows/*" -name "*.yaml" -o -name "*.yml" | head -5Repository: workastra/platform
Length of output: 148
🏁 Script executed:
cat ./.github/workflows/workastra_platform.yamlRepository: workastra/platform
Length of output: 3666
🏁 Script executed:
cat migration/build.gradle.kts | head -40Repository: workastra/platform
Length of output: 772
🏁 Script executed:
find . -name "MigrationApplication.java" | xargs catRepository: workastra/platform
Length of output: 388
🏁 Script executed:
grep -r "spring.datasource\|SPRING_DATASOURCE" . --include="*.java" 2>/dev/null | head -20Repository: workastra/platform
Length of output: 44
🏁 Script executed:
find . -name "application*.properties" 2>/dev/null | xargs cat 2>/dev/nullRepository: workastra/platform
Length of output: 44
🏁 Script executed:
cat core/build.gradle.ktsRepository: workastra/platform
Length of output: 665
🏁 Script executed:
find . -type f \( -name "*.sh" -o -name "Dockerfile*" \) | xargs grep -l "migration\|flyway" 2>/dev/nullRepository: workastra/platform
Length of output: 123
🏁 Script executed:
grep -r "MigrationApplication\|SpringApplication" . --include="*.java" -A 5 -B 2 2>/dev/null | head -40Repository: workastra/platform
Length of output: 3470
🏁 Script executed:
find . -path "*/.docker/*" -type f | xargs cat 2>/dev/nullRepository: workastra/platform
Length of output: 234
🏁 Script executed:
cat build.gradle.kts | grep -A 20 -B 5 "migration"Repository: workastra/platform
Length of output: 44
workastra_user cannot create objects in the public schema on PostgreSQL 18.
GRANT ALL PRIVILEGES ON DATABASE grants only database-level privileges (CONNECT, CREATE schema, TEMPORARY) — not schema-level privileges. PostgreSQL 15+ revokes the CREATE privilege on the public schema from non-owner roles. When Flyway attempts to create the schema history table in the public schema, it will fail with a permission error if executed as workastra_user.
Either make workastra_user the database owner or explicitly grant schema privileges:
🔧 Suggested fix
-CREATE DATABASE workastra_db;
-
CREATE USER workastra_user WITH ENCRYPTED PASSWORD 'workastra_password' ;
+
+CREATE DATABASE workastra_db OWNER workastra_user;
GRANT ALL PRIVILEGES ON DATABASE workastra_db TO workastra_user ;
+
+\connect workastra_db
+GRANT ALL ON SCHEMA public TO workastra_user;Alternatively, keep ownership with postgres and grant explicitly:
\connect workastra_db
GRANT CREATE, USAGE ON SCHEMA public TO workastra_user;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE DATABASE workastra_db; | |
| CREATE USER workastra_user WITH ENCRYPTED PASSWORD 'workastra_password' ; | |
| GRANT ALL PRIVILEGES ON DATABASE workastra_db TO workastra_user ; | |
| CREATE USER workastra_user WITH ENCRYPTED PASSWORD 'workastra_password' ; | |
| CREATE DATABASE workastra_db OWNER workastra_user; | |
| GRANT ALL PRIVILEGES ON DATABASE workastra_db TO workastra_user ; | |
| \connect workastra_db | |
| GRANT ALL ON SCHEMA public TO workastra_user; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.docker/database/01-create-workastra.sql around lines 1 - 5, The
CREATE/GRANT statements create workastra_db and workastra_user but do not allow
workastra_user to create objects in the public schema, so Flyway (when run as
workastra_user) will fail to create the schema history table; fix by either
making workastra_user the owner of workastra_db (so it has schema CREATE rights)
or explicitly granting schema-level privileges on public to workastra_user
(e.g., connect to workastra_db and GRANT CREATE, USAGE ON SCHEMA public TO
workastra_user), and ensure these changes are applied after the CREATE DATABASE
and CREATE USER steps so workastra_user can create schemas/objects.
| @@ -0,0 +1,5 @@ | |||
| CREATE DATABASE workastra_db; | |||
|
|
|||
| CREATE USER workastra_user WITH ENCRYPTED PASSWORD 'workastra_password' ; | |||
There was a problem hiding this comment.
Avoid hardcoding the database password in the bootstrap script.
workastra_password is checked into source control. Even for local dev, prefer parameterizing via environment variables (e.g., a .sh initdb script that reads ${WORKASTRA_DB_PASSWORD}) so the same script can be safely reused beyond local dev. Also, the trailing whitespace before ; on lines 3 and 5 is cosmetic noise.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.docker/database/01-create-workastra.sql at line 3, The SQL currently
hardcodes the password for user workastra_user; change the bootstrapping so the
password is provided from an environment variable (e.g., read
${WORKASTRA_DB_PASSWORD} in an init-shell script and pass it into the CREATE
USER statement) instead of embedding 'workastra_password', and remove the
trailing spaces before the semicolons to tidy the SQL; ensure the init script
validates the env var is set and fails fast with a clear message if not.
Comment on lines
8
to
10
| volumes: | ||
| - database_data:/var/lib/postgresql/18/data | ||
| - ./.docker/database:/docker-entrypoint-initdb.d |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the default PGDATA path in the official postgres:18 Docker image and where should named volumes be mounted?
💡 Result:
In the official postgres:18 Docker image, the default PGDATA path is /var/lib/postgresql/18/docker. Named volumes should be mounted at /var/lib/postgresql (the parent directory declared as VOLUME in the image), allowing PostgreSQL to create and use the version-specific PGDATA subdirectory within it for data persistence across container recreations and easier major version upgrades.
Citations:
- 1: https://hub.docker.com/_/postgres
- 2: https://github.com/docker-library/docs/blob/master/postgres/README.md
- 3: Change
PGDATAin 18+ to/var/lib/postgresql/MAJOR/dockerdocker-library/postgres#1259
Volume mount path is incompatible with postgres:18 — data will not persist.
In the official postgres:18 image, PGDATA is /var/lib/postgresql/18/docker, and the declared VOLUME is /var/lib/postgresql. Mounting database_data at /var/lib/postgresql/18/data does not match the actual PGDATA location, so PostgreSQL will write its data inside the container's anonymous volume at /var/lib/postgresql/18/docker while your named volume remains empty. On container recreation, all data (including roles/databases bootstrapped via /docker-entrypoint-initdb.d) will be lost.
Mount the parent directory so the image creates the versioned subdirectory inside your named volume:
🔧 Suggested fix
volumes:
- - database_data:/var/lib/postgresql/18/data
+ - database_data:/var/lib/postgresql
- ./.docker/database:/docker-entrypoint-initdb.dAlternatively, mount directly at the PGDATA path: database_data:/var/lib/postgresql/18/docker.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| volumes: | |
| - database_data:/var/lib/postgresql/18/data | |
| - ./.docker/database:/docker-entrypoint-initdb.d | |
| volumes: | |
| - database_data:/var/lib/postgresql | |
| - ./.docker/database:/docker-entrypoint-initdb.d |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compose.yaml` around lines 8 - 10, The compose YAML mounts the named volume
at /var/lib/postgresql/18/data which doesn't match the postgres:18 PGDATA
location, so PostgreSQL will write into the container's internal
/var/lib/postgresql/18/docker and your named volume will stay empty; update the
volumes entry for database_data to either mount the parent directory (use
database_data:/var/lib/postgresql) so the image can create the versioned subdir,
or mount directly to the PGDATA path
(database_data:/var/lib/postgresql/18/docker) so data persists across container
recreation.
Comment on lines
11
to
14
| ports: | ||
| - 8000:8080 | ||
| depends_on: | ||
| - database | ||
| - 5432:5432 | ||
| networks: | ||
| - backend |
There was a problem hiding this comment.
Binding 5432 to all interfaces and removing backend network — confirm intent.
Two things on this hunk worth a sanity check:
- 5432:5432binds PostgreSQL to0.0.0.0on the host; combined with the hardcodedsuperadmin/passwordcredentials, anyone on the LAN can connect. For local dev, prefer"127.0.0.1:5432:5432".- The summary states the
backendnetwork attachment was removed fromdatabase, but the file still showsnetworks: - backend(lines 13–14) and the top-levelnetworks: backend:block remains. With Keycloak gone and no other services onbackend, this network is now unused — either drop it or leave a comment explaining why it stays.
🔧 Suggested fix
ports:
- - 5432:5432
- networks:
- - backend
-
-networks:
- backend:
+ - "127.0.0.1:5432:5432"
volumes:
database_data:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compose.yaml` around lines 11 - 14, The compose service is binding Postgres
to all interfaces via the ports entry "- 5432:5432" and still references the
"backend" network even though it's unused; update the ports mapping to a
loopback-only binding (e.g., change "- 5432:5432" to "127.0.0.1:5432:5432" or
remove the host mapping entirely for dev), and either remove the "backend"
network reference from the database service and delete the top-level networks:
backend block, or leave the network and add a clear comment explaining why it
must remain; also ensure any hardcoded credentials referenced nearby (e.g.,
superadmin/password) are noted for separate cleanup.
Comment on lines
+29
to
+51
| SecurityFilterChain authorizationServerSecurityFilterChain( | ||
| HttpSecurity http, | ||
| Function<OidcUserInfoAuthenticationContext, OidcUserInfo> userInfoMapper | ||
| ) throws Exception { | ||
| http | ||
| .oauth2AuthorizationServer(authorizationServer -> { | ||
| http.securityMatcher(authorizationServer.getEndpointsMatcher()); | ||
|
|
||
| authorizationServer.oidc(oidc -> | ||
| oidc.userInfoEndpoint(userInfo -> userInfo.userInfoMapper(userInfoMapper)) | ||
| ); | ||
| }) | ||
| .authorizeHttpRequests(authorize -> authorize.anyRequest().authenticated()) | ||
| // Redirect to the login page when not authenticated from the authorization endpoint | ||
| .exceptionHandling(exceptions -> | ||
| exceptions.defaultAuthenticationEntryPointFor( | ||
| new LoginUrlAuthenticationEntryPoint("/login"), | ||
| new MediaTypeRequestMatcher(MediaType.TEXT_HTML) | ||
| ) | ||
| ); | ||
|
|
||
| return http.build(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there is no resource-server / JwtDecoder bean elsewhere that would make /userinfo work.
rg -n -C3 'oauth2ResourceServer|JwtDecoder|NimbusJwtDecoder|JWKSource' iam/srcRepository: workastra/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check the actual file mentioned in the review
cat -n iam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.javaRepository: workastra/platform
Length of output: 3378
🏁 Script executed:
#!/bin/bash
# Search entire codebase for JwtDecoder bean definitions with @ annotations
rg -n '@Bean|@Configuration' iam/src/main/java/com/workastra/iam/configuration/ | head -30Repository: workastra/platform
Length of output: 471
🏁 Script executed:
#!/bin/bash
# Search for oauth2ResourceServer in the entire repository (not just iam/src)
rg -n 'oauth2ResourceServer' . --type javaRepository: workastra/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for JwtDecoder in the entire repository
rg -n 'JwtDecoder|JWKSource' . --type java | head -50Repository: workastra/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# More thorough search for resource-server or JWT-related security configurations
# Check for any SecurityFilterChain beans that might be handling JWT
rg -n 'SecurityFilterChain|@Order' . --type java | grep -A2 -B2 'oauth2'Repository: workastra/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for application.properties or application.yml that might configure JWT
find . -name "application*.properties" -o -name "application*.yml" | xargs cat 2>/dev/null | head -100Repository: workastra/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if there are other configuration files in the iam module
find iam/src -name "*.java" -type f | grep -i config | head -20Repository: workastra/platform
Length of output: 359
🏁 Script executed:
#!/bin/bash
# Examine the UserInfoMapper class - key to understanding the /userinfo endpoint behavior
cat -n iam/src/main/java/com/workastra/iam/configuration/UserInfoMapper.javaRepository: workastra/platform
Length of output: 3725
🏁 Script executed:
#!/bin/bash
# Examine the TokenCustomizer class
cat -n iam/src/main/java/com/workastra/iam/configuration/TokenCustomizer.javaRepository: workastra/platform
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Check if there's a main application configuration or parent POM
find . -name "pom.xml" -o -name "build.gradle" | head -10Repository: workastra/platform
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search for any mention of /userinfo endpoint behavior or testing
rg -n 'userinfo|/userinfo|UserInfo' . --type java | head -30Repository: workastra/platform
Length of output: 1823
Missing oauth2ResourceServer(jwt) on the authorization-server filter chain — /userinfo will fail with JwtAuthenticationToken requirement.
The OIDC UserInfo endpoint receives requests with Authorization: Bearer <access_token> headers. Without oauth2ResourceServer(jwt) configured on the filter chain, Bearer tokens are not converted to JwtAuthenticationToken. However, UserInfoMapper (line 35) explicitly handles only JwtAuthenticationToken:
case JwtAuthenticationToken jwt -> this.buildUserInfoClaims(jwt);
default -> throw new IllegalStateException("Unsupported principal type: " + principal.getClass().getName());Any request without a JwtAuthenticationToken principal will trigger the default branch and throw IllegalStateException.
Suggested fix
`@Bean`
`@Order`(1)
SecurityFilterChain authorizationServerSecurityFilterChain(
HttpSecurity http,
Function<OidcUserInfoAuthenticationContext, OidcUserInfo> userInfoMapper
) throws Exception {
http
.oauth2AuthorizationServer(authorizationServer -> {
http.securityMatcher(authorizationServer.getEndpointsMatcher());
authorizationServer.oidc(oidc ->
oidc.userInfoEndpoint(userInfo -> userInfo.userInfoMapper(userInfoMapper))
);
})
.authorizeHttpRequests(authorize -> authorize.anyRequest().authenticated())
// Redirect to the login page when not authenticated from the authorization endpoint
.exceptionHandling(exceptions ->
exceptions.defaultAuthenticationEntryPointFor(
new LoginUrlAuthenticationEntryPoint("/login"),
new MediaTypeRequestMatcher(MediaType.TEXT_HTML)
)
- );
+ )
+ .oauth2ResourceServer(rs -> rs.jwt(Customizer.withDefaults()));
return http.build();
}This also requires a JwtDecoder bean, typically derived from the authorization server's JWKSource (e.g., via OidcProviderConfiguration).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@iam/src/main/java/com/workastra/iam/configuration/AuthorizationServerConfiguration.java`
around lines 29 - 51, Add JWT resource-server support to the
authorization-server filter chain so Bearer access tokens become
JwtAuthenticationToken for the OIDC /userinfo flow: update the
SecurityFilterChain method authorizationServerSecurityFilterChain to call
oauth2ResourceServer(jwt) on the same HttpSecurity instance used in
oauth2AuthorizationServer and configure JWT decoding (ensure a JwtDecoder bean
derived from your JWKSource/OIDC provider is available). This change ensures the
userInfoMapper (used in oidc.userInfoEndpoint with
OidcUserInfoAuthenticationContext) will receive JwtAuthenticationToken instances
instead of causing the IllegalStateException in the default branch.
cbdc5cd to
868064d
Compare
phongphongg
force-pushed
the
feat/authorization-server
branch
from
868064d to
ced3206
Compare
phongphongg
added
type: feature
phongphongg
added
type: chore
This file contains hidden or 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
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.
Summary by CodeRabbit
New Features
/api/v1/auth/meendpoint to retrieve current authenticated user information.Infrastructure
Chores