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

Add OIDC backend configuration schema and validation #17274

Merged
merged 8 commits into from
Jan 15, 2024

Conversation

uwwint
Copy link
Contributor

@uwwint uwwint commented Jan 12, 2024

Add schema validation for oidc_backends_config.xml.
The base mechanism can be used for all XML files.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

…used for all XML files.

Mention accepted_audiences in oidc_backends_config.xml.sample
@github-actions github-actions bot added area/util area/auth Authentication and authorization labels Jan 12, 2024
@github-actions github-actions bot added this to the 24.0 milestone Jan 12, 2024
@uwwint
Copy link
Contributor Author

uwwint commented Jan 12, 2024

Hi @nuwang,

I did this one when testing OIDC.

KR,
Uwe

Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to have schema for our config options, however I think we discussed removing all those little config files and just defining the config inline in galaxy.yml. In the meantime config/schemas is not the right location for this, I'd sugest lib/galaxy/authnz/xsd to follow the pattern used for other schemas.

lib/galaxy/util/__init__.py Outdated Show resolved Hide resolved
uwwint and others added 2 commits January 15, 2024 13:10
change processing order for validation to occur after pre-processing the XML
ensure validation is only done if lxml is available
raise validation error, such that issue is immediately visible to the user.
@mvdbeek mvdbeek changed the title add schema validation to oidc_backends_config. Add OIDC backend configuration schema and validation Jan 15, 2024
@nuwang nuwang merged commit f7018e4 into galaxyproject:dev Jan 15, 2024
53 checks passed
@@ -294,13 +294,22 @@ def unique_id(KEY_SIZE=128):
return md5(random_bits).hexdigest()


def parse_xml(fname: StrPath, strip_whitespace=True, remove_comments=True) -> ElementTree:
def parse_xml(
fname: StrPath, schemafname: Union[StrPath, None] = None, strip_whitespace=True, remove_comments=True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could break the use of this method by third-party apps (via https://pypi.org/project/galaxy-util/ ), it would be safer to have schemafname be added as the last parameter).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to open a followup or should I do it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do it later.

Comment on lines +332 to +333
except etree.DocumentInvalid as e:
log.exception(f"Validation of file %s failed with error {e}" % fname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been:

    except etree.DocumentInvalid:
        log.exception("Validation of file %s failed", fname)

nsoranzo added a commit that referenced this pull request Jan 15, 2024
@galaxyproject galaxyproject deleted a comment from github-actions bot May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth Authentication and authorization area/util kind/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants