Actually check for auth flows in provider enrollment #2601
Conversation
JAORMX
force-pushed
the
provider-use-authflows
branch
from
2f9e950 to
656a54c
Compare
Signed-off-by: Juan Antonio Osorio <[email protected]>
JAORMX
force-pushed
the
provider-use-authflows
branch
from
656a54c to
013aa38
Compare
| @@ -288,6 +295,11 @@ func (s *Server) StoreProviderToken(ctx context.Context, | |||
| return nil, providerError(err) | |||
| } | |||
|
|
|||
| if !slices.Contains(provider.AuthFlows, db.AuthorizationFlowUserInput) { | |||
There was a problem hiding this comment.
why not call p.SupportsAuthFlow() here? is it because one is using a protobuf constant and the other a db constant? Since we're using the constant verbatim and not from a variable, could we use the protobuf constant here as well?
There was a problem hiding this comment.
I think getProviderFromRequestOrDefault actually returns a db.Provider, not a proto object mentioned in SupportsAuthFlow.
With that said, I'd prefer to have it return something (maybe a ProviderBuilder?) that encapsulates this check more than slices.Contains on a returned database row.
There was a problem hiding this comment.
Should we have a separate PR with that refactor?
There was a problem hiding this comment.
I'm fine with a separate PR if it comes soon and we don't propagate code patters like this over time
| @@ -288,6 +295,11 @@ func (s *Server) StoreProviderToken(ctx context.Context, | |||
| return nil, providerError(err) | |||
| } | |||
|
|
|||
| if !slices.Contains(provider.AuthFlows, db.AuthorizationFlowUserInput) { | |||
There was a problem hiding this comment.
I think getProviderFromRequestOrDefault actually returns a db.Provider, not a proto object mentioned in SupportsAuthFlow.
With that said, I'd prefer to have it return something (maybe a ProviderBuilder?) that encapsulates this check more than slices.Contains on a returned database row.
| @@ -56,6 +58,11 @@ func (s *Server) GetAuthorizationURL(ctx context.Context, | |||
| return nil, providerError(err) | |||
| } | |||
|
|
|||
| if !slices.Contains(provider.AuthFlows, db.AuthorizationFlowOauth2AuthorizationCodeFlow) { | |||
There was a problem hiding this comment.
Can we use provider.SupportsAuthFlow(...) here? I think this is the same comment jakub had down below.
Ideally, provider returned from getProviderFromRequestOrDefault would be either an actual Provider instance, or at least a ProviderBuilder instance. But that's a larger refactor.
|
|
||
| // SupportsAuthFlow returns true if the provider supports the given auth flow | ||
| func (p *Provider) SupportsAuthFlow(flow AuthorizationFlow) bool { | ||
| return slices.Contains(p.GetAuthFlows(), flow) | ||
| } |
There was a problem hiding this comment.
I don't see where this is used?
Summary
This modifies the provider enrollment to actually verify that the provider supports the given auth flows before attempting them.
This is enforced both in the CLI and the server.
Closes: #2518
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: