Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
website/integrations: add engomo #10538
website/integrations: add engomo #10538
Changes from 26 commits
5498ea1
a3d5164
96e13d3
e432e8a
0000434
a33f721
9597792
07e0705
cb11886
cc14b39
978089b
c768a6b
0dcc6d4
99761b8
39e7d28
ac5bbc9
565569c
eba5920
6acf11a
2da3ea0
5d14fb8
53d6f93
44c2eb3
95b6ab9
be74949
546a3a0
215d577
bd514aa
f73e638
be672cd
b15f154
6dce4ed
7c09cd3
7d7e6b8
225df39
29e5366
50a026c
02a4726
365c900
f6e5f6b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know how/what to do with this. Do you mean the name? I can use anything more "standardized" if you want. All my SP's got the
SP-
in front of the apps name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? The Scopes?
We struggled in my company with a huge firewalling company that provides IDP as well (that we have at work). There we figured out that their IDP isn't using any standard scopes. You always have to add them one by one and only all 4 of them did work.
The
offline_access
is only needed if you need access to the app (doesn't make sense without).I also got in contact with the dev/support guys of engomo. They told me exactly what their application needs.
What I think about right now is that we don't need the certificate... at least I guess so. Will check and report back.
EDIT: cert is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When looking at review comments look at the line before the comment starts. in this case it would be the IMPORTANT and that block.
I was saying that i'm not sure the block is needed as it can be guessed and the notice is not present in other integration pages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And since this block is mentioned, could you comma separate each of the scopes as "authentik default oauth mapping" is quite repetitive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I comma seperated now and remove the "note line" ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mabye a header should be added here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First thank you for your help :)
What do you mean with this?
something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment was referencing the
Application:
line.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and at the provider line a little higher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so you mean I should do the
Application:
as this?Application
If so, no problem will do it, but it isn't the way I was doing it on my old PR that was merged in the past.