[NAE-2118] Implement OpenID Connector Auth for Admin node #317
Conversation
- change registry collection type in ProviderRegistry - add doc for ProviderRegistry.registerProvider
- fix logging in UserServiceImpl
- optimize RealmServiceImpl.addProvider - remove unused AuthMethodProvider.createAuthMethod
Retoocs
added
improvement
WalkthroughThe changes update authentication provider management by modifying the method signatures and return types for adding providers, removing the creation of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RealmService
participant ProviderRegistry
participant RealmRepository
Client->>RealmService: addProvider(realmId, config)
RealmService->>ProviderRegistry: getProvider(config.type)
ProviderRegistry-->>RealmService: provider
RealmService->>RealmRepository: findById(realmId)
RealmRepository-->>RealmService: realm
RealmService->>Realm: addAuthMethodConfig(config)
RealmService->>RealmRepository: save(realm)
RealmRepository-->>RealmService: updatedRealm
RealmService-->>Client: updatedRealm
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/provider/ProviderRegistry.java (1)
17-23: Minor Javadoc polishNit: the description sentences are missing a terminating period and start with a lowercase character. Tightening this keeps generated docs clean.
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java (1)
132-135: Log level reconsiderationA missing user is currently logged with
WARN. Given that look-ups for non-existent users can be part of normal control flow (e.g. login attempts), consider downgrading toDEBUGto avoid log noise and alert fatigue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java(1 hunks)nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/UserServiceImpl.java(1 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/provider/AuthMethodProvider.java(0 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/provider/ProviderRegistry.java(1 hunks)nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/RealmService.java(1 hunks)
💤 Files with no reviewable changes (1)
- nae-user-common/src/main/java/com/netgrif/application/engine/auth/provider/AuthMethodProvider.java
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: Build
🔇 Additional comments (1)
nae-user-common/src/main/java/com/netgrif/application/engine/auth/service/RealmService.java (1)
35-35: Adjust downstream callersThe return type change from
AuthMethodtoRealmis a breaking API modification. Ensure every call-site (services, controllers, tests) has been updated; otherwise compilation errors or silent misbehaviour (ignored return value) will surface.
| protected final Map<String, Class<? extends AbstractAuthConfig>> configClasses = new ConcurrentHashMap<>(); | ||
|
|
||
| protected final Map<String, AuthMethodProvider<?>> providers = new HashMap<>(); | ||
| protected final Map<String, AuthMethodProvider<?>> providers = new ConcurrentHashMap<>(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Locale-safe keys & duplicate-registration guard
type.toLowerCase() depends on the default JVM locale (e.g. Turkish “İ”), which may lead to subtle look-ups mismatches.
A ConcurrentHashMap already offers putIfAbsent; using it makes accidental double registration explicit.
- protected final Map<String, Class<? extends AbstractAuthConfig>> configClasses = new ConcurrentHashMap<>();
- protected final Map<String, AuthMethodProvider<?>> providers = new ConcurrentHashMap<>();
+ protected final ConcurrentMap<String, Class<? extends AbstractAuthConfig>> configClasses = new ConcurrentHashMap<>();
+ protected final ConcurrentMap<String, AuthMethodProvider<?>> providers = new ConcurrentHashMap<>();- providers.put(type.toLowerCase(), provider);
- configClasses.put(type.toLowerCase(), provider.getConfigClass());
+ String key = type.toLowerCase(Locale.ROOT);
+ if (providers.putIfAbsent(key, provider) != null) {
+ log.warn("Provider for type [{}] was already registered – overriding", key);
+ }
+ configClasses.put(key, provider.getConfigClass());📝 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.
| protected final Map<String, Class<? extends AbstractAuthConfig>> configClasses = new ConcurrentHashMap<>(); | |
| protected final Map<String, AuthMethodProvider<?>> providers = new HashMap<>(); | |
| protected final Map<String, AuthMethodProvider<?>> providers = new ConcurrentHashMap<>(); | |
| // switch from general Map to ConcurrentMap to expose atomic operations | |
| protected final ConcurrentMap<String, Class<? extends AbstractAuthConfig>> configClasses = new ConcurrentHashMap<>(); | |
| protected final ConcurrentMap<String, AuthMethodProvider<?>> providers = new ConcurrentHashMap<>(); | |
| // … inside your registration method … | |
| // use a locale-neutral lowercase key | |
| String key = type.toLowerCase(Locale.ROOT); | |
| // guard against double-registration | |
| if (providers.putIfAbsent(key, provider) != null) { | |
| log.warn("Provider for type [{}] was already registered – overriding", key); | |
| } | |
| // still update the configClass map | |
| configClasses.put(key, provider.getConfigClass()); |
🤖 Prompt for AI Agents
In
nae-user-common/src/main/java/com/netgrif/application/engine/auth/provider/ProviderRegistry.java
around lines 13 to 16, avoid using type.toLowerCase() for keys to prevent
locale-dependent issues. Instead, normalize keys in a locale-safe way, such as
using Locale.ROOT with toLowerCase. Also, replace any manual checks for
duplicate registrations with the ConcurrentHashMap's putIfAbsent method to
explicitly handle and prevent accidental double registrations.
| public <C extends AbstractAuthConfig> Realm addProvider(String realmId, AuthMethodConfig<C> config) { | ||
| AuthMethodProvider<C> provider = (AuthMethodProvider<C>) providerRegistry.getProvider(config.getType()); | ||
| if (provider == null) { | ||
| throw new IllegalArgumentException("Provider type " + config.getType() + " not found"); | ||
| } | ||
|
|
||
| AuthMethod<C> authMethod = provider.createAuthMethod(config); | ||
| Realm realm = getRealmById(realmId).orElseThrow(() -> new IllegalArgumentException("Realm with id " + realmId + " not found")); | ||
| realm.addAuthMethod(config); | ||
| realmRepository.save(realm); | ||
|
|
||
| return (T) authMethod; | ||
| return realmRepository.save(realm); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unchecked cast & missing config-provider compatibility check
AuthMethodProvider<C>cast is unchecked and the variable is not used any further – it can be safely dropped.- Nothing verifies that the supplied
configmatches the provider’s expected configuration class; a typo intypewould pass until runtime errors occur elsewhere.
- AuthMethodProvider<C> provider = (AuthMethodProvider<C>) providerRegistry.getProvider(config.getType());
- if (provider == null) {
- throw new IllegalArgumentException("Provider type " + config.getType() + " not found");
- }
+ AuthMethodProvider<?> provider = providerRegistry.getProvider(config.getType());
+ if (provider == null) {
+ throw new IllegalArgumentException("Provider type %s not found".formatted(config.getType()));
+ }
+ if (!provider.getConfigClass().isAssignableFrom(config.getClass())) {
+ throw new IllegalArgumentException("Config class %s is not compatible with provider %s"
+ .formatted(config.getClass().getSimpleName(), config.getType()));
+ }Also remove the now-unused AuthMethod import to avoid compiler warnings.
📝 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.
| public <C extends AbstractAuthConfig> Realm addProvider(String realmId, AuthMethodConfig<C> config) { | |
| AuthMethodProvider<C> provider = (AuthMethodProvider<C>) providerRegistry.getProvider(config.getType()); | |
| if (provider == null) { | |
| throw new IllegalArgumentException("Provider type " + config.getType() + " not found"); | |
| } | |
| AuthMethod<C> authMethod = provider.createAuthMethod(config); | |
| Realm realm = getRealmById(realmId).orElseThrow(() -> new IllegalArgumentException("Realm with id " + realmId + " not found")); | |
| realm.addAuthMethod(config); | |
| realmRepository.save(realm); | |
| return (T) authMethod; | |
| return realmRepository.save(realm); | |
| } | |
| public <C extends AbstractAuthConfig> Realm addProvider(String realmId, AuthMethodConfig<C> config) { | |
| AuthMethodProvider<?> provider = providerRegistry.getProvider(config.getType()); | |
| if (provider == null) { | |
| throw new IllegalArgumentException("Provider type %s not found" | |
| .formatted(config.getType())); | |
| } | |
| if (!provider.getConfigClass().isAssignableFrom(config.getClass())) { | |
| throw new IllegalArgumentException("Config class %s is not compatible with provider %s" | |
| .formatted(config.getClass().getSimpleName(), config.getType())); | |
| } | |
| Realm realm = getRealmById(realmId) | |
| .orElseThrow(() -> new IllegalArgumentException("Realm with id " + realmId + " not found")); | |
| realm.addAuthMethod(config); | |
| return realmRepository.save(realm); | |
| } |
🤖 Prompt for AI Agents
In
nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java
around lines 101 to 110, remove the unchecked cast to AuthMethodProvider<C> and
the unused provider variable entirely. Instead, add a check to verify that the
config's class matches the expected configuration class for the provider type
before proceeding. If the types do not match, throw an IllegalArgumentException
with a clear message. Also, remove the unused AuthMethod import to prevent
compiler warnings.
There was a problem hiding this comment.
Odpisujem ti po slovensky. Existuje predpoklad, ze ak najde provider cez config.getType() tak pre provider je automaticky splnena podmienka provider.getConfigClass().isAssignableFrom(config.getClass())
There was a problem hiding this comment.
@Retoocs Ďakujem za vysvetlenie! Máš pravdu - ak je systém navrhnutý tak, že provider registry garantuje kompatibilitu medzi config.getType() a príslušným providerom, tak dodatočná validácia isAssignableFrom je skutočne zbytočná.
V takom prípade by sa mohol kód zjednodušiť len odstránením nepoužitej premennej provider, keďže sa už nepoužíva po odstránení createAuthMethod volania:
@Override
public <C extends AbstractAuthConfig> Realm addProvider(String realmId, AuthMethodConfig<C> config) {
if (providerRegistry.getProvider(config.getType()) == null) {
throw new IllegalArgumentException("Provider type %s not found".formatted(config.getType()));
}
Realm realm = getRealmById(realmId)
.orElseThrow(() -> new IllegalArgumentException("Realm with id " + realmId + " not found"));
realm.addAuthMethod(config);
return realmRepository.save(realm);
}Toto by eliminovalo unchecked cast a zároveň zachovalo validáciu existencie providera.
(\ /)
( ^.^ )
o_(")(")
curious about the architecture
✏️ Learnings added
Learnt from: Retoocs
PR: netgrif/application-engine#317
File: nae-user-ce/src/main/java/com/netgrif/application/engine/auth/service/RealmServiceImpl.java:101-110
Timestamp: 2025-07-18T08:42:08.397Z
Learning: In the authentication provider system, when a provider is found via `config.getType()` through the provider registry, the condition `provider.getConfigClass().isAssignableFrom(config.getClass())` is automatically satisfied due to the system's design. The provider registry guarantees type compatibility between config types and their corresponding providers, making additional validation checks unnecessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Description
Optimized
RealmServiceImpl.addProvidermethod and updated some logging.Implements NAE-2118
Dependencies
No new dependencies were introduced
Third party dependencies
Blocking Pull requests
There are no dependencies on other PR
How Has Been This Tested?
Manually
Test Configuration
Checklist:
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation