@high-optician-2097 so my plan will be something like that: 1. access_request_handler.go: NewAccessRequest i will delete code that authenticates client 2. On Handlers i will add 2 methods: CanHandlerRequest and IsClientAuthRequired 3. So we can iterate through token endpoints and if CanHandleRequest return true, we can pass request to this handler (this also will free us from type asserting error in order to check if handler is suitable), but before that we call IsClientAuthRequired, if true then we authenticate client and set it to accessRequest, otherwise not

and this will be breaking changes certainly for version

Current simplified code:

func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session Session) (AccessRequester, error) {
   client, err := f.AuthenticateClient(ctx, r, r.PostForm)
   if err != nil {
      return accessRequest, err
   }
   accessRequest.Client = client

   var found = false
   for _, loader := range f.TokenEndpointHandlers {
      if err := loader.HandleTokenEndpointRequest(ctx, accessRequest); err == nil {
         found = true
      } else if <http://errors.Is|errors.Is>(err, ErrUnknownRequest) {
         // do nothing
      } else if err != nil {
         return accessRequest, err
      }
   }

   if !found {
      return nil, errorsx.WithStack(ErrInvalidRequest)
   }
   return accessRequest, nil
}

After my suggested changes

func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session Session) (AccessRequester, error) {
   for _, loader := range f.TokenEndpointHandlers {
      if !loader.CanHandlerRequest(ctx, accessRequest) {
         continue   
      }  
      if loader.IsClientAuthRequired() {
         client, err := f.AuthenticateClient(ctx, r, r.PostForm)
         if err != nil {
            return accessRequest, err
         }
         accessRequest.Client = client
      }  
      if err := loader.HandleTokenEndpointRequest(ctx, accessRequest); err == nil {
         return accessRequest, err
      }
      return accessRequest, nil
   }

   return nil, errorsx.WithStack(ErrInvalidRequest)
}

Alternative: We can implement a new method instead of NewAccessRequest, something like NewJwtAccessRequest, but i don't like it personally. However it will free us from touching current handlers. But i think it will be hard for Hydra to use it.

I would however revert the logic to

loader.CanSkipClientAuth

!

"loader.CanSkipClientAuth" - no problem

just curios: why you want to revert logic to loader.CanSkipClientAuth?

so I want the default (false) to have client auth

and the non default (true) to skip it

this makes it a bit easier in case you forget to set the value somewhere or something

Can you plz help me with this part

for _, loader := range f.TokenEndpointHandlers {
      if err := loader.HandleTokenEndpointRequest(ctx, accessRequest); err == nil {
         found = true
      } else if <http://errors.Is|errors.Is>(err, ErrUnknownRequest) {
         // do nothing
      } else if err != nil {
         return accessRequest, err
      }
   }

In original code i see, that even if some token endpoint is executed, we still continue to iterate over all endpoints, so it is possible to execute two or more endpoints at the same time. In oauth 2.0 flow it does not make sense. I think only one endpoint should be executed per request, but maybe i am missing some point. Should i preserve this logic or no?

@high-optician-2097 Ok, i looked up code myself and looks like this logic should be preserved, because Open ID Connect Explicit Token handler relies on it

@high-optician-2097 Hello! I would like to discuss with you interface for retrieving public keys, to check signature of auth grant assertion. I am thinking about this interface

type JWTAuthGrantStorage interface {
   GetPublicKey(ctx context.Context, issuer string, subject string, keyId string) (interface{}, error)
   GetPublicKeys(ctx context.Context, issuer string, subject string) ([]interface{}, error)
   GetPublicKeyScopes(ctx context.Context, issuer string, subject string, keyId string) ([]string, error)
}

What do you think?

but then we will need some kind of identifier for this assertion. So i decided to attach scopes to public key

and why current interface missing setters for subject and username in the first place?

becuase they are not used anywhere in fosite

thanks! 😊

While we were developing jwt auth grant, we faced one problem. Oauth2 Client that is used in integration tests, has support for jwt auth grant, but this clients assumes, that scope parameter in inside assertion, as a jwt claim (which goeas againts rfc). We open issue here, you can see details https://github.com/golang/oauth2/issues/461 But for now we must choose the way) We have diffrent options : 1. write own client just for jwt test 2. do not test scope for now in integration test 3. add support for scope in assertion, but we still can't test scope as post parameter then @high-optician-2097 what do you think? because we would like to open PR with supporting jwt auth grant and integration test is the last issue, we must solve.

Also can we start here to discuss how to add support for auth grant jwt in Hydra? Mostly from hydra we need to implement this interface

type JWTAuthGrantStorage interface {
   // GetPublicKey returns public key, issued by 'issuer', and assigned for subject. Public key is used to check
   // signature of jwt assertion in authorization grants.
   GetPublicKey(ctx context.Context, issuer string, subject string, keyId string) (*jose.JSONWebKey, error)

   // GetPublicKeys returns public key, set issued by 'issuer', and assigned for subject.
   GetPublicKeys(ctx context.Context, issuer string, subject string) (*jose.JSONWebKeySet, error)

   // GetPublicKeyScopes returns assigned scope for assertion, identified by public key, issued by 'issuer'.
   GetPublicKeyScopes(ctx context.Context, issuer string, subject string, keyId string) ([]string, error)
}

I was thinking about two variants: 1. we can use existing API to add public key, but create a new endpoint to add relation between public key and its issuer and subject 2. Or we can create new endpoint to add both public key and it's issuer and subject in one request

and yes, forgot to mention, in both variants we still can manage keys through /keys endpoints. E.x delete key will result in deleting binding also

We only need the public key in ORY Hydra correct?

So to be honest - I think we should not do this via the HTTP API. I think this key will not change often, correct? There will also not be hundreds of thousands of keys I think - more like a few?

Also, I thought the “subject” value would be variable - meaning that I can exchange JWT with sub “foo” and “bar” and “baz” for their respective access tokens without any additional allow list

"We only need the public key in ORY Hydra correct?" - only public keys, private is outside of hydra.

"So to be honest - I think we should not do this via the HTTP API. I think this key will not change often, correct? There will also not be hundreds of thousands of keys I think - more like a few?" - If you mean configuration it is not very suited, because main case for auth grant via jwt is service accounts. And we must be able to create them on the fly, so we need to add new public keys to Hydra using api, because service accounts can be created through API as well.

"Also, I thought the “subject” value would be variable - meaning that I can exchange JWT with sub “foo” and “bar” and “baz” for their respective access tokens without any additional allow list" - RFC recommends to check them. The only way is to bind public key to subject also, however we can make it optional. You can bind or not key to subject.

So yes, I think a dedicated API would be good. However, before implementing it in ORY Hydra, could you maybe create an issue or a PR with the planned API layout? We can then go over it and make it the best API we possibly can 🙂

How you would like to see API layout, in what form?

just like “GET /path” with parameters “asdf”, “bfdsa”, …

@high-optician-2097 Here it is https://github.com/ory/hydra/issues/2229

@high-optician-2097 here goes fosite PR for adding jwt grant. Hydra in process)

@high-optician-2097 still waiting your ping)

🙂

Is it office or home?

@high-optician-2097 just reminding about my question 😃 "Hi again! How are you, fine? I need your help) So looking back at our grants https://github.com/ory/hydra/issues/2229 i am wondering how to delete expired grants. Looking up the code in Hydra code, i see that there are two ways, that is currently used: 1) delete something expired in the same transaction, where new  something is inserted (looking at SetClientAssertionJWT method in persister) 2) or by having a dedicated handler (like FlushHandler), which external client must call with some periodicity. What is better way for grants, what you think?"

so nice !

making themselves target for punching? 😂

This black squares is so terrifying actually!

haha yeah..

@clean-mouse-35824 as promised, looking into it now

@clean-mouse-35824 can you please enable write access for maintainers?

in the pr

where this option?

but doesn’t matter I can solve it differently

thats why i dont have this checkbox(

the ci is a bit difficult..let’s see if it passes now

@clean-mouse-35824 had to resolve some regression issues in the tests but it’s all done now

@magnificent-energy-493 please make sure to put vladimir on the mailout list 🙂

released as 0.37 - thank you for your hard work!

Please include this guy too

https://github.com/seliverstov-tinkoff - he helped with tests)

Ok, it is now time to open PR for Hydra after some tweaks )))

@high-optician-2097 Hi! Will you able to look at https://github.com/ory/hydra/pull/2384 ?

@high-optician-2097 Hi! I noticed that

make sdk

no longer works in Makefile in Hydra, because this file

bash <(curl <https://raw.githubusercontent.com/ory/cli/master/install.sh>) -b .bin v0.0.72

looks like no longer exists, how can make to work it again?

$ bash <(curl https://raw.githubusercontent.com/ory/meta/master/install.sh) -b .bin v0.0.72 ory

that works

@high-optician-2097 can you please review this https://github.com/ory/hydra/pull/2384 if you have time? we really want it to be merged 😃. Everything is done there

@high-optician-2097 Hi again! I think this PR https://github.com/ory/hydra/pull/2384 is ready for you 😃

@high-optician-2097 Is it possible to receive Christmas gift from you in the form of this PR merged https://github.com/ory/hydra/pull/2384? 😋

Would be wonderful if we can merge it before 2022 Year