Skip to content
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.

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

feat: implement custom resolvers for custom auth over oauth2 #11053

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Aiosa
Copy link

@Aiosa Aiosa commented Oct 8, 2024

Resolves #11052

Describe changes proposed in this pull request:

  • SecurityMapper no longer retrieves study groups, these have their own mapper
  • SecurityRepository now is parametrized by another generic object, that is given by the auth configuration to the repository if used. Such repository then decides whether it is able to be used in the current context. Newly, repository can be switched between with security.repository.type:
    • cbioportal: the default, injects SecurityMyBatisRepository
    • disabled: allows all access, injects FullAccessResolver
    • [any other]: custom implementation + custom args

There is included a LsaaiOauth2DrivenResolver that demonstrates the usage; I would like to remove this implementation as it is specially designed to work for my usecase. It can be used like this:

security.repository.type=lsaai
security.repository.lsaai.userinfo=https://example.com/oidc/userinfo

I would like a discussion on whether this is a good design, or whether there are some issues (interaction with different flows, usage in different scenarios, over-complexity due the need to compile / build images yourself...). But IMHO this is a crucial feature if this app should be used in production systems - being able to customize authentication and authorization to your needs.

No new tests are necessary since this PR just modularizes the current behavior, so the current test suite should be sufficient.

Checks

  • The commit log is comprehensible. It follows 7 rules of great commit messages. We can fix this during merge by using a squash+merge if necessary
  • Has tests or has a separate issue that describes the types of test that should be created. If no test is included it should explicitly be mentioned in the PR why there is no test.
  • Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!
  • Make sure your PR has one of the labels defined in https://github.com/cBioPortal/cbioportal/blob/master/.github/release-drafter.yml (GUI does not let me add them)

@inodb
Copy link
Member

inodb commented Oct 24, 2024

Thanks @Aiosa ! Let's discuss if an alternative is to use keycloak as a middleman instead and manage custom resolvers there: https://www.keycloak.org/docs/latest/server_development/index.html#_providers

@Aiosa
Copy link
Author

Aiosa commented Oct 25, 2024

That indeed is a possible solution (at least not really looking into details). Of course cbioportal cannot support all the authentication designs out there, but so cannot keycloak.

There is included a LsaaiOauth2DrivenResolver that demonstrates the usage; I would like to remove this implementation as it is specially designed to work for my usecase.

The idea is that cbioportal implements the same logics as before, but in a way that someone can add custom resolver and compile own image. The same logics as keycloak supports I believe.

I would like to avoid dealing with keycloak, since it makes things harder to manage - unclear sources of auth errors, at least two more containers to run, unclear design (confusing for newcomers), bad user experience (some apps use lsaai, some keycloak, I cannot change every service we run just because of cbioportal), ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom OAuth2 Authroziation Hardcoded in Database
2 participants