Skip to content

Conversation

@samdbmg
Copy link
Member

@samdbmg samdbmg commented Feb 13, 2025

Details

Adds AppNote 0016 proposing approaches to handling authorisation in TAMS, based on the BBC/AWS workshop in Salford in November 2024.

Jira Issue (if relevant)

Jira URL: https://jira.dev.bbc.co.uk/browse/CLOUDFIT-3534

Related PRs

Merge after #113

If merged after any of the following PRs, add the new endpoints they create to the auth logic listings in this PR. If merged before, add that logic in those PRs

Submitter PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • API version has been incremented if necessary
  • ADR status has been updated, and ADR implementation has been recorded
  • Documentation updated (README, etc.)
  • PR added to Jira Issue (if relevant)
  • Follow-up stories added to Jira

Reviewer PR Checks

(tick as appropriate)

  • PR completes task/fixes bug
  • Design makes sense, and fits with our current code base
  • Code is easy to follow
  • PR size is sensible
  • Commit history is sensible and tidy

Info on PRs

The checks above are guidelines. They don't all have to be ticked, but they should all have been considered.

@samdbmg samdbmg requested a review from a team as a code owner February 13, 2025 09:45
@samdbmg samdbmg force-pushed the sammg-add-authorisation-appnote branch from cc16ad9 to bcdf5f3 Compare February 14, 2025 10:39
@j616 j616 force-pushed the sammg-authentication-methods branch from dbafef9 to e25a61a Compare June 18, 2025 09:39
Base automatically changed from sammg-authentication-methods to main June 18, 2025 09:41
@j616 j616 force-pushed the sammg-add-authorisation-appnote branch from bcdf5f3 to 4cd8daf Compare June 19, 2025 10:00
@j616 j616 marked this pull request as draft June 26, 2025 13:51
@j616
Copy link
Contributor

j616 commented Jun 26, 2025

Converted to a draft PR while we make changes

@j616 j616 force-pushed the sammg-add-authorisation-appnote branch 3 times, most recently from fc91efd to bcd2add Compare June 30, 2025 14:55
@j616 j616 force-pushed the sammg-add-authorisation-appnote branch 3 times, most recently from 2cf408f to 9d08f99 Compare July 9, 2025 15:39
Copy link
Member Author

@samdbmg samdbmg left a comment

Choose a reason for hiding this comment

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

This all looks good overall - few nitpicks and suggested wording tweaks inlined

@j616 j616 marked this pull request as ready for review July 11, 2025 16:08
@j616 j616 mentioned this pull request Jul 25, 2025
11 tasks
- name: flow_tag.{name}
in: query
description: |
Filter `referenced_by_flows` on tag values. This option is the same as the `tag.{name}` query parameter on the `/flows/` API endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

My gut feeling is that this should also "filter" the flow id from the first_referenced_by_flow field in the same way. Otherwise if we are using this tag query parameter to provide an early implementation of RBAC/ABAC we would be removing a flow id from referenced_by_flows only to leave it visible in first_referenced_by_fMy gut feeling is that this should also "filter" the flow id from the first_referenced_by_flow field in the same way. Otherwise if we are using this tag query parameter to provide an early implementation of RBAC/ABAC we would be removing a flow id from referenced_by_flowsonly to leave it visible infirst_referenced_by_flow. Just a thought and i'm not "wedded to it".low. Just a thought and i'm not "wedded to it".

Copy link
Contributor

@j616 j616 Sep 24, 2025

Choose a reason for hiding this comment

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

I think you're right. But I think what we choose to do here may be affected by the discussion on whether we are happy to elevate auth classes strait to the core spec. So I'll hold off on this change for now.

- name: flow_tag.{name}
in: query
description: |
Filter `referenced_by_flows` on tag values. This option is the same as the `tag.{name}` query parameter on the `/flows/` API endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

As per comment above about also filtering first_referenced_by_flow.

- name: flow_tag_exists.{name}
in: query
description: |
Filter `referenced_by_flows` on tag names. This option is the same as the `tag_exists.{name}` query parameter on the `/flows/` API endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

As per comment above about also filtering first_referenced_by_flow.

- name: flow_tag_exists.{name}
in: query
description: |
Filter `referenced_by_flows` on tag names. This option is the same as the `tag_exists.{name}` query parameter on the `/flows/` API endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

As per comment above about also filtering first_referenced_by_flow.

@j616 j616 force-pushed the sammg-add-authorisation-appnote branch from 1b1ff86 to 2d15d69 Compare September 24, 2025 09:39
@j616 j616 force-pushed the sammg-add-authorisation-appnote branch from 71e901a to 5db39a0 Compare September 24, 2025 14:39
Note that object instance delete requires `write` as the API will not let a user remove all instances of an object directly.
…t the use of new endpoints.

Note that coarse grained permissions for webhooks now require read, not write, permissions. This is to avoid a privilege escalation attack where a client with write, but not read, permissions uses websockets to perform read operations.
@samdbmg samdbmg mentioned this pull request Sep 29, 2025
11 tasks
samdbmg added a commit that referenced this pull request Oct 1, 2025
Adds an application note describing authorisation in TAMS.
Note that this is the state of the appnote at 01f2140 in
#115 - at that point it was reworked
into multiple PRs and the history flattened.

Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
samdbmg added a commit that referenced this pull request Oct 10, 2025
Adds an application note describing authorisation in TAMS.
Note that this is the state of the appnote at 01f2140 in
#115 - at that point it was reworked
into multiple PRs and the history flattened.

Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
samdbmg added a commit that referenced this pull request Nov 18, 2025
Adds an application note describing authorisation in TAMS.
Note that this is the state of the appnote at 01f2140 in
#115 - at that point it was reworked
into multiple PRs and the history flattened.

Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
samdbmg added a commit that referenced this pull request Nov 19, 2025
Adds an application note describing authorisation in TAMS.
Note that this is the state of the appnote at 01f2140 in
#115 - at that point it was reworked
into multiple PRs and the history flattened.

Co-authored-by: James Sandford <james.sandford@bbc.co.uk>
@samdbmg
Copy link
Member Author

samdbmg commented Nov 19, 2025

Superseded by #154

@samdbmg samdbmg closed this Nov 19, 2025
@samdbmg samdbmg deleted the sammg-add-authorisation-appnote branch November 19, 2025 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants