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.

Sign up for GitHub

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

Jump to bottom

Add provider configuration to the session store #3348

Merged
merged 8 commits into from
May 16, 2024
Merged

Add provider configuration to the session store #3348

merged 8 commits into from
May 16, 2024

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented May 16, 2024

Summary

  • Extend the GetAuthorizationURLRequest protobuf with a config - When GetAuthorizationURLRequest is called, it also creates the session state for the provider about to be created. Let's extend the message to include the config.
  • Add providerClass to GetAuthorizationURLRequest - At the moment the session state treats the name and the class the same, which works because we only have github. Let's pass the name and the state separately. For backwards compatibility, we still use the provider name as the class unless set explicitly.
  • Pass the config from GetAuthorizationURL to a newly added database column in the session_store table - Store the configuration passed through the GetAuthorizationURL to the database so it can be retrieved when actually creating the provider.

Related: #3263

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

A bunch of enrolls

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@jhrozek jhrozek requested a review from a team as a code owner May 16, 2024 12:09
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

JAORMX
JAORMX previously approved these changes May 16, 2024
View reviewed changes
@jhrozek
Extend the GetAuthorizationURLRequest protobuf with a config
58ea21b 
When GetAuthorizationURLRequest is called, it also creates the session
state for the provider about to be created. Let's extend the message to
include the config.
@jhrozek
Add providerClass to GetAuthorizationURLRequest
b55683a 
At the moment the session state treats the name and the class the same,
which works because we only have github. Let's pass the name and the
state separately.

For backwards compatibility, we still use the provider name as the class
unless set explicitly.
@jhrozek
Pass the config from GetAuthorizationURL to a newly added database co…
f15deb2 
…lumn in the session_store table

Store the configuration passed through the GetAuthorizationURL to the
database so it can be retrieved when actually creating the provider.
@jhrozek
Bump database migration version
04d2aab
@jhrozek
Fix tests
9aa3402
@jhrozek
Fix lint
5ce20a7
@jhrozek
Bump the migrations again
ab30aa7
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@coveralls
Copy link

coveralls commented May 16, 2024 •
edited
Loading

Coverage Status

coverage: 49.41% (-0.008%) from 49.418%
when pulling f33cdb3 on jhrozek:config_in_session_store
into e2336a5 on stacklok:main.

@jhrozek jhrozek mentioned this pull request May 16, 2024
Merged
10 tasks
stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Invisible Unicode Characters Detected.

stacklokbot
stacklokbot reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@stacklokbot stacklokbot left a comment

Choose a reason for hiding this comment

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

✅ No Mixed Scripts Detected.

@jhrozek jhrozek merged commit 2f83c54 into mindersec:main May 16, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacklokbot stacklokbot stacklokbot left review comments

@JAORMX JAORMX JAORMX approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jhrozek @coveralls @JAORMX @stacklokbot