feat: auth provider config with admin bootstrapping and SDK extraction#86
Merged
Conversation
This commit adds ORCiD OAuth and JWT authentication configuration to the deployment environment example file, providing developers with the necessary environment variables to set up authentication for their OSA instances. refactor: remove Python SDK from repository This commit removes the entire Python SDK codebase from the repository as part of a restructuring effort. The SDK included CLI tools, runtime components, type definitions, testing utilities, and deployment functionality that are being relocated or redesigned. refactor: remove CLI module and restructure config Remove CLI commands, console utilities, and daemon management to focus on server-only architecture. Restructure config by promoting server fields to top-level and moving auth providers to nested structure. feat: add admin ORCiD bootstrapping support Add auth.admins.orcid config field with validation for automatic SUPERADMIN role assignment on first login for specified ORCiD IDs.
|
Contributor
Greptile SummaryThis PR restructures the server config (
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| server/osa/config.py | Restructured config: promoted name/domain to top-level, added auth.providers and auth.admins with ORCiD validation. Clean refactor with proper validation. |
| server/osa/domain/auth/service/auth.py | Admin bootstrap logic grants SUPERADMIN to configured ORCiD IDs on first login. The external_id check is not scoped to the orcid provider. |
| deploy/.env.example | Adds ORCID_CLIENT_ID/SECRET and JWT_SECRET vars, but these are not wired through docker-compose.yml to the server container's expected OSA_AUTH__PROVIDERS__ORCID__ env vars. |
| server/.env.example | Not updated to reflect the config restructure — still uses OSA_AUTH__ORCID__CLIENT_ID instead of OSA_AUTH__PROVIDERS__ORCID__CLIENT_ID, so the env vars will be silently ignored. |
| server/osa/domain/auth/util/di/provider.py | Correctly passes admin_orcids set from config to AuthService. Clean DI wiring. |
| server/osa/infrastructure/auth/di.py | Updated to use config.auth.providers.orcid path. Correctly checks client_id before registering provider. |
| server/osa/infrastructure/index/di.py | Simplified to return empty IndexRegistry, making index backends dormant. Clean simplification. |
| server/tests/unit/config/test_config.py | Good test coverage for new config shape: top-level fields, provider-keyed auth, ORCiD validation, defaults. |
| server/tests/unit/domain/auth/test_admin_bootstrap.py | Comprehensive tests covering admin SUPERADMIN grant, non-admin base_role, empty admins, existing user non-retroactivity, and single-role assignment. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[OAuth Callback] --> B["_find_or_create_user()"]
B --> C{Linked account exists?}
C -->|Yes| D[Return existing user]
C -->|No| E[Create User + LinkedAccount]
E --> F{external_id in admin_orcids?}
F -->|Yes| G[effective_role = SUPERADMIN]
F -->|No| H{base_role configured?}
H -->|Yes| I[effective_role = base_role]
H -->|No| J[No role assigned]
G --> K[Save RoleAssignment]
I --> K
J --> L[Return new user]
K --> L
Comments Outside Diff (1)
-
server/.env.example, line 22-25 (link)Env vars point to the old config path
The config was restructured so that ORCiD settings live under
auth.providers.orcidinstead ofauth.orcid. With theOSA_prefix and__nested delimiter, these env vars should now beOSA_AUTH__PROVIDERS__ORCID__CLIENT_IDandOSA_AUTH__PROVIDERS__ORCID__CLIENT_SECRET. The current names (OSA_AUTH__ORCID__CLIENT_ID,OSA_AUTH__ORCID__CLIENT_SECRET) will silently be ignored because there is no longer anorcidfield directly onAuthConfig— it moved toAuthConfig.providers.orcid.Similarly,
OSA_AUTH__ORCID__SANDBOXon line 29 should becomeOSA_AUTH__PROVIDERS__ORCID__SANDBOX.
Last reviewed commit: 5932d57
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+459
to
+460
| if identity_info.external_id in self._admin_orcids: | ||
| effective_role = Role.SUPERADMIN |
Contributor
There was a problem hiding this comment.
Admin check is not scoped to the ORCiD provider
This checks identity_info.external_id in self._admin_orcids without verifying that identity_info.provider == "orcid". While only ORCiD is supported today, the architecture already supports multiple providers (see ProviderRegistry). If a future provider happens to use an external_id that matches an ORCiD ID pattern, a non-ORCiD user would be silently granted SUPERADMIN. Consider adding a provider guard:
Suggested change
| if identity_info.external_id in self._admin_orcids: | |
| effective_role = Role.SUPERADMIN | |
| if identity_info.provider == "orcid" and identity_info.external_id in self._admin_orcids: |
Comment on lines
+43
to
+44
| ORCID_CLIENT_ID= | ||
| ORCID_CLIENT_SECRET= |
Contributor
There was a problem hiding this comment.
ORCiD env vars are defined but never wired
ORCID_CLIENT_ID and ORCID_CLIENT_SECRET are defined here but deploy/docker-compose.yml does not map them to the OSA_AUTH__PROVIDERS__ORCID__CLIENT_ID / OSA_AUTH__PROVIDERS__ORCID__CLIENT_SECRET env vars the server expects. The JWT value is correctly mapped in docker-compose.yml, but the ORCiD credentials are missing. Without that mapping, deployers who fill in these values will find that ORCiD login is never enabled.
The server service's environment: section in docker-compose.yml needs corresponding entries to forward these values.
6 tasks
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
1 participant
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
osa.yamlto promotename/domainto top-level, add provider-keyed auth config (auth.providers.orcid), and addauth.admins.orcidlist for SUPERADMIN bootstrapping on first login. Operational config (database, logging, workers, indexes) stays env-var-only.auth.admins.orcidare granted SUPERADMIN at user creation time. Non-retroactive — existing users are unaffected.server/osa/cli/(cyclopts-based local dev CLI). MoveOSAPathstoserver/osa/util/paths.py. Removecyclopts,rich,typerdependencies and[project.scripts]entry point.indexesfrom config surface and simplifyIndexProviderto return an empty registry. Vector search code stays in place but is no longer wired.sdk/py/directory is removed from this repo — it now lives in its own repository.Test plan
osa.clireferencestest_admin_bootstrap.pycovers: admin ORCiD gets SUPERADMIN, non-admin gets base_role, existing users unaffectedtest_config.pycovers: top-level name/domain, provider-keyed auth, ORCiD ID validation,${VAR}interpolationdeploy/.env.exampleincludes auth config section (ORCID_CLIENT_ID, ORCID_CLIENT_SECRET, JWT_SECRET)ruff check osa/ tests/)Closes #85