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

Improve scope management #239

Merged
merged 2 commits into from
Nov 2, 2021
Merged

Improve scope management #239

merged 2 commits into from
Nov 2, 2021

Conversation

aarranz
Copy link
Contributor

@aarranz aarranz commented Sep 30, 2021

Proposed changes

This PR updates the validateScope method to allow providing a space-separated list of scopes (in addition to support current comma-separated behaviour). This PR also adds unit tests.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality
    to not work as expected)

Checklist

  • I have read the
    CONTRIBUTING
    doc
  • I have signed the
    CLA
  • I have added tests that prove my fix is effective or that my feature
    works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream
    modules

Further comments

OAuth2/Open ID Connect services use space-separated lists of scopes by default, so this should make standard clients happier and allow to use more libraries for connecting to KeyRock. In any case, the default comma-separated behaviour should be maintained and ensured due to the new unit tests.

Now that KeyRock uses a custom implementation of oauth-server, the implementation can be simplified avoiding to tests if the scope parameter is an argument array or just the scope string (I think that this requirement is meet just at current version but please, confirm if possible).

@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️

@aarranz
Copy link
Contributor Author

aarranz commented Oct 21, 2021

Dear KeyRock team,

Any comment regarding this? Something to change?

Best regards.

@apozohue10
Copy link
Contributor

Hi @aarranz,

apologies for the delay. I have made some tests to check that everything was ok. I will accept the PR right now.

@apozohue10 apozohue10 merged commit 3b5c9f9 into ging:master Nov 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants