[BUG] Security concerns with common gitlab code

[prasadtalasila]

Tyr common gitlab code violates security contrxt of python packages.

Steps to Reproduce

See this review from Claude

Review: extract dtaas-gitlab reusable GitLab client / user-provisioning library

Verdict: request changes.

The refactor itself is clean and behaviour-preserving. The packaging and CI
wiring around it is not: as written, merging this breaks PyPI publication of
dtaas-services, and the extracted code lands in a directory that no CI job
touches.

---

Verification performed

Everything below was reproduced locally against the branch head, not inferred
from diffs or from the SonarCloud summary.

Check	Result
dtaas_services suite	496 passed, 1 skipped, 5 errors (the 5 are tests/system_tests/ which shell out to docker, absent in this environment — unrelated to the change)
dtaas_gitlab suite	25 passed, 98% line coverage
pylint dtaas_gitlab tests --rcfile=.pylintrc	10.00/10
flake8 at the repo's --max-line-length=127	clean
poetry build --format wheel in deploy/services/cli	succeeds, but see Blocker 1
pip install of that wheel with the source tree absent	fails, see Blocker 1
PyPI state	dtaas-services latest published is 0.4.1; dtaas-gitlab returns 404 (does not exist)

No net test loss. The three PAT tests removed from
test_personal_token.py and the entire test_validators.py file reappear in
lib/tests/ with equivalent assertions. Worth stating explicitly, because a
diff showing 133 deleted test lines usually means something else.

---

Blockers

1. The path dependency is baked into the published wheel as an absolute local path

pyproject.toml gains:

dtaas-gitlab = {path = "../../../lib", develop = true}

Building the wheel produces this metadata:

Requires-Dist: dtaas-gitlab @ file:///<builder-working-directory>/lib

Two consequences, both confirmed:

1. PyPI will reject the upload. Warehouse refuses any requires_dist
   entry containing a direct URL — 400 Bad Request … Invalid value for requires_dist. Error: Can't have direct dependency. The publish-pypi
   job in platform-services-cli.yml fires on every push to
   feature/distributed-demo, so the first merge turns that job red.

2. If it somehow shipped, it would be uninstallable. Reproduced by moving
   the lib/ directory aside and installing the built wheel:

   Processing /<builder-path>/lib (from dtaas_services)
   ERROR: Could not install packages due to an OSError:
     [Errno 2] No such file or directory: '/<builder-path>/lib'
   

   The path is the build machine's path. It cannot resolve on any consumer
   machine. dtaas-gitlab is not on PyPI either, so there is no fallback.

Note also that the in-tree version is already 0.4.3 while PyPI has 0.4.1,
so the publish job will genuinely attempt an upload rather than no-op.

Options, roughly in order of preference:

• Publish dtaas-gitlab to PyPI first (its own workflow + trusted-publisher
  config), then depend on it by version range. The path dep can stay for local
  dev via a Poetry develop group or a [tool.poetry.group.dev] override,
  but the [tool.poetry.dependencies] entry that ends up in metadata must be
  a version constraint.
• Or keep the source shared but stop publishing it separately: keep the module
  inside dtaas_services and let the CLI depend on dtaas-services.
• Whichever route, add a guard so a direct-URL Requires-Dist can never reach
  the publish step — twine check does not catch this class, so a grep of
  the built metadata for @ file:// in CI is the cheap version.

2. lib/ has no CI at all

• No workflow has a paths: entry matching lib/**. lib-ms.yml matches
  servers/lib/**, which is the unrelated Node microservice.
• lint-scripts.yml runs flake8 and pylint over
  cli/ servers/ script/ deploy/services/cli/ deploy/workspace/ — lib/ is
  not in the list.
• platform-services-cli.yml filters on deploy/services/cli/**, so a change
  confined to lib/ will not run the services tests even though services now
  depends on that code at runtime. A regression introduced in
  dtaas_gitlab.create_user merges with a fully green PR.

This is what the 0.0% coverage-on-new-code figure is reporting: the 25 tests
exist and pass locally, but nothing in CI executes them or uploads their
coverage. Meanwhile the moved lines disappear from the --cov=dtaas_services
measurement, so the project-level number drops for free.

Minimum fix: a python-lib.yml reusing python-reusable.yml with
working-directory: lib, plus adding lib/** to the path filters of
platform-services-cli.yml and lint-scripts.yml.

3. No lib/poetry.lock

git ls-files lib/ shows no lockfile, and nothing in .gitignore excludes
one. Every other Poetry package in the repo commits its lock. Without it the
python-gitlab transitive chain re-resolves on every install, so builds are
not reproducible and there is no hash-pinned record of what a release was
actually built against — the same supply-chain gap flagged on the logger
microservice previously.

---

Security-relevant findings

4. The library mutates global warning state permanently

client.py:

if not ssl_verify:
    warnings.filterwarnings("ignore",
        category=urllib3.exceptions.InsecureRequestWarning)

This was acceptable when it lived inside a single application entry point. As
a library function it is not: one call anywhere in a host process silences
InsecureRequestWarning for the entire interpreter, for the rest of its life,
including every unrelated urllib3 consumer in that process. Any future
importer of dtaas-gitlab inherits that suppression without asking for it.

The library should not touch process-global state. Either drop the
filterwarnings call and let the consuming application decide (the current
_api.py caller is exactly the right place for it), or scope it with
warnings.catch_warnings() around the actual request rather than at client
construction.

Related: ssl_verify is typed bool, but python-gitlab accepts a CA-bundle
path string in the same slot. Typing it bool | str costs nothing and keeps
the secure option — verify against the deployment's own CA — available.
Right now the only escape hatch a self-signed GitLab deployment has is
switching verification off entirely.

5. PAT scope and lifetime are hardcoded policy in a shared library

USER_PAT_NAME = "dtaas"
USER_PAT_SCOPES = ["api", "read_repository", "write_repository"]
_PAT_EXPIRY_DAYS = 365

api is effectively full read/write across everything the user can reach —
it subsumes both repository scopes listed alongside it. A 365-day
non-rotating token with that scope, issued automatically per provisioned user,
is a broad standing credential.

None of this is new, but this PR is the moment it stops being one
application's choice and becomes the default for every future consumer,
including the CLI in the follow-up PR. That makes it the right time to turn
scopes, expires_at/expiry_days, and name into caller-supplied
parameters with a least-privilege default, so a consumer that only needs
read_repository can ask for it.

Minor, same function: datetime.now() is naive local time while GitLab
interprets expires_at against the instance's own clock. datetime.now( timezone.utc) removes an off-by-one-day edge case on the boundary.

6. "Created" and "already exists" are indistinguishable to a careless caller

create_user returns (True, "", None) for HTTP 409. The docstring explains
it, and the in-repo caller handles it correctly, but for a public library API
a True that means "someone else's account already occupies this username"
is a footgun. Two things worth doing:

• Return an explicit status (enum or a third state) rather than encoding it in
  a nullable id.
• Document loudly that on the 409 path the supplied password is not
  applied. A caller could reasonably assume that after create_user returns
  success, the credentials it passed are the live credentials for that
  account. They are not, and the provisioning flow silently issues no PAT.

7. No py.typed marker

packages = [{include = "dtaas_gitlab"}] ships no py.typed, so once the
library is installed as a real wheel, type information is stripped for
downstream checkers. This is invisible today only because develop = true
puts the source on the path. Add py.typed and include it in the package
data — cheap, and dtaas_services runs pyright.

---

Lower severity

8. README documents signatures that don't exist.
   create_user(gl, *, username, email, password, name=None) — there is no
   name parameter; the implementation hardcodes "name": username.
   create_user_pat(gl, user_id, username, ...) — the ... implies optional
   parameters that also don't exist. For a package whose README is its API
   contract, this should match the code exactly.

9. lib/ is an ambiguous home. The repo already has servers/lib (Node
   microservice) and dtaas_services/pkg/lib/. A third top-level lib/
   holding exactly one Python package invites confusion in path filters,
   grep, and docs — as it already has in the CI filters above. Placing it at
   lib/ matches the earlier design direction, so this is a naming nit
   rather than a redirect, but a README line and clearer package naming
   would help.

10. Docs not updated. docs/developer/codebase/dtaas-services.md still
    shows a package-layout tree containing validators.py under
    services/gitlab/, and docs/developer/codebase/publish-packages.md
    still says the repo maintains "Two Poetry-based CLIs" with no mention of
    a third publishable Python artifact or its release story.

11. Uncovered branches. users.py:61 (the non-409 GitlabCreateError
    path — the actual failure branch of the error handler) and
    validators.py:40. Both are one test each.

12. Packaging metadata. authors = ["Mohamed Abdulkarim"] omits the email
    Poetry conventionally expects. There is no CHANGELOG or release note for
    0.1.0, so a consumer pinning a version range has nothing to read.

---

What is good here

• The seam is drawn in the right place. _api.py keeps env-var resolution and
  URL derivation; the library takes an explicit URL and token and does no
  environment, filesystem, or console I/O. That is exactly the contract the
  README claims, and it holds under inspection.
• Behaviour is genuinely preserved. _create_single_user delegates with the
  same trimming, the same validation ordering, and the same
  (success, error, user_id) contract; _create_user_and_pat's handling of
  the user_id is None case is unchanged.
• Tests moved rather than vanished, and the new suite is slightly better
  organised than what it replaced.
• Validation still runs before any API call, so malformed usernames and
  emails never reach GitLab. That property survived the move intact.

@8ohamed, please make the necessary changes. Thanks.